Refactor Plugin VM installation flow to be driven by the installer

The Plugin VM installation flow currently is driven by the UI side, with
it calling into the installer after each step to start the next step.
This CL changes this so after it starts the installation, the UI side
doesn't need to call into the installer (aside from cancelling) and just
needs to update what it displays to the user.

This makes the flow easier to follow and reduces the coupling between
the UI and the installer backend. It also makes it easier to make
changes to the flow, for example adding additional steps.

Bug: 1038816
Change-Id: Ice6a32a572c16a2bc32e86c166ff6e486791d877
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1989722Reviewed-by: Julian Watson <juwa@google.com>
Commit-Queue: Timothy Loh <timloh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#730058}

chrome/browser/chromeos/plugin_vm/plugin_vm_drive_image_download_service.cc

@@ -102,7 +102,7 @@ plugin_vm::PluginVmInstaller::FailureReason ConvertToFailureReason(
       NOTREACHED();
       // This is only used to avoid compiler warnings, it is
       // not actually reachable.
-      return FailureReason::LOGIC_ERROR;
+      return FailureReason::DOWNLOAD_FAILED_UNKNOWN;
   }
 }
 }  // namespace

chrome/browser/chromeos/plugin_vm/plugin_vm_installer.cc

@@ -51,7 +51,7 @@ bool PluginVmInstaller::IsProcessing() {
   return State::NOT_STARTED < state_ && state_ < State::CONFIGURED;
 }
 
