Tidy up PluginVmInstaller declaration order and header comments

plugin_vm_installer.h:
- Update declaration order to explicitly show installation flow,
and match style guide.
- Make a couple of internal functions private
- Update comments to be clearer/simpler/etc.

plugin_vm_installer.cc changes:
- Re-order function definitions in .cc to match header file

Bug: 1063748
Change-Id: I4618d5a3845d8ac381063cec0ae1adc798364702
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2576967
Commit-Queue: Timothy Loh <timloh@chromium.org>
Reviewed-by: Jason Lin <lxj@google.com>
Cr-Commit-Position: refs/heads/master@{#835116}

chrome/browser/chromeos/plugin_vm/plugin_vm_installer.cc

@@ -108,11 +108,10 @@ PluginVmSetupResult BucketForCancelledInstall(
 
 }  // namespace
 
-PluginVmInstaller::~PluginVmInstaller() = default;
-
-bool PluginVmInstaller::IsProcessing() {
-  return state_ != State::kIdle;
-}
+PluginVmInstaller::PluginVmInstaller(Profile* profile)
+    : profile_(profile),
+      download_service_(
+          DownloadServiceFactory::GetForKey(profile->GetProfileKey())) {}
 
 base::Optional<PluginVmInstaller::FailureReason> PluginVmInstaller::Start() {
   if (IsProcessing()) {
@@ -182,6 +181,143 @@ void PluginVmInstaller::Cancel() {
   }
 }
 
+bool PluginVmInstaller::IsProcessing() {
+  return state_ != State::kIdle;
+}
+
+void PluginVmInstaller::SetObserver(Observer* observer) {
+  observer_ = observer;
+}
+
+void PluginVmInstaller::RemoveObserver() {
+  observer_ = nullptr;
+}
+
+std::string PluginVmInstaller::GetCurrentDownloadGuid() {
+  return current_download_guid_;
+}
+
+void PluginVmInstaller::OnDownloadStarted() {}
+
+void PluginVmInstaller::OnDownloadProgressUpdated(uint64_t bytes_downloaded,
+                                                  int64_t content_length) {
+  DCHECK_EQ(installing_state_, InstallingState::kDownloadingImage);
+  if (observer_)
+    observer_->OnDownloadProgressUpdated(bytes_downloaded, content_length);
+
+  if (content_length <= 0)
+    content_length = kDownloadSizeFallbackEstimate;
+
+  UpdateProgress(
+      std::min(1., static_cast<double>(bytes_downloaded) / content_length));
+}
+
+void PluginVmInstaller::OnDownloadCompleted(
+    const download::CompletionInfo& info) {
+  downloaded_image_ = info.path;
+  downloaded_image_size_ = info.bytes_downloaded;
+  current_download_guid_.clear();
+
+  if (downloaded_image_for_testing_)
+    downloaded_image_ = downloaded_image_for_testing_.value();
+
+  if (!VerifyDownload(info.hash256)) {
+    OnDownloadFailed(FailureReason::HASH_MISMATCH);
+    return;
+  }
+
+  RecordPluginVmImageDownloadedSizeHistogram(info.bytes_downloaded);
+  StartImport();
+}
+
+void PluginVmInstaller::OnDownloadFailed(FailureReason reason) {
+  RemoveTemporaryImageIfExists();
+  current_download_guid_.clear();
+
+  if (using_drive_download_service_) {
+    drive_download_service_->ResetState();
+    using_drive_download_service_ = false;
+  }
+
+  InstallFailed(reason);
+}
+
+void PluginVmInstaller::OnDiskImageProgress(
+    const vm_tools::concierge::DiskImageStatusResponse& signal) {
+  if (signal.command_uuid() != current_import_command_uuid_)
+    return;
+
+  const uint64_t percent_completed = signal.progress();
+  const vm_tools::concierge::DiskImageStatus status = signal.status();
+
+  switch (status) {
+    case vm_tools::concierge::DiskImageStatus::DISK_STATUS_CREATED:
+      VLOG(1) << "Disk image status indicates that importing is done.";
+      RequestFinalStatus();
+      return;
+    case vm_tools::concierge::DiskImageStatus::DISK_STATUS_IN_PROGRESS:
+      UpdateProgress(percent_completed / 100.);
+      return;
+    default:
+      LOG(ERROR) << "Disk image status signal has status: " << status
+                 << " with error message: " << signal.failure_reason()
+                 << " and current progress: " << percent_completed;
+      OnImported(FailureReason::UNEXPECTED_DISK_IMAGE_STATUS);
+      return;
+  }
+}
+
+bool PluginVmInstaller::VerifyDownload(
+    const std::string& downloaded_archive_hash) {
+  if (downloaded_archive_hash.empty()) {
+    LOG(ERROR) << "No hash found for downloaded PluginVm image archive";
+    return false;
+  }
+  const base::Value* plugin_vm_image_hash_ptr =
+      profile_->GetPrefs()
+          ->GetDictionary(prefs::kPluginVmImage)
+          ->FindKey("hash");
+  if (!plugin_vm_image_hash_ptr) {
+    LOG(ERROR) << "Hash of PluginVm image is not specified";
+    return false;
+  }
+  std::string plugin_vm_image_hash = plugin_vm_image_hash_ptr->GetString();
+
+  if (!base::EqualsCaseInsensitiveASCII(plugin_vm_image_hash,
+                                        downloaded_archive_hash)) {
+    LOG(ERROR) << "Downloaded PluginVm image archive hash ("
+               << downloaded_archive_hash << ") doesn't match "
+               << "hash specified by the PluginVmImage policy ("
+               << plugin_vm_image_hash << ")";
+    return false;
+  }
+
+  return true;
+}
+
+int64_t PluginVmInstaller::RequiredFreeDiskSpace() {
+  return static_cast<int64_t>(profile_->GetPrefs()->GetInteger(
+             prefs::kPluginVmRequiredFreeDiskSpaceGB)) *
+         kBytesPerGigabyte;
+}
+
+void PluginVmInstaller::SetDownloadServiceForTesting(
+    download::DownloadService* download_service) {
+  download_service_ = download_service;
+}
+
+void PluginVmInstaller::SetDownloadedImageForTesting(
+    const base::FilePath& downloaded_image) {
+  downloaded_image_for_testing_ = downloaded_image;
+}
+
+void PluginVmInstaller::SetDriveDownloadServiceForTesting(
+    std::unique_ptr<PluginVmDriveImageDownloadService> drive_download_service) {
+  drive_download_service_ = std::move(drive_download_service);
+}
+
+PluginVmInstaller::~PluginVmInstaller() = default;
+
 void PluginVmInstaller::CheckLicense() {
   UpdateInstallingState(InstallingState::kCheckingLicense);
 
@@ -312,69 +448,6 @@ void PluginVmInstaller::StartDlcDownload() {
                           weak_ptr_factory_.GetWeakPtr()));
 }
 
-void PluginVmInstaller::StartDispatcher() {
-  DCHECK_EQ(installing_state_, InstallingState::kDownloadingDlc);
-  UpdateInstallingState(InstallingState::kStartingDispatcher);
-
-  PluginVmManagerFactory::GetForProfile(profile_)->StartDispatcher(
-      base::BindOnce(&PluginVmInstaller::OnDispatcherStarted,
-                     weak_ptr_factory_.GetWeakPtr()));
-}
-
-void PluginVmInstaller::OnDispatcherStarted(bool success) {
-  if (state_ == State::kCancelling) {
-    CancelFinished();
-    return;
-  }
-
-  if (!success) {
-    InstallFailed(FailureReason::DISPATCHER_NOT_AVAILABLE);
-    return;
-  }
-
-  StartDownload();
-}
-
-void PluginVmInstaller::StartDownload() {
-  DCHECK_EQ(installing_state_, InstallingState::kStartingDispatcher);
-  UpdateInstallingState(InstallingState::kDownloadingImage);
-  UpdateProgress(/*state_progress=*/0);
-
-  GURL url = GetPluginVmImageDownloadUrl();
-  // This may have changed since running StartDlcDownload.
-  if (!url.is_valid()) {
-    InstallFailed(FailureReason::INVALID_IMAGE_URL);
-    return;
-  }
-
-  base::Optional<std::string> drive_id = GetIdFromDriveUrl(url);
-  using_drive_download_service_ = drive_id.has_value();
-
-  if (using_drive_download_service_) {
-    if (!drive_download_service_) {
-      drive_download_service_ =
-          std::make_unique<PluginVmDriveImageDownloadService>(this, profile_);
-    } else {
-      drive_download_service_->ResetState();
-    }
-
-    drive_download_service_->StartDownload(drive_id.value());
-  } else {
-    download_service_->StartDownload(GetDownloadParams(url));
-  }
-}
-
-void PluginVmInstaller::CancelDownload() {
-  if (using_drive_download_service_) {
-    DCHECK(drive_download_service_);
-    drive_download_service_->CancelDownload();
-  } else {
-    download_service_->CancelDownload(current_download_guid_);
-    current_download_guid_.clear();
-  }
-  CancelFinished();
-}
-
 void PluginVmInstaller::OnDlcDownloadProgressUpdated(double progress) {
   DCHECK_EQ(installing_state_, InstallingState::kDownloadingDlc);
   if (state_ == State::kCancelling)
@@ -428,112 +501,78 @@ void PluginVmInstaller::OnDlcDownloadCompleted(
   InstallFailed(reason);
 }
 
-void PluginVmInstaller::OnDownloadStarted() {}
-
-void PluginVmInstaller::OnDownloadProgressUpdated(uint64_t bytes_downloaded,
-                                                  int64_t content_length) {
-  DCHECK_EQ(installing_state_, InstallingState::kDownloadingImage);
-  if (observer_)
-    observer_->OnDownloadProgressUpdated(bytes_downloaded, content_length);
-
-  if (content_length <= 0)
-    content_length = kDownloadSizeFallbackEstimate;
+void PluginVmInstaller::StartDispatcher() {
+  DCHECK_EQ(installing_state_, InstallingState::kDownloadingDlc);
+  UpdateInstallingState(InstallingState::kStartingDispatcher);
 
-  UpdateProgress(
-      std::min(1., static_cast<double>(bytes_downloaded) / content_length));
+  PluginVmManagerFactory::GetForProfile(profile_)->StartDispatcher(
+      base::BindOnce(&PluginVmInstaller::OnDispatcherStarted,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
-void PluginVmInstaller::OnDownloadCompleted(
-    const download::CompletionInfo& info) {
-  downloaded_image_ = info.path;
-  downloaded_image_size_ = info.bytes_downloaded;
-  current_download_guid_.clear();
-
-  if (downloaded_image_for_testing_)
-    downloaded_image_ = downloaded_image_for_testing_.value();
-
-  if (!VerifyDownload(info.hash256)) {
-    OnDownloadFailed(FailureReason::HASH_MISMATCH);
+void PluginVmInstaller::OnDispatcherStarted(bool success) {
+  if (state_ == State::kCancelling) {
+    CancelFinished();
     return;
   }
 
-  RecordPluginVmImageDownloadedSizeHistogram(info.bytes_downloaded);
-  StartImport();
-}
-
-void PluginVmInstaller::OnDownloadFailed(FailureReason reason) {
-  RemoveTemporaryImageIfExists();
-  current_download_guid_.clear();
-
-  if (using_drive_download_service_) {
-    drive_download_service_->ResetState();
-    using_drive_download_service_ = false;
+  if (!success) {
+    InstallFailed(FailureReason::DISPATCHER_NOT_AVAILABLE);
+    return;
   }
 
-  InstallFailed(reason);
+  StartDownload();
 }
 
-void PluginVmInstaller::StartImport() {
-  DCHECK_EQ(installing_state_, InstallingState::kDownloadingImage);
-  UpdateInstallingState(InstallingState::kImporting);
+void PluginVmInstaller::StartDownload() {
+  DCHECK_EQ(installing_state_, InstallingState::kStartingDispatcher);
+  UpdateInstallingState(InstallingState::kDownloadingImage);
   UpdateProgress(/*state_progress=*/0);
 
-  base::ThreadPool::PostTaskAndReply(
-      FROM_HERE, {base::TaskPriority::USER_VISIBLE, base::MayBlock()},
-      base::BindOnce(&PluginVmInstaller::DetectImageType,
-                     base::Unretained(this)),
-      base::BindOnce(&PluginVmInstaller::OnImageTypeDetected,
-                     weak_ptr_factory_.GetWeakPtr()));
-}
-
-void PluginVmInstaller::UpdateProgress(double state_progress) {
-  DCHECK_EQ(state_, State::kInstalling);
-  if (state_progress < 0 || state_progress > 1) {
-    LOG(ERROR) << "Unexpected progress value " << state_progress
-               << " in installing state "
-               << GetInstallingStateName(installing_state_);
+  GURL url = GetPluginVmImageDownloadUrl();
+  // This may have changed since running StartDlcDownload.
+  if (!url.is_valid()) {
+    InstallFailed(FailureReason::INVALID_IMAGE_URL);
     return;
   }
 
-  double start_range = 0;
-  double end_range = 0;
-  switch (installing_state_) {
-    case InstallingState::kDownloadingDlc:
-      start_range = 0;
-      end_range = 0.01;
-      break;
-    case InstallingState::kDownloadingImage:
-      start_range = 0.01;
-      end_range = 0.45;
-      break;
-    case InstallingState::kImporting:
-      start_range = 0.45;
-      end_range = 1;
-      break;
-    default:
-      // Other states take a negligible amount of time so we don't send progress
-      // updates.
-      NOTREACHED();
+  base::Optional<std::string> drive_id = GetIdFromDriveUrl(url);
+  using_drive_download_service_ = drive_id.has_value();
+
+  if (using_drive_download_service_) {
+    if (!drive_download_service_) {
+      drive_download_service_ =
+          std::make_unique<PluginVmDriveImageDownloadService>(this, profile_);
+    } else {
+      drive_download_service_->ResetState();
     }
 
-  double new_progress =
-      start_range + (end_range - start_range) * state_progress;
-  if (new_progress < progress_) {
-    LOG(ERROR) << "Progress went backwards from " << progress_ << " to "
-               << new_progress;
-    return;
+    drive_download_service_->StartDownload(drive_id.value());
+  } else {
+    download_service_->StartDownload(GetDownloadParams(url));
   }
+}
 
-  progress_ = new_progress;
-  if (observer_)
-    observer_->OnProgressUpdated(new_progress);
+void PluginVmInstaller::OnStartDownload(
+    const std::string& download_guid,
+    download::DownloadParams::StartResult start_result) {
+  if (start_result == download::DownloadParams::ACCEPTED)
+    current_download_guid_ = download_guid;
+  else
+    OnDownloadFailed(FailureReason::DOWNLOAD_FAILED_UNKNOWN);
 }
 
-void PluginVmInstaller::UpdateInstallingState(
-    InstallingState installing_state) {
-  DCHECK_NE(installing_state, InstallingState::kInactive);
-  installing_state_ = installing_state;
-  observer_->OnStateUpdated(installing_state_);
+void PluginVmInstaller::StartImport() {
+  DCHECK_EQ(installing_state_, InstallingState::kDownloadingImage);
+  UpdateInstallingState(InstallingState::kImporting);
+  UpdateProgress(/*state_progress=*/0);
+
+  base::ThreadPool::PostTaskAndReply(
+      FROM_HERE, {base::TaskPriority::USER_VISIBLE, base::MayBlock()},
+      base::BindOnce(&PluginVmInstaller::DetectImageType,
+                     base::Unretained(this)),
+      base::BindOnce(&PluginVmInstaller::OnImageTypeDetected,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 void PluginVmInstaller::DetectImageType() {
@@ -652,31 +691,6 @@ void PluginVmInstaller::OnImportDiskImage(base::Optional<ReplyType> reply) {
   //   at this stage though.
 }
 
-void PluginVmInstaller::OnDiskImageProgress(
-    const vm_tools::concierge::DiskImageStatusResponse& signal) {
-  if (signal.command_uuid() != current_import_command_uuid_)
-    return;
-
-  const uint64_t percent_completed = signal.progress();
-  const vm_tools::concierge::DiskImageStatus status = signal.status();
-
-  switch (status) {
-    case vm_tools::concierge::DiskImageStatus::DISK_STATUS_CREATED:
-      VLOG(1) << "Disk image status indicates that importing is done.";
-      RequestFinalStatus();
-      return;
-    case vm_tools::concierge::DiskImageStatus::DISK_STATUS_IN_PROGRESS:
-      UpdateProgress(percent_completed / 100.);
-      return;
-    default:
-      LOG(ERROR) << "Disk image status signal has status: " << status
-                 << " with error message: " << signal.failure_reason()
-                 << " and current progress: " << percent_completed;
-      OnImported(FailureReason::UNEXPECTED_DISK_IMAGE_STATUS);
-      return;
-  }
-}
-
 void PluginVmInstaller::RequestFinalStatus() {
   vm_tools::concierge::DiskImageStatusRequest status_request;
   status_request.set_command_uuid(current_import_command_uuid_);
@@ -733,6 +747,85 @@ void PluginVmInstaller::OnImported(
   InstallFinished();
 }
 
+void PluginVmInstaller::UpdateInstallingState(
+    InstallingState installing_state) {
+  DCHECK_NE(installing_state, InstallingState::kInactive);
+  installing_state_ = installing_state;
+  observer_->OnStateUpdated(installing_state_);
+}
+
+void PluginVmInstaller::UpdateProgress(double state_progress) {
+  DCHECK_EQ(state_, State::kInstalling);
+  if (state_progress < 0 || state_progress > 1) {
+    LOG(ERROR) << "Unexpected progress value " << state_progress
+               << " in installing state "
+               << GetInstallingStateName(installing_state_);
+    return;
+  }
+
+  double start_range = 0;
+  double end_range = 0;
+  switch (installing_state_) {
+    case InstallingState::kDownloadingDlc:
+      start_range = 0;
+      end_range = 0.01;
+      break;
+    case InstallingState::kDownloadingImage:
+      start_range = 0.01;
+      end_range = 0.45;
+      break;
+    case InstallingState::kImporting:
+      start_range = 0.45;
+      end_range = 1;
+      break;
+    default:
+      // Other states take a negligible amount of time so we don't send progress
+      // updates.
+      NOTREACHED();
+  }
+
+  double new_progress =
+      start_range + (end_range - start_range) * state_progress;
+  if (new_progress < progress_) {
+    LOG(ERROR) << "Progress went backwards from " << progress_ << " to "
+               << new_progress;
+    return;
+  }
+
+  progress_ = new_progress;
+  if (observer_)
+    observer_->OnProgressUpdated(new_progress);
+}
+
+void PluginVmInstaller::InstallFailed(FailureReason reason) {
+  state_ = State::kIdle;
+  GetWakeLock()->CancelWakeLock();
+  installing_state_ = InstallingState::kInactive;
+  base::UmaHistogramEnumeration(kFailureReasonHistogram, reason);
+  RecordPluginVmSetupResultHistogram(PluginVmSetupResult::kError);
+  if (observer_)
+    observer_->OnError(reason);
+}
+
+void PluginVmInstaller::InstallFinished() {
+  base::UmaHistogramLongTimes(kSetupTimeHistogram,
+                              base::TimeTicks::Now() - setup_start_tick_);
+  state_ = State::kIdle;
+  GetWakeLock()->CancelWakeLock();
+  installing_state_ = InstallingState::kInactive;
+}
+
+void PluginVmInstaller::CancelDownload() {
+  if (using_drive_download_service_) {
+    DCHECK(drive_download_service_);
+    drive_download_service_->CancelDownload();
+  } else {
+    download_service_->CancelDownload(current_download_guid_);
+    current_download_guid_.clear();
+  }
+  CancelFinished();
+}
+
 void PluginVmInstaller::CancelImport() {
   VLOG(1) << "Cancelling disk image import with command_uuid: "
           << current_import_command_uuid_;
@@ -770,53 +863,14 @@ void PluginVmInstaller::OnImportDiskImageCancelled(
   CancelFinished();
 }
 
-void PluginVmInstaller::SetObserver(Observer* observer) {
-  observer_ = observer;
-}
-
-void PluginVmInstaller::RemoveObserver() {
-  observer_ = nullptr;
-}
-
-void PluginVmInstaller::SetDownloadServiceForTesting(
-    download::DownloadService* download_service) {
-  download_service_ = download_service;
-}
-
-void PluginVmInstaller::SetDownloadedImageForTesting(
-    const base::FilePath& downloaded_image) {
-  downloaded_image_for_testing_ = downloaded_image;
-}
-
-std::string PluginVmInstaller::GetCurrentDownloadGuid() {
-  return current_download_guid_;
-}
-
-void PluginVmInstaller::SetDriveDownloadServiceForTesting(
-    std::unique_ptr<PluginVmDriveImageDownloadService> drive_download_service) {
-  drive_download_service_ = std::move(drive_download_service);
-}
-
-PluginVmInstaller::PluginVmInstaller(Profile* profile)
-    : profile_(profile),
-      download_service_(
-          DownloadServiceFactory::GetForKey(profile->GetProfileKey())) {}
-
-GURL PluginVmInstaller::GetPluginVmImageDownloadUrl() {
-  const base::Value* url_ptr = profile_->GetPrefs()
-                                   ->GetDictionary(prefs::kPluginVmImage)
-                                   ->FindKey("url");
-  if (!url_ptr) {
-    LOG(ERROR) << "Url to PluginVm image is not specified";
-    return GURL();
-  }
-  return GURL(url_ptr->GetString());
-}
+void PluginVmInstaller::CancelFinished() {
+  DCHECK_EQ(state_, State::kCancelling);
+  state_ = State::kIdle;
+  GetWakeLock()->CancelWakeLock();
+  installing_state_ = InstallingState::kInactive;
 
-int64_t PluginVmInstaller::RequiredFreeDiskSpace() {
-  return static_cast<int64_t>(profile_->GetPrefs()->GetInteger(
-             prefs::kPluginVmRequiredFreeDiskSpaceGB)) *
-         kBytesPerGigabyte;
+  if (observer_)
+    observer_->OnCancelFinished();
 }
 
 std::string PluginVmInstaller::GetStateName(State state) {
@@ -851,6 +905,17 @@ std::string PluginVmInstaller::GetInstallingStateName(InstallingState state) {
   }
 }
 
+GURL PluginVmInstaller::GetPluginVmImageDownloadUrl() {
+  const base::Value* url_ptr = profile_->GetPrefs()
+                                   ->GetDictionary(prefs::kPluginVmImage)
+                                   ->FindKey("url");
+  if (!url_ptr) {
+    LOG(ERROR) << "Url to PluginVm image is not specified";
+    return GURL();
+  }
+  return GURL(url_ptr->GetString());
+}
+
 download::DownloadParams PluginVmInstaller::GetDownloadParams(const GURL& url) {
   download::DownloadParams params;
 
@@ -879,43 +944,6 @@ download::DownloadParams PluginVmInstaller::GetDownloadParams(const GURL& url) {
   return params;
 }
 
-void PluginVmInstaller::OnStartDownload(
-    const std::string& download_guid,
-    download::DownloadParams::StartResult start_result) {
-  if (start_result == download::DownloadParams::ACCEPTED)
-    current_download_guid_ = download_guid;
-  else
-    OnDownloadFailed(FailureReason::DOWNLOAD_FAILED_UNKNOWN);
-}
-
-bool PluginVmInstaller::VerifyDownload(
-    const std::string& downloaded_archive_hash) {
-  if (downloaded_archive_hash.empty()) {
-    LOG(ERROR) << "No hash found for downloaded PluginVm image archive";
-    return false;
-  }
-  const base::Value* plugin_vm_image_hash_ptr =
-      profile_->GetPrefs()
-          ->GetDictionary(prefs::kPluginVmImage)
-          ->FindKey("hash");
-  if (!plugin_vm_image_hash_ptr) {
-    LOG(ERROR) << "Hash of PluginVm image is not specified";
-    return false;
-  }
-  std::string plugin_vm_image_hash = plugin_vm_image_hash_ptr->GetString();
-
-  if (!base::EqualsCaseInsensitiveASCII(plugin_vm_image_hash,
-                                        downloaded_archive_hash)) {
-    LOG(ERROR) << "Downloaded PluginVm image archive hash ("
-               << downloaded_archive_hash << ") doesn't match "
-               << "hash specified by the PluginVmImage policy ("
-               << plugin_vm_image_hash << ")";
-    return false;
-  }
-
-  return true;
-}
-
 void PluginVmInstaller::RemoveTemporaryImageIfExists() {
   if (using_drive_download_service_) {
     drive_download_service_->RemoveTemporaryArchive(
@@ -941,34 +969,6 @@ void PluginVmInstaller::OnTemporaryImageRemoved(bool success) {
   creating_new_vm_ = false;
 }
 
-void PluginVmInstaller::CancelFinished() {
-  DCHECK_EQ(state_, State::kCancelling);
-  state_ = State::kIdle;
-  GetWakeLock()->CancelWakeLock();
-  installing_state_ = InstallingState::kInactive;
-
-  if (observer_)
-    observer_->OnCancelFinished();
-}
-
-void PluginVmInstaller::InstallFailed(FailureReason reason) {
-  state_ = State::kIdle;
-  GetWakeLock()->CancelWakeLock();
-  installing_state_ = InstallingState::kInactive;
-  base::UmaHistogramEnumeration(kFailureReasonHistogram, reason);
-  RecordPluginVmSetupResultHistogram(PluginVmSetupResult::kError);
-  if (observer_)
-    observer_->OnError(reason);
-}
-
-void PluginVmInstaller::InstallFinished() {
-  base::UmaHistogramLongTimes(kSetupTimeHistogram,
-                              base::TimeTicks::Now() - setup_start_tick_);
-  state_ = State::kIdle;
-  GetWakeLock()->CancelWakeLock();
-  installing_state_ = InstallingState::kInactive;
-}
-
 device::mojom::WakeLock* PluginVmInstaller::GetWakeLock() {
   if (!wake_lock_) {
     mojo::Remote<device::mojom::WakeLockProvider> wake_lock_provider;

chrome/browser/chromeos/plugin_vm/plugin_vm_installer.h

@@ -30,14 +30,18 @@ namespace plugin_vm {
 
 class PluginVmDriveImageDownloadService;
 
-// PluginVmInstaller is responsible for installing the PluginVm image,
-// including downloading this image from url specified by the user policy,
-// and importing the downloaded image archive using concierge D-Bus services.
+// PluginVmInstaller is responsible for installing Plugin VM, including
+// downloading the image from url specified by the user policy, and importing
+// the downloaded image archive using concierge D-Bus services.
 //
-// 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
-// DownloadService class is used to make the request directly.
+// The installation flow is fairly linear. On top of cancelled and failed
+// installs, the branches are:
+// - OnListVmDisks() exits if a VM already exists (installed via vmc).
+// - StartDownload() uses a PluginVmDriveImageDownloadService for images hosted
+// on Drive, and the DownloadService for all other images.
+// - OnFDPrepared() calls concierge's CreateDiskImage() or ImportDiskImage()
+// depending on whether an .iso (new VM) or archive (prepared VM) is
+// downloaded.
 class PluginVmInstaller : public KeyedService,
                           public chromeos::ConciergeClient::DiskImageObserver {
  public:
@@ -86,7 +90,7 @@ class PluginVmInstaller : public KeyedService,
     kImporting,
   };
 
-  // Observer class for the PluginVm image related events.
+  // Observer for installation progress.
   class Observer {
    public:
     virtual ~Observer() = default;
@@ -110,22 +114,22 @@ class PluginVmInstaller : public KeyedService,
 
   explicit PluginVmInstaller(Profile* profile);
 
-  // Returns true if installer is processing a PluginVm image at the moment.
-  bool IsProcessing();
-
   // Start the installation. Progress updates will be sent to the observer.
   // Returns a FailureReason if the installation couldn't be started.
   base::Optional<FailureReason> Start();
-  // Cancel the installation.
+  // Cancel the installation, and calls OnCancelFinished() when done. Some steps
+  // cannot be directly cancelled, in which case we wait for the step to
+  // complete and then abort the installation.
+  // DLC will not be removed, but the downloaded image will be.
   void Cancel();
 
+  // Returns whether the installer is already running.
+  bool IsProcessing();
+
   void SetObserver(Observer* observer);
   void RemoveObserver();
 
-  // Called by DlcserviceClient, are not supposed to be used by other classes.
-  void OnDlcDownloadProgressUpdated(double progress);
-  void OnDlcDownloadCompleted(
-      const chromeos::DlcserviceClient::InstallResult& install_result);
+  std::string GetCurrentDownloadGuid();
 
   // Used by PluginVmImageDownloadClient and PluginVmDriveImageDownloadService,
   // other classes should not call into here.
@@ -139,10 +143,10 @@ class PluginVmInstaller : public KeyedService,
   void OnDiskImageProgress(
       const vm_tools::concierge::DiskImageStatusResponse& signal) override;
 
-  // Helper function that returns true in case downloaded PluginVm image
-  // archive passes hash verification and false otherwise.
+  // Helper function that returns whether the hash of the downloaded image
+  // matches the hash specified in policy.
   // Public for testing purposes.
-  bool VerifyDownload(const std::string& downloaded_archive_hash);
+  bool VerifyDownload(const std::string& download_hash);
 
   // Returns free disk space required to install Plugin VM in bytes.
   int64_t RequiredFreeDiskSpace();
@@ -156,44 +160,103 @@ class PluginVmInstaller : public KeyedService,
   void SetDriveDownloadServiceForTesting(
       std::unique_ptr<PluginVmDriveImageDownloadService>
           drive_download_service);
-  std::string GetCurrentDownloadGuid();
 
  private:
+  enum class State {
+    kIdle,
+    kInstalling,
+    kCancelling,
+  };
+
+  ~PluginVmInstaller() override;
+
+  // The entire installation flow!
+
   void CheckLicense();
   void OnLicenseChecked(bool license_is_valid);
+
   void CheckForExistingVm();
   void OnConciergeAvailable(bool success);
   void OnListVmDisks(
       base::Optional<vm_tools::concierge::ListVmDisksResponse> response);
+
   void CheckDiskSpace();
   void OnAvailableDiskSpace(int64_t bytes);
+
   void StartDlcDownload();
+  // Called repeatedly.
+  void OnDlcDownloadProgressUpdated(double progress);
+  void OnDlcDownloadCompleted(
+      const chromeos::DlcserviceClient::InstallResult& install_result);
+
   void StartDispatcher();
   void OnDispatcherStarted(bool success);
+
   void StartDownload();
-  void DetectImageType();
+  // This is only called in the DownloadService flow.
+  void OnStartDownload(const std::string& download_guid,
+                       download::DownloadParams::StartResult start_result);
+  // Download progress/completion happens in the public methods OnDownload*().
+
   void StartImport();
+  void DetectImageType();
+  void OnImageTypeDetected();
+  // Ran as a blocking task preparing the FD for the ImportDiskImage call.
+  base::Optional<base::ScopedFD> PrepareFD();
+  // Calls CreateDiskImage or ImportDiskImage, depending on whether we are
+  // creating a new VM from an ISO, or importing a prepared VM image.
+  void OnFDPrepared(base::Optional<base::ScopedFD> maybe_fd);
+  // Callback for the concierge CreateDiskImage/ImportDiskImage calls. The
+  // import has just started (unless that failed).
+  template <typename ReplyType>
+  void OnImportDiskImage(base::Optional<ReplyType> reply);
+  // Progress updates are sent to OnDiskImageProgress(). After we get a signal
+  // that the import is finished successfully, we make one final call to
+  // concierge's DiskImageStatus method to get a final resolution.
+  void RequestFinalStatus();
+  void OnFinalDiskImageStatus(
+      base::Optional<vm_tools::concierge::DiskImageStatusResponse> response);
+  // Finishes the processing of installation. If |failure_reason| has a value,
+  // then the import has failed, otherwise it was successful.
+  void OnImported(base::Optional<FailureReason> failure_reason);
+
+  // End of the install flow!
 
-  void UpdateProgress(double state_progress);
   void UpdateInstallingState(InstallingState installing_state);
+  // Only used on the long-running steps: kDownloadingDlc, kDownloadingImage,
+  // kImporting.
+  void UpdateProgress(double state_progress);
 
-  // Cancels the download of PluginVm image finishing the image processing.
-  // Downloaded PluginVm image archive is being deleted.
+  // One of InstallFailed() and InstallFinished() will be called at the end of
+  // each successfully started installation. These clean up state and log
+  // histograms.
+  void InstallFailed(FailureReason reason);
+  // Callers also need to call the appropriate observer functions indicating
+  // success type.
+  void InstallFinished();
+
+  // Cancels the image download. The partial download will be deleted.
   void CancelDownload();
-  // Makes a call to concierge to cancel the import.
+  // Calls concierge to cancel the import.
   void CancelImport();
-  // Reset state and call observers.
+  // Callback for the concierge CancelDiskImageOperation call.
+  void OnImportDiskImageCancelled(
+      base::Optional<vm_tools::concierge::CancelDiskImageResponse> response);
+  // Called once cancel is completed, firing the OnCancelFinished() observer
+  // event.
   void CancelFinished();
 
-  void InstallFailed(FailureReason reason);
-  // Reset state, callers also need to call the appropriate observer functions.
-  void InstallFinished();
+  // Stringify for logging purposes.
+  static std::string GetStateName(State state);
+  static std::string GetInstallingStateName(InstallingState state);
 
-  enum class State {
-    kIdle,
-    kInstalling,
-    kCancelling,
-  };
+  GURL GetPluginVmImageDownloadUrl();
+  download::DownloadParams GetDownloadParams(const GURL& url);
+
+  void RemoveTemporaryImageIfExists();
+  void OnTemporaryImageRemoved(bool success);
+
+  device::mojom::WakeLock* GetWakeLock();
 
   Profile* profile_ = nullptr;
   Observer* observer_ = nullptr;
@@ -203,7 +266,7 @@ class PluginVmInstaller : public KeyedService,
   base::TimeTicks setup_start_tick_;
   std::string current_download_guid_;
   base::FilePath downloaded_image_;
-  // Used to identify our running import with concierge:
+  // Used to identify our running import with concierge.
   std::string current_import_command_uuid_;
   // -1 when is not yet determined.
   int64_t downloaded_image_size_ = -1;
@@ -217,60 +280,7 @@ class PluginVmInstaller : public KeyedService,
   int64_t free_disk_space_for_testing_ = -1;
   base::Optional<base::FilePath> downloaded_image_for_testing_;
 
-  ~PluginVmInstaller() override;
-
-  // Get string representation of state for logging purposes.
-  static std::string GetStateName(State state);
-  static std::string GetInstallingStateName(InstallingState state);
-
-  GURL GetPluginVmImageDownloadUrl();
-  download::DownloadParams GetDownloadParams(const GURL& url);
-
-  void OnStartDownload(const std::string& download_guid,
-                       download::DownloadParams::StartResult start_result);
-
-  // Callback when image type has been detected. This will make call to
-  // concierge's ImportDiskImage.
-  void OnImageTypeDetected();
-
-  // Ran as a blocking task preparing the FD for the ImportDiskImage call.
-  base::Optional<base::ScopedFD> PrepareFD();
-
-  // Callback when the FD is prepared. Makes the call to CreateDiskImage or
-  // ImportDiskImage, depending on whether we are trying to create a new VM
-  // from an ISO, or import prepared VM image.
-  void OnFDPrepared(base::Optional<base::ScopedFD> maybeFd);
-
-  // Callback for the concierge CreateDiskImage/ImportDiskImage calls.
-  template <typename ReplyType>
-  void OnImportDiskImage(base::Optional<ReplyType> reply);
-
-  // After we get a signal that the import is finished successfully, we
-  // make one final call to concierge's DiskImageStatus method to get a
-  // final resolution.
-  void RequestFinalStatus();
-
-  // Callback for the final call to concierge's DiskImageStatus to
-  // get the final result of the disk import operation. This moves
-  // the installer to a finishing state, depending on the result of the
-  // query. Called when the signal for the command indicates that we
-  // are done with importing.
-  void OnFinalDiskImageStatus(
-      base::Optional<vm_tools::concierge::DiskImageStatusResponse> reply);
-
-  // Finishes the processing of PluginVm image. If |failure_reason| has a value,
-  // then the import has failed, otherwise it was successful.
-  void OnImported(base::Optional<FailureReason> failure_reason);
-
-  // Callback for the concierge CancelDiskImageOperation call.
-  void OnImportDiskImageCancelled(
-      base::Optional<vm_tools::concierge::CancelDiskImageResponse> reply);
-
-  void RemoveTemporaryImageIfExists();
-  void OnTemporaryImageRemoved(bool success);
-
   // Keep the system awake during installation.
-  device::mojom::WakeLock* GetWakeLock();
   mojo::Remote<device::mojom::WakeLock> wake_lock_;
 
   base::WeakPtrFactory<PluginVmInstaller> weak_ptr_factory_{this};