Minor clean-ups to Plugin VM installer

- Remove the START_DOWNLOADING state in the UI code.
- Rename START_DOWNLOADING_DLC to STARTING and fix the initial state to
be that. While we logically start installation immediately upon opening
the dialog, we actually wait for AddedToWidget() to be called first, so
retaining this state allows us to avoid confusion of being in an active
state without having started the installation.
- Remove the OnDlcDownloadStarted and OnDownloadStarted callbacks, the
installer UI doesn't need these.
- Add TODO suggesting further improvements regarding cancellation.

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

chrome/browser/chromeos/plugin_vm/plugin_vm_installer.cc

@@ -100,9 +100,6 @@ void PluginVmInstaller::StartDlcDownload() {
                      weak_ptr_factory_.GetWeakPtr()),
       base::BindRepeating(&PluginVmInstaller::OnDlcDownloadProgressUpdated,
                           weak_ptr_factory_.GetWeakPtr()));
-
-  if (observer_)
-    observer_->OnDlcDownloadStarted();
 }
 
 void PluginVmInstaller::CancelDlcDownload() {
@@ -184,8 +181,6 @@ void PluginVmInstaller::OnDlcDownloadCompleted(
 
 void PluginVmInstaller::OnDownloadStarted() {
   download_start_tick_ = base::TimeTicks::Now();
-  if (observer_)
-    observer_->OnDownloadStarted();
 }
 
 void PluginVmInstaller::OnDownloadProgressUpdated(uint64_t bytes_downloaded,

chrome/browser/chromeos/plugin_vm/plugin_vm_installer.h

@@ -66,12 +66,10 @@ class PluginVmInstaller : public KeyedService,
   class Observer {
    public:
     virtual ~Observer() = default;
-    virtual void OnDlcDownloadStarted() = 0;
     virtual void OnDlcDownloadProgressUpdated(double progress,
                                               base::TimeDelta elapsed_time) = 0;
     virtual void OnDlcDownloadCompleted() = 0;
     virtual void OnDlcDownloadCancelled() = 0;
-    virtual void OnDownloadStarted() = 0;
     virtual void OnDownloadProgressUpdated(uint64_t bytes_downloaded,
                                            int64_t content_length,
                                            base::TimeDelta elapsed_time) = 0;

chrome/browser/chromeos/plugin_vm/plugin_vm_installer_unittest.cc

@@ -70,14 +70,12 @@ const int kDownloadedPluginVmImageSizeInMb = 123456789u / (1024 * 1024);
 
 class MockObserver : public PluginVmInstaller::Observer {
  public:
-  MOCK_METHOD0(OnDlcDownloadStarted, void());
   MOCK_METHOD2(OnDlcDownloadProgressUpdated,
                void(double progress, base::TimeDelta elapsed_time));
   MOCK_METHOD0(OnDlcDownloadCompleted, void());
   MOCK_METHOD0(OnDlcDownloadCancelled, void());
   MOCK_METHOD1(OnDlcDownloadFailed,
                void(plugin_vm::PluginVmInstaller::FailureReason));
-  MOCK_METHOD0(OnDownloadStarted, void());
   MOCK_METHOD3(OnDownloadProgressUpdated,
                void(uint64_t bytes_downloaded,
                     int64_t content_length,
@@ -377,7 +375,6 @@ class PluginVmInstallerDriveTest : public PluginVmInstallerTestBase {
 TEST_F(PluginVmInstallerDownloadServiceTest, DownloadPluginVmImageParamsTest) {
   SetupConciergeForSuccessfulDiskImageImport(fake_concierge_client_);
 
-  EXPECT_CALL(*observer_, OnDlcDownloadStarted());
   EXPECT_CALL(*observer_, OnDlcDownloadCompleted());
   EXPECT_CALL(*observer_, OnDownloadCompleted());
   EXPECT_CALL(*observer_, OnImportProgressUpdated(50.0, _));
@@ -400,7 +397,6 @@ TEST_F(PluginVmInstallerDownloadServiceTest, DownloadPluginVmImageParamsTest) {
 TEST_F(PluginVmInstallerDownloadServiceTest, OnlyOneImageIsProcessedTest) {
   SetupConciergeForSuccessfulDiskImageImport(fake_concierge_client_);
 
-  EXPECT_CALL(*observer_, OnDlcDownloadStarted());
   EXPECT_CALL(*observer_, OnDlcDownloadCompleted());
   EXPECT_CALL(*observer_, OnDownloadCompleted());
   EXPECT_CALL(*observer_, OnImportProgressUpdated(50.0, _));
@@ -449,7 +445,6 @@ TEST_F(PluginVmInstallerDownloadServiceTest,
   EXPECT_CALL(*observer_,
               OnDownloadFailed(
                   PluginVmInstaller::FailureReason::DOWNLOAD_FAILED_ABORTED));
-  EXPECT_CALL(*observer_, OnDlcDownloadStarted()).Times(2);
   EXPECT_CALL(*observer_, OnDlcDownloadCompleted()).Times(2);
   EXPECT_CALL(*observer_, OnDownloadCompleted());
   EXPECT_CALL(*observer_, OnImportProgressUpdated(50.0, _));
@@ -517,7 +512,6 @@ TEST_F(PluginVmInstallerDownloadServiceTest, EmptyPluginVmImageUrlTest) {
   EXPECT_CALL(
       *observer_,
       OnDownloadFailed(PluginVmInstaller::FailureReason::INVALID_IMAGE_URL));
-  EXPECT_CALL(*observer_, OnDlcDownloadStarted());
   EXPECT_CALL(*observer_, OnDlcDownloadCompleted());
   StartAndRunToCompletion();
 
@@ -544,7 +538,7 @@ TEST_F(PluginVmInstallerDownloadServiceTest, CannotStartIfPluginVmIsDisabled) {
 TEST_F(PluginVmInstallerDriveTest, InvalidDriveUrlTest) {
   SetPluginVmImagePref(kDriveUrl2, kHash);
 
-  EXPECT_CALL(*observer_, OnDownloadStarted());
+  EXPECT_CALL(*observer_, OnDlcDownloadCompleted());
   EXPECT_CALL(
       *observer_,
       OnDownloadFailed(PluginVmInstaller::FailureReason::INVALID_IMAGE_URL));
@@ -555,7 +549,7 @@ TEST_F(PluginVmInstallerDriveTest, NoConnectionDriveTest) {
   SetPluginVmImagePref(kDriveUrl, kHash);
   fake_drive_service_->set_offline(true);
 
-  EXPECT_CALL(*observer_, OnDownloadStarted());
+  EXPECT_CALL(*observer_, OnDlcDownloadCompleted());
   EXPECT_CALL(*observer_,
               OnDownloadFailed(
                   PluginVmInstaller::FailureReason::DOWNLOAD_FAILED_NETWORK));
@@ -565,7 +559,7 @@ TEST_F(PluginVmInstallerDriveTest, NoConnectionDriveTest) {
 TEST_F(PluginVmInstallerDriveTest, WrongHashDriveTest) {
   SetPluginVmImagePref(kDriveUrl, kHash2);
 
-  EXPECT_CALL(*observer_, OnDownloadStarted());
+  EXPECT_CALL(*observer_, OnDlcDownloadCompleted());
   EXPECT_CALL(*observer_, OnDownloadFailed(
                               PluginVmInstaller::FailureReason::HASH_MISMATCH));
 
@@ -576,7 +570,7 @@ TEST_F(PluginVmInstallerDriveTest, DriveDownloadFailedAfterStartingTest) {
   SetPluginVmImagePref(kDriveUrl, kHash);
   SimpleFakeDriveService* fake_drive_service = SetUpSimpleFakeDriveService();
 
-  EXPECT_CALL(*observer_, OnDownloadStarted());
+  EXPECT_CALL(*observer_, OnDlcDownloadCompleted());
   EXPECT_CALL(*observer_, OnDownloadProgressUpdated(5, 100, _));
   EXPECT_CALL(*observer_, OnDownloadProgressUpdated(10, 100, _));
   EXPECT_CALL(*observer_,
@@ -600,7 +594,7 @@ TEST_F(PluginVmInstallerDriveTest, CancelledDriveDownloadTest) {
   SetPluginVmImagePref(kDriveUrl, kHash);
   SimpleFakeDriveService* fake_drive_service = SetUpSimpleFakeDriveService();
 
-  EXPECT_CALL(*observer_, OnDownloadStarted());
+  EXPECT_CALL(*observer_, OnDlcDownloadCompleted());
   EXPECT_CALL(*observer_, OnDownloadProgressUpdated(5, 100, _));
   EXPECT_CALL(*observer_, OnDownloadCompleted()).Times(0);
 
@@ -617,7 +611,7 @@ TEST_F(PluginVmInstallerDriveTest, CancelledDriveDownloadTest) {
 TEST_F(PluginVmInstallerDriveTest, SuccessfulDriveDownloadTest) {
   SetPluginVmImagePref(kDriveUrl, kHash);
 
-  EXPECT_CALL(*observer_, OnDownloadStarted());
+  EXPECT_CALL(*observer_, OnDlcDownloadCompleted());
   EXPECT_CALL(*observer_, OnDownloadCompleted());
   EXPECT_CALL(*observer_,
               OnDownloadProgressUpdated(_, std::strlen(kContent), _))

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

@@ -193,13 +193,12 @@ bool PluginVmInstallerView::Accept() {
 
 bool PluginVmInstallerView::Cancel() {
   switch (state_) {
+    case State::STARTING:
     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);
@@ -223,13 +222,6 @@ gfx::Size PluginVmInstallerView::CalculatePreferredSize() const {
   return gfx::Size(kWindowWidth, kWindowHeight);
 }
 
-void PluginVmInstallerView::OnDlcDownloadStarted() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  state_ = State::DOWNLOADING_DLC;
-  OnStateUpdated();
-}
-
 void PluginVmInstallerView::OnDlcDownloadProgressUpdated(
     double progress,
     base::TimeDelta elapsed_time) {
@@ -243,22 +235,19 @@ void PluginVmInstallerView::OnDlcDownloadCompleted() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK_EQ(state_, State::DOWNLOADING_DLC);
 
-  state_ = State::START_DOWNLOADING;
+  state_ = State::DOWNLOADING;
   OnStateUpdated();
 }
 
+// TODO(timloh): Cancelling the installation immediately closes the dialog, but
+// getting back to a clean state could take several seconds. If a user then
+// re-opens the dialog, it could cause it to fail unexpectedly. We should make
+// use of these callback to avoid this (and possibly merge them into a single
+// callback).
 void PluginVmInstallerView::OnDlcDownloadCancelled() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 }
 
-void PluginVmInstallerView::OnDownloadStarted() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  DCHECK_EQ(state_, State::START_DOWNLOADING);
-
-  state_ = State::DOWNLOADING;
-  OnStateUpdated();
-}
-
 void PluginVmInstallerView::OnDownloadProgressUpdated(
     uint64_t bytes_downloaded,
     int64_t content_length,
@@ -337,9 +326,8 @@ void PluginVmInstallerView::OnImported() {
 
 base::string16 PluginVmInstallerView::GetBigMessage() const {
   switch (state_) {
-    case State::START_DLC_DOWNLOADING:
+    case State::STARTING:
     case State::DOWNLOADING_DLC:
-    case State::START_DOWNLOADING:
     case State::DOWNLOADING:
     case State::IMPORTING:
       return l10n_util::GetStringUTF16(
@@ -360,9 +348,8 @@ base::string16 PluginVmInstallerView::GetBigMessage() const {
 
 base::string16 PluginVmInstallerView::GetMessage() const {
   switch (state_) {
-    case State::START_DLC_DOWNLOADING:
+    case State::STARTING:
     case State::DOWNLOADING_DLC:
-    case State::START_DOWNLOADING:
       return l10n_util::GetStringUTF16(
           IDS_PLUGIN_VM_INSTALLER_START_DOWNLOADING_MESSAGE);
     case State::DOWNLOADING:
@@ -428,9 +415,8 @@ PluginVmInstallerView::~PluginVmInstallerView() {
 
 int PluginVmInstallerView::GetCurrentDialogButtons() const {
   switch (state_) {
-    case State::START_DLC_DOWNLOADING:
+    case State::STARTING:
     case State::DOWNLOADING_DLC:
-    case State::START_DOWNLOADING:
     case State::DOWNLOADING:
     case State::IMPORTING:
       return ui::DIALOG_BUTTON_CANCEL;
@@ -450,9 +436,8 @@ int PluginVmInstallerView::GetCurrentDialogButtons() const {
 base::string16 PluginVmInstallerView::GetCurrentDialogButtonLabel(
     ui::DialogButton button) const {
   switch (state_) {
-    case State::START_DLC_DOWNLOADING:
+    case State::STARTING:
     case State::DOWNLOADING_DLC:
-    case State::START_DOWNLOADING:
     case State::DOWNLOADING:
     case State::IMPORTING: {
       DCHECK_EQ(button, ui::DIALOG_BUTTON_CANCEL);
@@ -489,7 +474,7 @@ void PluginVmInstallerView::AddedToWidget() {
         plugin_vm::PluginVmSetupResult::kPluginVmIsNotAllowed);
   }
 
-  if (state_ == State::START_DOWNLOADING)
+  if (state_ == State::STARTING)
     StartInstallation();
   else
     OnStateUpdated();
@@ -514,8 +499,7 @@ void PluginVmInstallerView::OnStateUpdated() {
   }
 
   const bool progress_bar_visible =
-      state_ == State::START_DLC_DOWNLOADING ||
-      state_ == State::DOWNLOADING_DLC || state_ == State::START_DOWNLOADING ||
+      state_ == State::STARTING || state_ == State::DOWNLOADING_DLC ||
       state_ == State::DOWNLOADING || state_ == State::IMPORTING;
   progress_bar_->SetVisible(progress_bar_visible);
   // Values outside the range [0,1] display an infinite loading animation.
@@ -624,7 +608,7 @@ void PluginVmInstallerView::StartInstallation() {
   // retry button is clicked).
   setup_start_tick_ = base::TimeTicks::Now();
 
-  state_ = State::START_DLC_DOWNLOADING;
+  state_ = State::DOWNLOADING_DLC;
   OnStateUpdated();
 
   plugin_vm_installer_->SetObserver(this);

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

@@ -33,12 +33,10 @@ class PluginVmInstallerView : public views::BubbleDialogDelegateView,
   gfx::Size CalculatePreferredSize() const override;
 
   // plugin_vm::PluginVmImageDownload::Observer implementation.
-  void OnDlcDownloadStarted() override;
   void OnDlcDownloadProgressUpdated(double progress,
                                     base::TimeDelta elapsed_time) override;
   void OnDlcDownloadCompleted() override;
   void OnDlcDownloadCancelled() override;
-  void OnDownloadStarted() override;
   void OnDownloadProgressUpdated(uint64_t bytes_downloaded,
                                  int64_t content_length,
                                  base::TimeDelta elapsed_time) override;
@@ -62,15 +60,12 @@ 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.
-    DOWNLOADING,        // PluginVm image downloading is in progress.
-    IMPORTING,          // Downloaded PluginVm image importing is in progress.
-    FINISHED,           // PluginVm environment setting has been finished.
-    ERROR,              // Something unexpected happened.
+    STARTING,         // View was just created, installation hasn't yet started
+    DOWNLOADING_DLC,  // PluginVm DLC downloading and installing in progress.
+    DOWNLOADING,      // PluginVm image downloading is in progress.
+    IMPORTING,        // Downloaded PluginVm image importing is in progress.
+    FINISHED,         // PluginVm environment setting has been finished.
+    ERROR,            // Something unexpected happened.
   };
 
   ~PluginVmInstallerView() override;
@@ -104,7 +99,7 @@ class PluginVmInstallerView : public views::BubbleDialogDelegateView,
   views::ImageView* big_image_ = nullptr;
   base::TimeTicks setup_start_tick_;
 
-  State state_ = State::START_DOWNLOADING;
+  State state_ = State::STARTING;
   base::Optional<plugin_vm::PluginVmInstaller::FailureReason> reason_;
 
   base::OnceCallback<void(bool success)> finished_callback_for_testing_;