-void PluginVmInstaller::StartDlcDownload() {
+void PluginVmInstaller::Start() {
   if (IsProcessing()) {
     LOG(ERROR) << "Download of a PluginVm image couldn't be started as"
                << " another PluginVm image is currently being processed "
@@ -69,18 +69,37 @@ void PluginVmInstaller::StartDlcDownload() {
     return;
   }
 
-  State prev_state = state_;
+  StartDlcDownload();
+}
+
+void PluginVmInstaller::Cancel() {
+  switch (state_) {
+    case State::DOWNLOADING_DLC:
+      CancelDlcDownload();
+      return;
+    case State::DOWNLOADING:
+      CancelDownload();
+      return;
+    case State::IMPORTING:
+      CancelImport();
+      return;
+    default:
+      LOG(ERROR) << "Tried to cancel installation from unexpected state "
+                 << GetStateName(state_);
+      return;
+  }
+}
+
+void PluginVmInstaller::StartDlcDownload() {
   state_ = State::DOWNLOADING_DLC;
   dlc_download_start_tick_ = base::TimeTicks::Now();
 
-  if (prev_state != State::DOWNLOAD_DLC_CANCELLED) {
-    chromeos::DlcserviceClient::Get()->Install(
-        dlc_module_list_,
-        base::BindOnce(&PluginVmInstaller::OnDlcDownloadCompleted,
-                       weak_ptr_factory_.GetWeakPtr()),
-        base::BindRepeating(&PluginVmInstaller::OnDlcDownloadProgressUpdated,
-                            weak_ptr_factory_.GetWeakPtr()));
-  }
+  chromeos::DlcserviceClient::Get()->Install(
+      dlc_module_list_,
+      base::BindOnce(&PluginVmInstaller::OnDlcDownloadCompleted,
+                     weak_ptr_factory_.GetWeakPtr()),
+      base::BindRepeating(&PluginVmInstaller::OnDlcDownloadProgressUpdated,
+                          weak_ptr_factory_.GetWeakPtr()));
 
   if (observer_)
     observer_->OnDlcDownloadStarted();
@@ -88,20 +107,12 @@ void PluginVmInstaller::StartDlcDownload() {
 
 void PluginVmInstaller::CancelDlcDownload() {
   state_ = State::DOWNLOAD_DLC_CANCELLED;
-
-  if (observer_)
-    observer_->OnDlcDownloadCancelled();
 }
 
 void PluginVmInstaller::StartDownload() {
-  if (state_ != State::DOWNLOADED_DLC) {
-    LOG(ERROR) << "Download of a PluginVm image couldn't be started as "
-               << "StartDlcDownload() was not called prior.";
-    OnDownloadFailed(FailureReason::DLC_DOWNLOAD_NOT_STARTED);
-    return;
-  }
-
+  DCHECK_EQ(state_, State::DOWNLOADING_DLC);
   state_ = State::DOWNLOADING;
+
   GURL url = GetPluginVmImageDownloadUrl();
   if (url.is_empty()) {
     OnDownloadFailed(FailureReason::INVALID_IMAGE_URL);
@@ -136,8 +147,9 @@ void PluginVmInstaller::CancelDownload() {
 }
 
 void PluginVmInstaller::OnDlcDownloadProgressUpdated(double progress) {
-  if (state_ != State::DOWNLOADING_DLC)
+  if (state_ == State::DOWNLOAD_DLC_CANCELLED)
     return;
+  DCHECK_EQ(state_, State::DOWNLOADING_DLC);
 
   if (observer_)
     observer_->OnDlcDownloadProgressUpdated(
@@ -147,21 +159,27 @@ void PluginVmInstaller::OnDlcDownloadProgressUpdated(double progress) {
 void PluginVmInstaller::OnDlcDownloadCompleted(
     const std::string& err,
     const dlcservice::DlcModuleList& dlc_module_list) {
-  if (state_ != State::DOWNLOADING_DLC)
+  if (state_ == State::DOWNLOAD_DLC_CANCELLED) {
+    if (observer_)
+      observer_->OnDlcDownloadCancelled();
+    state_ = State::NOT_STARTED;
     return;
+  }
+  DCHECK_EQ(state_, State::DOWNLOADING_DLC);
 
   // TODO(kimjae): Remove this check once PluginVM is converted to DLC.
   if (err == dlcservice::kErrorInvalidDlc) {
     LOG(ERROR) << "PluginVM DLC is probably not supported, skipping install.";
   } else if (err != dlcservice::kErrorNone) {
+    state_ = State::DOWNLOAD_DLC_FAILED;
     if (observer_)
       observer_->OnDownloadFailed(FailureReason::DLC_DOWNLOAD_FAILED);
     return;
   }
 
-  state_ = State::DOWNLOADED_DLC;
   if (observer_)
     observer_->OnDlcDownloadCompleted();
+  StartDownload();
 }
 
 void PluginVmInstaller::OnDownloadStarted() {
@@ -192,10 +210,10 @@ void PluginVmInstaller::OnDownloadCompleted(
     return;
   }
 
-  state_ = State::DOWNLOADED;
   if (observer_)
     observer_->OnDownloadCompleted();
   RecordPluginVmImageDownloadedSizeHistogram(info.bytes_downloaded);
+  StartImport();
 }
 
 void PluginVmInstaller::OnDownloadCancelled() {
@@ -228,14 +246,7 @@ void PluginVmInstaller::OnDownloadFailed(FailureReason reason) {
 }
 
 void PluginVmInstaller::StartImport() {
-  if (state_ != State::DOWNLOADED) {
-    LOG(ERROR) << "Importing of PluginVm image couldn't proceed as current "
-               << "state is " << GetStateName(state_) << " not "
-               << GetStateName(State::DOWNLOADED);
-    OnImported(FailureReason::LOGIC_ERROR);
-    return;
-  }
-
+  DCHECK_EQ(state_, State::DOWNLOADING);
   state_ = State::IMPORTING;
 
   VLOG(1) << "Starting PluginVm dispatcher service";
@@ -534,14 +545,10 @@ std::string PluginVmInstaller::GetStateName(State state) {
       return "DOWNLOADING_DLC";
     case State::DOWNLOAD_DLC_CANCELLED:
       return "DOWNLOAD_DLC_CANCELLED";
-    case State::DOWNLOADED_DLC:
-      return "DOWNLOADED_DLC";
     case State::DOWNLOADING:
       return "DOWNLOADING";
     case State::DOWNLOAD_CANCELLED:
       return "DOWNLOAD_CANCELLED";
-    case State::DOWNLOADED:
-      return "DOWNLOADED";
     case State::IMPORTING:
       return "IMPORTING";
     case State::IMPORT_CANCELLED:

chrome/browser/chromeos/plugin_vm/plugin_vm_installer.h

@@ -32,14 +32,6 @@ class PluginVmDriveImageDownloadService;
 // including downloading this image from url specified by the user policy,
 // and importing the downloaded image archive using concierge D-Bus services.
 //
-// Only one PluginVm image at a time is allowed to be processed.
-// Methods StartDownload() and StartImport() should be
-// called in this order. Image processing might be interrupted by
-// calling the corresponding cancel methods. If one of the methods mentioned is
-// called not in the correct order or before the previous state is finished then
-// associated fail method will be called by the installer and image processing
-// will be interrupted.
-//
 // This class uses one of two different objects for handling file downloads. If
 // the image is hosted on Drive, a PluginVmDriveImageDownloadService object is
 // used due to the need for using the Drive API. In all other cases, the
@@ -50,7 +42,7 @@ class PluginVmInstaller : public KeyedService,
   // FailureReasons values can be shown to the user. Do not reorder or renumber
   // these values without careful consideration.
   enum class FailureReason {
-    LOGIC_ERROR = 0,
+    // LOGIC_ERROR = 0,
     SIGNAL_NOT_CONNECTED = 1,
     OPERATION_IN_PROGRESS = 2,
     NOT_ALLOWED = 3,
@@ -67,7 +59,7 @@ class PluginVmInstaller : public KeyedService,
     INVALID_IMPORT_RESPONSE = 14,
     IMAGE_IMPORT_FAILED = 15,
     DLC_DOWNLOAD_FAILED = 16,
-    DLC_DOWNLOAD_NOT_STARTED = 17,
+    // DLC_DOWNLOAD_NOT_STARTED = 17,
   };
 
   // Observer class for the PluginVm image related events.
@@ -98,26 +90,10 @@ class PluginVmInstaller : public KeyedService,
   // Returns true if installer is processing a PluginVm image at the moment.
   bool IsProcessing();
 
-  // Initiates the PluginVM DLC download, should always be called before
-  // |StartDownload()|, which initiates the PluginVM image download.
-  void StartDlcDownload();
-  // DLC(s) cannot be currently cancelled when initiated, so this will cause
-  // progress and completed install callbacks to be blocked to the observer if
-  // there is an install taking place.
-  void CancelDlcDownload();
-
-  void StartDownload();
-  // Cancels the download of PluginVm image finishing the image processing.
-  // Downloaded PluginVm image archive is being deleted.
-  void CancelDownload();
-
-  // Proceed with importing (unzipping and registering) of the VM image.
-  // Should be called when download of PluginVm image is successfully completed.
-  // If called in other cases - importing is not started and
-  // OnImported(false /* success */) is called.
-  void StartImport();
-  // Makes a call to concierge to cancel the import.
-  void CancelImport();
+  // Start the installation. Progress updates will be sent to the observer.
+  void Start();
+  // Cancel the installation.
+  void Cancel();
 
   void SetObserver(Observer* observer);
   void RemoveObserver();
@@ -155,16 +131,30 @@ class PluginVmInstaller : public KeyedService,
   std::string GetCurrentDownloadGuidForTesting();
 
  private:
+  void StartDlcDownload();
+  void StartDownload();
+  void StartImport();
+
+  // DLC(s) cannot be currently cancelled when initiated, so this will cause
+  // progress and completed install callbacks to be blocked to the observer if
+  // there is an install taking place.
+  void CancelDlcDownload();
+  // Cancels the download of PluginVm image finishing the image processing.
+  // Downloaded PluginVm image archive is being deleted.
+  void CancelDownload();
+  // Makes a call to concierge to cancel the import.
+  void CancelImport();
+
   enum class State {
     NOT_STARTED,
     DOWNLOADING_DLC,
     DOWNLOAD_DLC_CANCELLED,
-    DOWNLOADED_DLC,
     DOWNLOADING,
     DOWNLOAD_CANCELLED,
-    DOWNLOADED,
     IMPORTING,
     IMPORT_CANCELLED,
+    // TODO(timloh): We treat these all the same as NOT_STARTED. Consider
+    // merging these together.
     CONFIGURED,
     DOWNLOAD_DLC_FAILED,
     DOWNLOAD_FAILED,

chrome/browser/ui/views/plugin_vm/plugin_vm_installer_view.cc

@@ -187,30 +187,34 @@ bool PluginVmInstallerView::Accept() {
   DCHECK_EQ(state_, State::ERROR);
   // Retry button has been clicked to retry setting of PluginVm environment
   // after error occurred.
-  StartPluginVmImageDownload();
+  StartInstallation();
   return false;
 }
 
 bool PluginVmInstallerView::Cancel() {
-  if (state_ == State::DOWNLOADING_DLC ||
-      state_ == State::START_DLC_DOWNLOADING) {
-    plugin_vm_installer_->CancelDlcDownload();
-
-    plugin_vm::RecordPluginVmSetupResultHistogram(
-        plugin_vm::PluginVmSetupResult::kUserCancelledDownloadingPluginVmDlc);
-  }
-  if (state_ == State::DOWNLOADING || state_ == State::START_DOWNLOADING) {
-    plugin_vm_installer_->CancelDownload();
-
-    plugin_vm::RecordPluginVmSetupResultHistogram(
-        plugin_vm::PluginVmSetupResult::kUserCancelledDownloadingPluginVmImage);
+  switch (state_) {
+    case State::DOWNLOADING_DLC:
+    case State::START_DLC_DOWNLOADING:
+      plugin_vm::RecordPluginVmSetupResultHistogram(
+          plugin_vm::PluginVmSetupResult::kUserCancelledDownloadingPluginVmDlc);
+      break;
+    case State::DOWNLOADING:
+    case State::START_DOWNLOADING:
+      plugin_vm::RecordPluginVmSetupResultHistogram(
+          plugin_vm::PluginVmSetupResult::
+              kUserCancelledDownloadingPluginVmImage);
+      break;
+    case State::IMPORTING:
+      plugin_vm::RecordPluginVmSetupResultHistogram(
+          plugin_vm::PluginVmSetupResult::kUserCancelledImportingPluginVmImage);
+      break;
+    case State::ERROR:
+      return true;
+    default:
+      NOTREACHED();
   }
-  if (state_ == State::IMPORTING) {
-    plugin_vm_installer_->CancelImport();
 
-    plugin_vm::RecordPluginVmSetupResultHistogram(
-        plugin_vm::PluginVmSetupResult::kUserCancelledImportingPluginVmImage);
-  }
+  plugin_vm_installer_->Cancel();
 
   return true;
 }
@@ -241,8 +245,6 @@ void PluginVmInstallerView::OnDlcDownloadCompleted() {
 
   state_ = State::START_DOWNLOADING;
   OnStateUpdated();
-
-  plugin_vm_installer_->StartDownload();
 }
 
 void PluginVmInstallerView::OnDlcDownloadCancelled() {
@@ -275,7 +277,6 @@ void PluginVmInstallerView::OnDownloadCompleted() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK_EQ(state_, State::DOWNLOADING);
 
-  plugin_vm_installer_->StartImport();
   state_ = State::IMPORTING;
   OnStateUpdated();
 }
@@ -378,7 +379,6 @@ base::string16 PluginVmInstallerView::GetMessage() const {
       DCHECK(reason_);
       switch (*reason_) {
         default:
-        case Reason::LOGIC_ERROR:
         case Reason::SIGNAL_NOT_CONNECTED:
         case Reason::OPERATION_IN_PROGRESS:
         case Reason::UNEXPECTED_DISK_IMAGE_STATUS:
@@ -490,13 +490,12 @@ void PluginVmInstallerView::AddedToWidget() {
   }
 
   if (state_ == State::START_DOWNLOADING)
-    StartPluginVmImageDownload();
+    StartInstallation();
   else
     OnStateUpdated();
 }
 
 void PluginVmInstallerView::OnStateUpdated() {
-  // TODO(https://crbug.com/1017511): display failure reasons.
   SetBigMessageLabel();
   SetMessageLabel();
   SetBigImage();
@@ -620,7 +619,7 @@ void PluginVmInstallerView::SetBigImage() {
           IDR_PLUGIN_VM_INSTALLER));
 }
 
-void PluginVmInstallerView::StartPluginVmImageDownload() {
+void PluginVmInstallerView::StartInstallation() {
   // In each case setup starts from this function (when dialog is opened or
   // retry button is clicked).
   setup_start_tick_ = base::TimeTicks::Now();
@@ -629,5 +628,5 @@ void PluginVmInstallerView::StartPluginVmImageDownload() {
   OnStateUpdated();
 
   plugin_vm_installer_->SetObserver(this);
-  plugin_vm_installer_->StartDlcDownload();
+  plugin_vm_installer_->Start();
 }

chrome/browser/ui/views/plugin_vm/plugin_vm_installer_view.h

@@ -62,6 +62,8 @@ class PluginVmInstallerView : public views::BubbleDialogDelegateView,
 
  private:
   enum class State {
+    // TODO(timloh): We should be able to simplify this by removing the
+    // START_DLC_DOWNLOADING and START_DOWNLOADING states.
     START_DLC_DOWNLOADING,  // PluginVm DLC downloading should be started.
     DOWNLOADING_DLC,    // PluginVm DLC downloading and installing in progress.
     START_DOWNLOADING,  // PluginVm image downloading should be started.
@@ -90,7 +92,7 @@ class PluginVmInstallerView : public views::BubbleDialogDelegateView,
   void SetMessageLabel();
   void SetBigImage();
 
-  void StartPluginVmImageDownload();
+  void StartInstallation();
 
   Profile* profile_ = nullptr;
   plugin_vm::PluginVmInstaller* plugin_vm_installer_ = nullptr;