Simplify cancelling of Plugin VM downloads

This CL simplifies the code for downloading Plugin VM images:
- Remove PluginVmInstaller::OnDownloadCancelled(). It is not necessary
for us to wait for the download to cancel. If the download service takes
a while to (or fails to) cancel, we will still be able to retry the
installation.
- Instead of tracking pre-existing downloads, compare download guid to
the current download guid.
- Remove excess VLOGs

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

chrome/browser/chromeos/plugin_vm/plugin_vm_image_download_client.cc

@@ -25,33 +25,32 @@ PluginVmInstaller* PluginVmImageDownloadClient::GetInstaller() {
   return PluginVmInstallerFactory::GetForProfile(profile_);
 }
 
-// TODO(okalitova): Remove logs.
+bool PluginVmImageDownloadClient::IsCurrentDownload(const std::string& guid) {
+  return guid == GetInstaller()->GetCurrentDownloadGuid();
+}
 
 void PluginVmImageDownloadClient::OnServiceInitialized(
     bool state_lost,
     const std::vector<download::DownloadMetaData>& downloads) {
-  VLOG(1) << __func__ << " called";
-  // TODO(okalitova): Manage downloads after sleep and log out.
+  // TODO(timloh): It appears that only completed downloads (aka previous
+  // successful installations) surface here, so this logic might not be needed.
   for (const auto& download : downloads) {
     VLOG(1) << "Download tracked by DownloadService: " << download.guid;
-    old_downloads_.insert(download.guid);
     DownloadServiceFactory::GetForKey(profile_->GetProfileKey())
         ->CancelDownload(download.guid);
   }
 }
 
 void PluginVmImageDownloadClient::OnServiceUnavailable() {
-  VLOG(1) << __func__ << " called";
 }
 
 void PluginVmImageDownloadClient::OnDownloadStarted(
     const std::string& guid,
     const std::vector<GURL>& url_chain,
     const scoped_refptr<const net::HttpResponseHeaders>& headers) {
-  VLOG(1) << __func__ << " called";
   // We do not want downloads that are tracked by download service from its
   // initialization to proceed.
-  if (old_downloads_.find(guid) != old_downloads_.end()) {
+  if (!IsCurrentDownload(guid)) {
     DownloadServiceFactory::GetForKey(profile_->GetProfileKey())
         ->CancelDownload(guid);
     return;
@@ -64,8 +63,7 @@ void PluginVmImageDownloadClient::OnDownloadStarted(
 void PluginVmImageDownloadClient::OnDownloadUpdated(const std::string& guid,
                                                     uint64_t bytes_uploaded,
                                                     uint64_t bytes_downloaded) {
-  DCHECK(old_downloads_.find(guid) == old_downloads_.end());
-  VLOG(1) << __func__ << " called";
+  DCHECK(IsCurrentDownload(guid));
   VLOG(1) << bytes_downloaded << " bytes downloaded";
   GetInstaller()->OnDownloadProgressUpdated(bytes_downloaded, content_length_);
 }
@@ -74,7 +72,6 @@ void PluginVmImageDownloadClient::OnDownloadFailed(
     const std::string& guid,
     const download::CompletionInfo& completion_info,
     download::Client::FailureReason clientReason) {
-  VLOG(1) << __func__ << " called";
   auto failureReason =
       PluginVmInstaller::FailureReason::DOWNLOAD_FAILED_UNKNOWN;
   switch (clientReason) {
@@ -100,22 +97,16 @@ void PluginVmImageDownloadClient::OnDownloadFailed(
       break;
   }
 
-  // We do not want to notify PluginVmInstaller about the status of downloads
-  // that are tracked by download service from its initialization.
-  if (old_downloads_.find(guid) != old_downloads_.end())
+  if (!IsCurrentDownload(guid))
     return;
 
-  if (clientReason == download::Client::FailureReason::CANCELLED)
-    GetInstaller()->OnDownloadCancelled();
-  else
-    GetInstaller()->OnDownloadFailed(failureReason);
+  GetInstaller()->OnDownloadFailed(failureReason);
 }
 
 void PluginVmImageDownloadClient::OnDownloadSucceeded(
     const std::string& guid,
     const download::CompletionInfo& completion_info) {
-  DCHECK(old_downloads_.find(guid) == old_downloads_.end());
-  VLOG(1) << __func__ << " called";
+  DCHECK(IsCurrentDownload(guid));
   VLOG(1) << "Downloaded file is in " << completion_info.path.value();
   GetInstaller()->OnDownloadCompleted(completion_info);
 }
@@ -123,15 +114,12 @@ void PluginVmImageDownloadClient::OnDownloadSucceeded(
 bool PluginVmImageDownloadClient::CanServiceRemoveDownloadedFile(
     const std::string& guid,
     bool force_delete) {
-  VLOG(1) << __func__ << " called";
   return true;
 }
 
 void PluginVmImageDownloadClient::GetUploadData(
     const std::string& guid,
     download::GetUploadDataCallback callback) {
-  DCHECK(old_downloads_.find(guid) == old_downloads_.end());
-  VLOG(1) << __func__ << " called";
   base::SequencedTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(std::move(callback), nullptr));
 }

chrome/browser/chromeos/plugin_vm/plugin_vm_image_download_client.h

@@ -28,11 +28,12 @@ class PluginVmImageDownloadClient : public download::Client {
   ~PluginVmImageDownloadClient() override;
 
  private:
-  std::set<std::string> old_downloads_;
   Profile* profile_ = nullptr;
   int64_t content_length_ = -1;
 
   PluginVmInstaller* GetInstaller();
+  // Returns false for cancelled downloads.
+  bool IsCurrentDownload(const std::string& guid);
 
   // download::Client implementation.
   void OnServiceInitialized(

chrome/browser/chromeos/plugin_vm/plugin_vm_installer.cc

@@ -327,11 +327,11 @@ void PluginVmInstaller::CancelDownload() {
   if (using_drive_download_service_) {
     DCHECK(drive_download_service_);
     drive_download_service_->CancelDownload();
-    CancelFinished();
   } else {
-    // OnDownloadCancelled() is called after the download is cancelled.
     download_service_->CancelDownload(current_download_guid_);
+    current_download_guid_.clear();
   }
+  CancelFinished();
 }
 
 void PluginVmInstaller::OnDlcDownloadProgressUpdated(double progress) {
@@ -420,20 +420,6 @@ void PluginVmInstaller::OnDownloadCompleted(
   StartImport();
 }
 
-void PluginVmInstaller::OnDownloadCancelled() {
-  DCHECK_EQ(state_, State::kCancelling);
-  DCHECK_EQ(installing_state_, InstallingState::kDownloadingImage);
-
-  RemoveTemporaryImageIfExists();
-  current_download_guid_.clear();
-  if (using_drive_download_service_) {
-    drive_download_service_->ResetState();
-    using_drive_download_service_ = false;
-  }
-
-  CancelFinished();
-}
-
 void PluginVmInstaller::OnDownloadFailed(FailureReason reason) {
   RemoveTemporaryImageIfExists();
   current_download_guid_.clear();
@@ -774,7 +760,7 @@ void PluginVmInstaller::SetDownloadedImageForTesting(
   downloaded_image_for_testing_ = downloaded_image;
 }
 
-std::string PluginVmInstaller::GetCurrentDownloadGuidForTesting() {
+std::string PluginVmInstaller::GetCurrentDownloadGuid() {
   return current_download_guid_;
 }
 

chrome/browser/chromeos/plugin_vm/plugin_vm_installer.h

@@ -125,13 +125,12 @@ class PluginVmInstaller : public KeyedService,
   void OnDlcDownloadCompleted(
       const chromeos::DlcserviceClient::InstallResult& install_result);
 
-  // Called by PluginVmImageDownloadClient, are not supposed to be used by other
-  // classes.
+  // Used by PluginVmImageDownloadClient and PluginVmDriveImageDownloadService,
+  // other classes should not call into here.
   void OnDownloadStarted();
   void OnDownloadProgressUpdated(uint64_t bytes_downloaded,
                                  int64_t content_length);
   void OnDownloadCompleted(const download::CompletionInfo& info);
-  void OnDownloadCancelled();
   void OnDownloadFailed(FailureReason reason);
 
   // ConciergeClient::DiskImageObserver:
@@ -155,7 +154,7 @@ class PluginVmInstaller : public KeyedService,
   void SetDriveDownloadServiceForTesting(
       std::unique_ptr<PluginVmDriveImageDownloadService>
           drive_download_service);
-  std::string GetCurrentDownloadGuidForTesting();
+  std::string GetCurrentDownloadGuid();
 
  private:
   void CheckLicense();

chrome/browser/chromeos/plugin_vm/plugin_vm_installer_unittest.cc

@@ -471,7 +471,7 @@ TEST_F(PluginVmInstallerDownloadServiceTest, DownloadPluginVmImageParamsTest) {
 
   StartAndRunUntil(InstallingState::kDownloadingImage);
 
-  std::string guid = installer_->GetCurrentDownloadGuidForTesting();
+  std::string guid = installer_->GetCurrentDownloadGuid();
   base::Optional<download::DownloadParams> params =
       download_service_->GetDownload(guid);
   ASSERT_TRUE(params.has_value());
@@ -536,7 +536,7 @@ TEST_F(PluginVmInstallerDownloadServiceTest,
   EXPECT_CALL(*observer_, OnError(FailureReason::DOWNLOAD_FAILED_ABORTED));
 
   StartAndRunUntil(InstallingState::kDownloadingImage);
-  std::string guid = installer_->GetCurrentDownloadGuidForTesting();
+  std::string guid = installer_->GetCurrentDownloadGuid();
   download_service_->SetFailedDownload(guid, false);
   task_environment_.RunUntilIdle();
   VerifyExpectations();
@@ -563,8 +563,6 @@ TEST_F(PluginVmInstallerDownloadServiceTest, CancelledDownloadTest) {
   StartAndRunUntil(InstallingState::kDownloadingImage);
   installer_->Cancel();
   task_environment_.RunUntilIdle();
-  // Finishing image processing as it should really happen.
-  installer_->OnDownloadCancelled();
 
   histogram_tester_->ExpectTotalCount(kPluginVmImageDownloadedSizeHistogram, 0);
   histogram_tester_->ExpectTotalCount(kFailureReasonHistogram, 0);