plugin_vm: pass failure reason enum to launcher view

Bug: 1017511
Change-Id: I5a7773cf33fbd69c0d400dbbf2fdf99cd4dfed48
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1904559
Commit-Queue: Julian Watson <juwa@google.com>
Reviewed-by: Timothy Loh <timloh@chromium.org>
Auto-Submit: Julian Watson <juwa@google.com>
Cr-Commit-Position: refs/heads/master@{#714343}

chrome/browser/chromeos/plugin_vm/plugin_vm_image_download_client.cc

@@ -73,11 +73,15 @@ void PluginVmImageDownloadClient::OnDownloadUpdated(const std::string& guid,
 void PluginVmImageDownloadClient::OnDownloadFailed(
     const std::string& guid,
     const download::CompletionInfo& completion_info,
-    download::Client::FailureReason reason) {
+    download::Client::FailureReason clientReason) {
   VLOG(1) << __func__ << " called";
-  switch (reason) {
+  auto managerReason =
+      PluginVmImageManager::FailureReason::DOWNLOAD_FAILED_UNKNOWN;
+  switch (clientReason) {
     case download::Client::FailureReason::NETWORK:
       VLOG(1) << "Failure reason: NETWORK";
+      managerReason =
+          PluginVmImageManager::FailureReason::DOWNLOAD_FAILED_NETWORK;
       break;
     case download::Client::FailureReason::UPLOAD_TIMEDOUT:
       VLOG(1) << "Failure reason: UPLOAD_TIMEDOUT";
@@ -90,6 +94,8 @@ void PluginVmImageDownloadClient::OnDownloadFailed(
       break;
     case download::Client::FailureReason::ABORTED:
       VLOG(1) << "Failure reason: ABORTED";
+      managerReason =
+          PluginVmImageManager::FailureReason::DOWNLOAD_FAILED_ABORTED;
       break;
     case download::Client::FailureReason::CANCELLED:
       VLOG(1) << "Failure reason: CANCELLED";
@@ -101,10 +107,10 @@ void PluginVmImageDownloadClient::OnDownloadFailed(
   if (old_downloads_.find(guid) != old_downloads_.end())
     return;
 
-  if (reason == download::Client::FailureReason::CANCELLED)
+  if (clientReason == download::Client::FailureReason::CANCELLED)
     GetManager()->OnDownloadCancelled();
   else
-    GetManager()->OnDownloadFailed();
+    GetManager()->OnDownloadFailed(managerReason);
 }
 
 void PluginVmImageDownloadClient::OnDownloadSucceeded(

chrome/browser/chromeos/plugin_vm/plugin_vm_image_manager.cc

@@ -53,7 +53,7 @@ void PluginVmImageManager::StartDownload() {
     LOG(ERROR) << "Download of a PluginVm image couldn't be started as"
                << " another PluginVm image is currently being processed "
                << "in state " << GetStateName(state_);
-    OnDownloadFailed();
+    OnDownloadFailed(FailureReason::OPERATION_IN_PROGRESS);
     return;
   }
 
@@ -63,14 +63,14 @@ void PluginVmImageManager::StartDownload() {
   if (!IsPluginVmAllowedForProfile(profile_)) {
     LOG(ERROR) << "Download of PluginVm image cannot be started because "
                << "the user is not allowed to run PluginVm";
-    OnDownloadFailed();
+    OnDownloadFailed(FailureReason::NOT_ALLOWED);
     return;
   }
 
   state_ = State::DOWNLOADING;
   GURL url = GetPluginVmImageDownloadUrl();
   if (url.is_empty()) {
-    OnDownloadFailed();
+    OnDownloadFailed(FailureReason::INVALID_IMAGE_URL);
     return;
   }
   download_service_->StartDownload(GetDownloadParams(url));
@@ -105,7 +105,7 @@ void PluginVmImageManager::OnDownloadCompleted(
   if (!VerifyDownload(info.hash256)) {
     LOG(ERROR) << "Downloaded PluginVm image archive hash doesn't match "
                << "hash specified by the PluginVmImage policy";
-    OnDownloadFailed();
+    OnDownloadFailed(FailureReason::HASH_MISMATCH);
     return;
   }
 
@@ -126,12 +126,12 @@ void PluginVmImageManager::OnDownloadCancelled() {
   state_ = State::NOT_STARTED;
 }
 
-void PluginVmImageManager::OnDownloadFailed() {
+void PluginVmImageManager::OnDownloadFailed(FailureReason reason) {
   state_ = State::DOWNLOAD_FAILED;
   RemoveTemporaryPluginVmImageArchiveIfExists();
   current_download_guid_.clear();
   if (observer_)
-    observer_->OnDownloadFailed();
+    observer_->OnDownloadFailed(reason);
 }
 
 void PluginVmImageManager::StartImport() {
@@ -139,7 +139,7 @@ void PluginVmImageManager::StartImport() {
     LOG(ERROR) << "Importing of PluginVm image couldn't proceed as current "
                << "state is " << GetStateName(state_) << " not "
                << GetStateName(State::DOWNLOADED);
-    OnImported(false);
+    OnImported(FailureReason::LOGIC_ERROR);
     return;
   }
 
@@ -157,7 +157,7 @@ void PluginVmImageManager::StartImport() {
 void PluginVmImageManager::OnPluginVmDispatcherStarted(bool success) {
   if (!success) {
     LOG(ERROR) << "Failed to start PluginVm dispatcher service";
-    OnImported(false);
+    OnImported(FailureReason::DISPATCHER_NOT_AVAILABLE);
     return;
   }
   GetConciergeClient()->WaitForServiceToBeAvailable(
@@ -168,12 +168,12 @@ void PluginVmImageManager::OnPluginVmDispatcherStarted(bool success) {
 void PluginVmImageManager::OnConciergeAvailable(bool success) {
   if (!success) {
     LOG(ERROR) << "Concierge did not become available";
-    OnImported(false);
+    OnImported(FailureReason::CONCIERGE_NOT_AVAILABLE);
     return;
   }
   if (!GetConciergeClient()->IsDiskImageProgressSignalConnected()) {
     LOG(ERROR) << "Disk image progress signal is not connected";
-    OnImported(false);
+    OnImported(FailureReason::SIGNAL_NOT_CONNECTED);
     return;
   }
   VLOG(1) << "Plugin VM dispatcher service has been started and disk image "
@@ -212,7 +212,7 @@ void PluginVmImageManager::OnFDPrepared(
 
   if (!maybeFd.has_value()) {
     LOG(ERROR) << "Could not open downloaded image archive";
-    OnImported(false);
+    OnImported(FailureReason::COULD_NOT_OPEN_IMAGE);
     return;
   }
 
@@ -237,7 +237,7 @@ void PluginVmImageManager::OnImportDiskImage(
   if (!reply.has_value()) {
     LOG(ERROR) << "Could not retrieve response from ImportDiskImage call to "
                << "concierge";
-    OnImported(false);
+    OnImported(FailureReason::INVALID_IMPORT_RESPONSE);
     return;
   }
 
@@ -251,7 +251,7 @@ void PluginVmImageManager::OnImportDiskImage(
       vm_tools::concierge::DiskImageStatus::DISK_STATUS_IN_PROGRESS) {
     LOG(ERROR) << "Disk image is not in progress. Status: " << response.status()
                << ", " << response.failure_reason();
-    OnImported(false);
+    OnImported(FailureReason::UNEXPECTED_DISK_IMAGE_STATUS);
     return;
   }
 
@@ -289,7 +289,7 @@ void PluginVmImageManager::OnDiskImageProgress(
       LOG(ERROR) << "Disk image status signal has status: " << status
                  << " with error message: " << signal.failure_reason()
                  << " and current progress: " << percent_completed;
-      OnImported(false);
+      OnImported(FailureReason::UNEXPECTED_DISK_IMAGE_STATUS);
       return;
   }
 }
@@ -308,7 +308,7 @@ void PluginVmImageManager::OnFinalDiskImageStatus(
   if (!reply.has_value()) {
     LOG(ERROR) << "Could not retrieve response from DiskImageStatus call to "
                << "concierge";
-    OnImported(false);
+    OnImported(FailureReason::INVALID_DISK_IMAGE_STATUS_RESPONSE);
     return;
   }
 
@@ -318,23 +318,25 @@ void PluginVmImageManager::OnFinalDiskImageStatus(
       vm_tools::concierge::DiskImageStatus::DISK_STATUS_CREATED) {
     LOG(ERROR) << "Disk image is not created. Status: " << response.status()
                << ", " << response.failure_reason();
-    OnImported(false);
+    OnImported(FailureReason::IMAGE_IMPORT_FAILED);
     return;
   }
 
-  OnImported(true);
+  OnImported(base::nullopt);
 }
 
-void PluginVmImageManager::OnImported(bool success) {
+void PluginVmImageManager::OnImported(
+    base::Optional<FailureReason> failure_reason) {
   GetConciergeClient()->RemoveDiskImageObserver(this);
   RemoveTemporaryPluginVmImageArchiveIfExists();
   current_import_command_uuid_.clear();
 
-  if (!success) {
+  if (failure_reason) {
     LOG(ERROR) << "Image import failed";
     state_ = State::IMPORT_FAILED;
-    if (observer_)
-      observer_->OnImportFailed();
+    if (observer_) {
+      observer_->OnImportFailed(*failure_reason);
+    }
 
     return;
   }
@@ -505,7 +507,7 @@ void PluginVmImageManager::OnStartDownload(
   if (start_result == download::DownloadParams::ACCEPTED)
     current_download_guid_ = download_guid;
   else
-    OnDownloadFailed();
+    OnDownloadFailed(FailureReason::DOWNLOAD_FAILED_UNKNOWN);
 }
 
 bool PluginVmImageManager::VerifyDownload(

chrome/browser/chromeos/plugin_vm/plugin_vm_image_manager.h

@@ -40,6 +40,27 @@ class PluginVmImageManager
     : public KeyedService,
       public chromeos::ConciergeClient::DiskImageObserver {
  public:
+  // FailureReasons values can be shown to the user. Do not reorder or renumber
+  // these values without careful consideration.
+  enum class FailureReason {
+    LOGIC_ERROR = 0,
+    SIGNAL_NOT_CONNECTED = 1,
+    OPERATION_IN_PROGRESS = 2,
+    NOT_ALLOWED = 3,
+    INVALID_IMAGE_URL = 4,
+    UNEXPECTED_DISK_IMAGE_STATUS = 5,
+    INVALID_DISK_IMAGE_STATUS_RESPONSE = 6,
+    DOWNLOAD_FAILED_UNKNOWN = 7,
+    DOWNLOAD_FAILED_NETWORK = 8,
+    DOWNLOAD_FAILED_ABORTED = 9,
+    HASH_MISMATCH = 10,
+    DISPATCHER_NOT_AVAILABLE = 11,
+    CONCIERGE_NOT_AVAILABLE = 12,
+    COULD_NOT_OPEN_IMAGE = 13,
+    INVALID_IMPORT_RESPONSE = 14,
+    IMAGE_IMPORT_FAILED = 15,
+  };
+
   // Observer class for the PluginVm image related events.
   class Observer {
    public:
@@ -50,13 +71,12 @@ class PluginVmImageManager
                                            base::TimeDelta elapsed_time) = 0;
     virtual void OnDownloadCompleted() = 0;
     virtual void OnDownloadCancelled() = 0;
-    // TODO(https://crbug.com/904851): Add failure reasons.
-    virtual void OnDownloadFailed() = 0;
+    virtual void OnDownloadFailed(FailureReason reason) = 0;
     virtual void OnImportProgressUpdated(int percent_completed,
                                          base::TimeDelta elapsed_time) = 0;
     virtual void OnImported() = 0;
     virtual void OnImportCancelled() = 0;
-    virtual void OnImportFailed() = 0;
+    virtual void OnImportFailed(FailureReason reason) = 0;
   };
 
   explicit PluginVmImageManager(Profile* profile);
@@ -86,7 +106,7 @@ class PluginVmImageManager
                                  int64_t content_length);
   void OnDownloadCompleted(const download::CompletionInfo& info);
   void OnDownloadCancelled();
-  void OnDownloadFailed();
+  void OnDownloadFailed(FailureReason reason);
 
   // ConciergeClient::DiskImageObserver:
   void OnDiskImageProgress(
@@ -171,8 +191,9 @@ class PluginVmImageManager
   void OnFinalDiskImageStatus(
       base::Optional<vm_tools::concierge::DiskImageStatusResponse> reply);
 
-  // Finishes the processing of PluginVm image.
-  void OnImported(bool success);
+  // 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(

chrome/browser/chromeos/plugin_vm/plugin_vm_image_manager_unittest.cc

@@ -62,12 +62,14 @@ class MockObserver : public PluginVmImageManager::Observer {
                     base::TimeDelta elapsed_time));
   MOCK_METHOD0(OnDownloadCompleted, void());
   MOCK_METHOD0(OnDownloadCancelled, void());
-  MOCK_METHOD0(OnDownloadFailed, void());
+  MOCK_METHOD1(OnDownloadFailed,
+               void(plugin_vm::PluginVmImageManager::FailureReason));
   MOCK_METHOD2(OnImportProgressUpdated,
                void(int percent_completed, base::TimeDelta elapsed_time));
   MOCK_METHOD0(OnImported, void());
   MOCK_METHOD0(OnImportCancelled, void());
-  MOCK_METHOD0(OnImportFailed, void());
+  MOCK_METHOD1(OnImportFailed,
+               void(plugin_vm::PluginVmImageManager::FailureReason));
 };
 
 class PluginVmImageManagerTest : public testing::Test {
@@ -252,7 +254,10 @@ TEST_F(PluginVmImageManagerTest, CanProceedWithANewImageWhenSucceededTest) {
 TEST_F(PluginVmImageManagerTest, CanProceedWithANewImageWhenFailedTest) {
   SetupConciergeForSuccessfulDiskImageImport(fake_concierge_client_);
 
-  EXPECT_CALL(*observer_, OnDownloadFailed());
+  EXPECT_CALL(
+      *observer_,
+      OnDownloadFailed(
+          PluginVmImageManager::FailureReason::DOWNLOAD_FAILED_ABORTED));
   EXPECT_CALL(*observer_, OnDownloadCompleted());
   EXPECT_CALL(*observer_, OnImportProgressUpdated(50.0, _));
   EXPECT_CALL(*observer_, OnImported());
@@ -287,7 +292,9 @@ TEST_F(PluginVmImageManagerTest, ImportNonExistingImageTest) {
   SetupConciergeForSuccessfulDiskImageImport(fake_concierge_client_);
 
   EXPECT_CALL(*observer_, OnDownloadCompleted());
-  EXPECT_CALL(*observer_, OnImportFailed());
+  EXPECT_CALL(*observer_,
+              OnImportFailed(
+                  PluginVmImageManager::FailureReason::COULD_NOT_OPEN_IMAGE));
 
   ProcessImageUntilImporting();
   // Should fail as fake downloaded file isn't set.
@@ -318,7 +325,9 @@ TEST_F(PluginVmImageManagerTest, CancelledImportTest) {
 
 TEST_F(PluginVmImageManagerTest, EmptyPluginVmImageUrlTest) {
   SetPluginVmImagePref("", kHash);
-  EXPECT_CALL(*observer_, OnDownloadFailed());
+  EXPECT_CALL(
+      *observer_,
+      OnDownloadFailed(PluginVmImageManager::FailureReason::INVALID_IMAGE_URL));
   ProcessImageUntilImporting();
 
   histogram_tester_->ExpectTotalCount(kPluginVmImageDownloadedSizeHistogram, 0);
@@ -334,7 +343,9 @@ TEST_F(PluginVmImageManagerTest, VerifyDownloadTest) {
 TEST_F(PluginVmImageManagerTest, CannotStartDownloadIfPluginVmGetsDisabled) {
   profile_->ScopedCrosSettingsTestHelper()->SetBoolean(
       chromeos::kPluginVmAllowed, false);
-  EXPECT_CALL(*observer_, OnDownloadFailed());
+  EXPECT_CALL(
+      *observer_,
+      OnDownloadFailed(PluginVmImageManager::FailureReason::NOT_ALLOWED));
   ProcessImageUntilImporting();
 }
 

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

@@ -285,7 +285,8 @@ void PluginVmLauncherView::OnDownloadCancelled() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 }
 
-void PluginVmLauncherView::OnDownloadFailed() {
+void PluginVmLauncherView::OnDownloadFailed(
+    plugin_vm::PluginVmImageManager::FailureReason reason) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   state_ = State::ERROR;
@@ -308,7 +309,8 @@ void PluginVmLauncherView::OnImportCancelled() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 }
 
-void PluginVmLauncherView::OnImportFailed() {
+void PluginVmLauncherView::OnImportFailed(
+    plugin_vm::PluginVmImageManager::FailureReason reason) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   state_ = State::ERROR;
@@ -396,6 +398,7 @@ void PluginVmLauncherView::AddedToWidget() {
 }
 
 void PluginVmLauncherView::OnStateUpdated() {
+  // TODO(https://crbug.com/1017511): display failure reasons.
   SetBigMessageLabel();
   SetMessageLabel();
   SetBigImage();

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

@@ -44,12 +44,14 @@ class PluginVmLauncherView : public views::BubbleDialogDelegateView,
                                  base::TimeDelta elapsed_time) override;
   void OnDownloadCompleted() override;
   void OnDownloadCancelled() override;
-  void OnDownloadFailed() override;
+  void OnDownloadFailed(
+      plugin_vm::PluginVmImageManager::FailureReason reason) override;
   void OnImportProgressUpdated(int percent_completed,
                                base::TimeDelta elapsed_time) override;
   void OnImported() override;
   void OnImportCancelled() override;
-  void OnImportFailed() override;
+  void OnImportFailed(
+      plugin_vm::PluginVmImageManager::FailureReason reason) override;
 
   // Public for testing purposes.
   base::string16 GetBigMessage() const;