borealis: Changed context_manager_impl to have a unique_ptr<context>

We will be implementing vm shutdown in the UI, the most expedient way is
to have a means of telling the context_manager to reset its state. This
proves somewhat complicated, so we submit this refactoring initially
which does not change anything functional.

How this works:
 - we make the context a unique_ptr so that we can easily reset state by
   deleting it. This is easier than reinitializing it in place.
 - I removed several redundant fields of the context_manager_impl which
   were used to track state. The state of the manager is now implicit
   in the state of some of its members (particularly, the context and
   the task queue) rather than explicit in those variables.

Bug: b/172006764
Change-Id: I8f897f125beee6dd5eb55c784c310f133094c180
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2507234
Commit-Queue: Nic Hollingum <hollingum@google.com>
Reviewed-by: Daniel Ng <danielng@google.com>
Cr-Commit-Position: refs/heads/master@{#823738}

chrome/browser/chromeos/borealis/borealis_context.h

@@ -28,9 +28,6 @@ class BorealisContext {
 
   Profile* profile() const { return profile_; }
 
-  bool borealis_running() const { return borealis_running_; }
-  void set_borealis_running(bool success) { borealis_running_ = success; }
-
   const std::string& vm_name() const { return vm_name_; }
   void set_vm_name(std::string vm_name) { vm_name_ = std::move(vm_name); }
 

chrome/browser/chromeos/borealis/borealis_context_manager_impl.cc

@@ -7,11 +7,17 @@
 #include <ostream>
 
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
+#include "chrome/browser/chromeos/borealis/borealis_context.h"
 #include "chrome/browser/chromeos/borealis/borealis_context_manager.h"
 #include "chrome/browser/chromeos/borealis/borealis_task.h"
 
 namespace {
 
+// We use a hard-coded name. When multi-instance becomes a feature we'll
+// need to determine the name instead.
+constexpr char kBorealisVmName[] = "borealis";
+
 std::ostream& operator<<(std::ostream& stream,
                          borealis::BorealisContextManager::Status status) {
   switch (status) {
@@ -33,18 +39,19 @@ std::ostream& operator<<(std::ostream& stream,
 namespace borealis {
 
 BorealisContextManagerImpl::BorealisContextManagerImpl(Profile* profile)
-    : profile_(profile), context_(profile_) {}
+    : profile_(profile) {}
 
 BorealisContextManagerImpl::~BorealisContextManagerImpl() = default;
 
 void BorealisContextManagerImpl::StartBorealis(ResultCallback callback) {
-  if (is_borealis_running_) {
+  if (context_ && task_queue_.empty()) {
     std::move(callback).Run(GetResult());
     return;
   }
   AddCallback(std::move(callback));
-  if (!is_borealis_starting_) {
-    is_borealis_starting_ = true;
+  if (!context_) {
+    context_ = base::WrapUnique(new BorealisContext(profile_));
+    context_->set_vm_name(kBorealisVmName);
     task_queue_ = GetTasks();
     NextTask();
   }
@@ -52,15 +59,12 @@ void BorealisContextManagerImpl::StartBorealis(ResultCallback callback) {
 
 base::queue<std::unique_ptr<BorealisTask>>
 BorealisContextManagerImpl::GetTasks() {
-  // We use a hard-coded name. When multi-instance becomes a feature we'll
-  // need to determine the name instead.
-  context_.set_vm_name("borealis");
   base::queue<std::unique_ptr<BorealisTask>> task_queue;
   task_queue.push(std::make_unique<MountDlc>());
   task_queue.push(std::make_unique<CreateDiskImage>());
   task_queue.push(std::make_unique<StartBorealisVm>());
-  task_queue.push(std::make_unique<AwaitBorealisStartup>(context_.profile(),
-                                                         context_.vm_name()));
+  task_queue.push(
+      std::make_unique<AwaitBorealisStartup>(profile_, kBorealisVmName));
   return task_queue;
 }
 
@@ -70,21 +74,18 @@ void BorealisContextManagerImpl::AddCallback(ResultCallback callback) {
 
 void BorealisContextManagerImpl::NextTask() {
   if (task_queue_.empty()) {
-    context_.set_borealis_running(true);
-    is_borealis_running_ = true;
     startup_status_ = Status::kSuccess;
     OnQueueComplete();
     return;
   }
-  current_task_ = std::move(task_queue_.front());
-  task_queue_.pop();
-  current_task_->Run(&context_,
-                     base::BindOnce(&BorealisContextManagerImpl::TaskCallback,
-                                    weak_factory_.GetWeakPtr()));
+  task_queue_.front()->Run(
+      context_.get(), base::BindOnce(&BorealisContextManagerImpl::TaskCallback,
+                                     weak_factory_.GetWeakPtr()));
 }
 
 void BorealisContextManagerImpl::TaskCallback(Status status,
                                               std::string error) {
+  task_queue_.pop();
   startup_status_ = status;
   if (startup_status_ == Status::kSuccess) {
     NextTask();
@@ -97,7 +98,6 @@ void BorealisContextManagerImpl::TaskCallback(Status status,
 }
 
 void BorealisContextManagerImpl::OnQueueComplete() {
-  is_borealis_starting_ = false;
   while (!callback_queue_.empty()) {
     ResultCallback callback = std::move(callback_queue_.front());
     callback_queue_.pop();
@@ -107,7 +107,7 @@ void BorealisContextManagerImpl::OnQueueComplete() {
 
 BorealisContextManager::Result BorealisContextManagerImpl::GetResult() {
   if (startup_status_ == Status::kSuccess) {
-    return BorealisContextManager::Result(&context_);
+    return BorealisContextManager::Result(context_.get());
   }
   return BorealisContextManager::Result(startup_status_, startup_error_);
 }

chrome/browser/chromeos/borealis/borealis_context_manager_impl.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_CHROMEOS_BOREALIS_BOREALIS_CONTEXT_MANAGER_IMPL_H_
 #define CHROME_BROWSER_CHROMEOS_BOREALIS_BOREALIS_CONTEXT_MANAGER_IMPL_H_
 
+#include <memory>
+
 #include "base/containers/queue.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/chromeos/borealis/borealis_context.h"
@@ -41,16 +43,12 @@ class BorealisContextManagerImpl : public BorealisContextManager {
   // error if it doesn't).
   BorealisContextManager::Result GetResult();
 
-  bool is_borealis_running_ = false;
-  bool is_borealis_starting_ = false;
-
   Profile* profile_ = nullptr;
   BorealisContextManager::Status startup_status_ = Status::kSuccess;
   std::string startup_error_;
-  BorealisContext context_;
+  std::unique_ptr<BorealisContext> context_;
   base::queue<ResultCallback> callback_queue_;
   base::queue<std::unique_ptr<BorealisTask>> task_queue_;
-  std::unique_ptr<BorealisTask> current_task_;
 
   base::WeakPtrFactory<BorealisContextManagerImpl> weak_factory_{this};
 };

chrome/browser/chromeos/borealis/borealis_context_manager_unittest.cc

@@ -125,8 +125,11 @@ TEST_F(BorealisContextManagerTest, NoTasksImpliesSuccess) {
   BorealisContextManagerImplForTesting context_manager(
       profile_.get(), /*tasks=*/0, /*success=*/true);
   EXPECT_CALL(callback_expectation, Callback(testing::_))
-      .WillOnce(testing::Invoke(
-          [](BorealisContextManager::Result result) { result.Ok(); }));
+      .WillOnce(testing::Invoke([](BorealisContextManager::Result result) {
+        EXPECT_TRUE(result.Ok());
+        // Even with no tasks, the context will give the VM a name.
+        EXPECT_EQ(result.Success().vm_name(), "borealis");
+      }));
   context_manager.StartBorealis(callback_expectation.GetCallback());
   task_environment_.RunUntilIdle();
 }