Settings buttons for Crostini operations

Buttons now enable/disable appropriately when crostini dialogs
(e.g. installer, upgrader, remover) are showing.

Bug: 1024693
Change-Id: Ic060aee82665ef5083f09b25e0dd4e9723d047a5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2049145Reviewed-by: Jason Lin <lxj@google.com>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#740539}

chrome/browser/chromeos/crostini/crostini_manager.cc

@@ -1818,29 +1818,37 @@ void CrostiniManager::GetContainerSshKeys(
 }
 
 void CrostiniManager::SetInstallerViewStatus(bool open) {
-  installer_dialog_showing_ = open;
-  for (auto& observer : installer_view_status_observers_) {
-    observer.OnCrostiniInstallerViewStatusChanged(open);
-  }
+  SetCrostiniDialogStatus(DialogType::INSTALLER, open);
 }
 
 bool CrostiniManager::GetInstallerViewStatus() const {
-  return installer_dialog_showing_;
+  return GetCrostiniDialogStatus(DialogType::INSTALLER);
+}
+
+bool CrostiniManager::GetCrostiniDialogStatus(DialogType dialog_type) const {
+  return open_crostini_dialogs_.count(dialog_type) == 1;
 }
 
-void CrostiniManager::AddInstallerViewStatusObserver(
-    InstallerViewStatusObserver* observer) {
-  installer_view_status_observers_.AddObserver(observer);
+void CrostiniManager::SetCrostiniDialogStatus(DialogType dialog_type,
+                                              bool open) {
+  if (open) {
+    open_crostini_dialogs_.insert(dialog_type);
+  } else {
+    open_crostini_dialogs_.erase(dialog_type);
+  }
+  for (auto& observer : crostini_dialog_status_observers_) {
+    observer.OnCrostiniDialogStatusChanged(dialog_type, open);
+  }
 }
 
-void CrostiniManager::RemoveInstallerViewStatusObserver(
-    InstallerViewStatusObserver* observer) {
-  installer_view_status_observers_.RemoveObserver(observer);
+void CrostiniManager::AddCrostiniDialogStatusObserver(
+    CrostiniDialogStatusObserver* observer) {
+  crostini_dialog_status_observers_.AddObserver(observer);
 }
 
-bool CrostiniManager::HasInstallerViewStatusObserver(
-    InstallerViewStatusObserver* observer) {
-  return installer_view_status_observers_.HasObserver(observer);
+void CrostiniManager::RemoveCrostiniDialogStatusObserver(
+    CrostiniDialogStatusObserver* observer) {
+  crostini_dialog_status_observers_.RemoveObserver(observer);
 }
 
 void CrostiniManager::OnDBusShuttingDownForTesting() {

chrome/browser/chromeos/crostini/crostini_manager.h

@@ -104,10 +104,12 @@ class UpgradeContainerProgressObserver {
       const std::vector<std::string>& messages) = 0;
 };
 
-class InstallerViewStatusObserver : public base::CheckedObserver {
+class CrostiniDialogStatusObserver : public base::CheckedObserver {
  public:
-  // Called when the CrostiniInstallerView is opened or closed.
-  virtual void OnCrostiniInstallerViewStatusChanged(bool open) = 0;
+  // Called when a Crostini dialog (installer, upgrader, etc.) opens or
+  // closes.
+  virtual void OnCrostiniDialogStatusChanged(DialogType dialog_type,
+                                             bool open) = 0;
 };
 
 class VmShutdownObserver : public base::CheckedObserver {
@@ -579,9 +581,12 @@ class CrostiniManager : public KeyedService,
 
   void SetInstallerViewStatus(bool open);
   bool GetInstallerViewStatus() const;
-  void AddInstallerViewStatusObserver(InstallerViewStatusObserver* observer);
-  void RemoveInstallerViewStatusObserver(InstallerViewStatusObserver* observer);
-  bool HasInstallerViewStatusObserver(InstallerViewStatusObserver* observer);
+
+  void SetCrostiniDialogStatus(DialogType dialog_type, bool open);
+  bool GetCrostiniDialogStatus(DialogType dialog_type) const;
+  void AddCrostiniDialogStatusObserver(CrostiniDialogStatusObserver* observer);
+  void RemoveCrostiniDialogStatusObserver(
+      CrostiniDialogStatusObserver* observer);
 
   void OnDBusShuttingDownForTesting();
 
@@ -860,12 +865,12 @@ class CrostiniManager : public KeyedService,
   std::map<CrostiniManager::RestartId, std::unique_ptr<CrostiniRestarter>>
       restarters_by_id_;
 
-  // True when the installer dialog is showing. At that point, it is invalid
-  // to allow Crostini uninstallation.
-  bool installer_dialog_showing_ = false;
-
-  base::ObserverList<InstallerViewStatusObserver>
-      installer_view_status_observers_;
+  base::ObserverList<CrostiniDialogStatusObserver>
+      crostini_dialog_status_observers_;
+  // Contains the types of crostini dialogs currently open. It is generally
+  // invalid to show more than one. e.g. uninstalling and installing are
+  // mutually exclusive.
+  std::set<DialogType> open_crostini_dialogs_;
 
   bool dbus_observers_removed_ = false;
 

chrome/browser/chromeos/crostini/crostini_simple_types.h

@@ -196,6 +196,13 @@ enum class CorruptionStates {
   kMaxValue = OTHER_CORRUPTION,
 };
 
+// Dialog types used by CrostiniDialogStatusObserver.
+enum class DialogType {
+  INSTALLER,
+  UPGRADER,
+  REMOVER,
+};
+
 }  // namespace crostini
 
 enum class ContainerOsVersion {

chrome/browser/resources/settings/chromeos/crostini_page/crostini_browser_proxy.js

@@ -85,6 +85,12 @@ cr.define('settings', function() {
 
     /** Show the container upgrade UI. */
     requestCrostiniContainerUpgradeView() {}
+
+    /**
+     * Request chrome send a crostini-upgrader-status-changed event with the
+     * current upgrader dialog status
+     */
+    requestCrostiniUpgraderDialogStatus() {}
   }
 
   /** @implements {settings.CrostiniBrowserProxy} */
@@ -158,6 +164,11 @@ cr.define('settings', function() {
     requestCrostiniContainerUpgradeView() {
       chrome.send('requestCrostiniContainerUpgradeView');
     }
+
+    /** @override */
+    requestCrostiniUpgraderDialogStatus() {
+      chrome.send('requestCrostiniUpgraderDialogStatus');
+    }
   }
 
   // The singleton instance_ can be replaced with a test version of this wrapper

chrome/browser/resources/settings/chromeos/crostini_page/crostini_subpage.html

@@ -27,7 +27,8 @@
           </div>
         </div>
         <cr-button on-click="onContainerUpgradeClick_"
-            aria-labelledby="upgradeCrostiniLabel">
+            aria-labelledby="upgradeCrostiniLabel" 
+            disabled="[[disableUpgradeButton_]]">
           $i18n{crostiniContainerUpgradeButton}
         </cr-button>
       </div>

chrome/browser/resources/settings/chromeos/crostini_page/crostini_subpage.js

@@ -64,6 +64,7 @@ Polymer({
      */
     hideCrostiniUninstall_: {
       type: Boolean,
+      computed: 'or_(installerShowing_, upgraderDialogShowing_)',
     },
 
     /**
@@ -88,6 +89,32 @@ Polymer({
         return loadTimeData.getBoolean('showCrostiniDiskResize');
       },
     },
+
+    /*
+     * Whether the installer is showing.
+     * @private {boolean}
+     */
+    installerShowing_: {
+      type: Boolean,
+    },
+
+    /**
+     * Whether the upgrader dialog is showing.
+     * @private {boolean}
+     */
+    upgraderDialogShowing_: {
+      type: Boolean,
+    },
+
+    /**
+     * Whether the button to launch the Crostini container upgrade flow should
+     * be disabled.
+     * @private {boolean}
+     */
+    disableUpgradeButton_: {
+      type: Boolean,
+      computed: 'or_(installerShowing_, upgraderDialogShowing_)',
+    }
   },
 
   /** settings.RouteOriginBehavior override */
@@ -99,12 +126,16 @@ Polymer({
   ],
 
   attached() {
-    const callback = (status) => {
-      this.hideCrostiniUninstall_ = status;
-    };
-    this.addWebUIListener('crostini-installer-status-changed', callback);
+    this.addWebUIListener('crostini-installer-status-changed', (status) => {
+      this.installerShowing_ = status;
+    });
+    this.addWebUIListener('crostini-upgrader-status-changed', (status) => {
+      this.upgraderDialogShowing_ = status;
+    });
     settings.CrostiniBrowserProxyImpl.getInstance()
         .requestCrostiniInstallerStatus();
+    settings.CrostiniBrowserProxyImpl.getInstance()
+        .requestCrostiniUpgraderDialogStatus();
   },
 
   ready() {
@@ -186,8 +217,23 @@ Polymer({
         settings.routes.CROSTINI_PORT_FORWARDING);
   },
 
-  /** @private */
-  and_(a, b) {
+  /**
+   * @private
+   * @param {boolean} a
+   * @param {boolean} b
+   * @return {boolean}
+   */
+  and_: function(a, b) {
     return a && b;
   },
+
+  /**
+   * @private
+   * @param {boolean} a
+   * @param {boolean} b
+   * @return {boolean}
+   */
+  or_: function(a, b) {
+    return a || b;
+  },
 });

chrome/browser/ui/webui/chromeos/crostini_upgrader/crostini_upgrader_dialog.cc

@@ -63,8 +63,12 @@ bool CrostiniUpgraderDialog::CanCloseDialog() const {
 void CrostiniUpgraderDialog::OnDialogShown(content::WebUI* webui) {
   upgrader_ui_ = static_cast<CrostiniUpgraderUI*>(webui->GetController());
   upgrader_ui_->set_launch_closure(std::move(launch_closure_));
-  crostini::CrostiniManager::GetForProfile(Profile::FromWebUI(webui))
-      ->UpgradePromptShown(
+
+  auto* crostini_manager =
+      crostini::CrostiniManager::GetForProfile(Profile::FromWebUI(webui));
+  crostini_manager->SetCrostiniDialogStatus(crostini::DialogType::UPGRADER,
+                                            true);
+  crostini_manager->UpgradePromptShown(
       crostini::ContainerId(crostini::kCrostiniDefaultVmName,
                             crostini::kCrostiniDefaultContainerName));
   return SystemWebDialogDelegate::OnDialogShown(webui);
@@ -73,6 +77,10 @@ void CrostiniUpgraderDialog::OnDialogShown(content::WebUI* webui) {
 void CrostiniUpgraderDialog::OnCloseContents(content::WebContents* source,
                                              bool* out_close_dialog) {
   upgrader_ui_ = nullptr;
+  auto* crostini_manager = crostini::CrostiniManager::GetForProfile(
+      Profile::FromBrowserContext(source->GetBrowserContext()));
+  crostini_manager->SetCrostiniDialogStatus(crostini::DialogType::UPGRADER,
+                                            false);
   return SystemWebDialogDelegate::OnCloseContents(source, out_close_dialog);
 }
 

chrome/browser/ui/webui/settings/chromeos/crostini_handler.cc

@@ -113,11 +113,16 @@ void CrostiniHandler::RegisterMessages() {
       "requestCrostiniContainerUpgradeView",
       base::BindRepeating(&CrostiniHandler::HandleRequestContainerUpgradeView,
                           weak_ptr_factory_.GetWeakPtr()));
+  web_ui()->RegisterMessageCallback(
+      "requestCrostiniUpgraderDialogStatus",
+      base::BindRepeating(
+          &CrostiniHandler::HandleCrostiniUpgraderDialogStatusRequest,
+          weak_ptr_factory_.GetWeakPtr()));
 }
 
 void CrostiniHandler::OnJavascriptAllowed() {
   crostini::CrostiniManager::GetForProfile(profile_)
-      ->AddInstallerViewStatusObserver(this);
+      ->AddCrostiniDialogStatusObserver(this);
   if (chromeos::CrosUsbDetector::Get()) {
     chromeos::CrosUsbDetector::Get()->AddUsbDeviceObserver(this);
   }
@@ -125,11 +130,8 @@ void CrostiniHandler::OnJavascriptAllowed() {
 }
 
 void CrostiniHandler::OnJavascriptDisallowed() {
-  if (crostini::CrostiniManager::GetForProfile(profile_)
-          ->HasInstallerViewStatusObserver(this)) {
   crostini::CrostiniManager::GetForProfile(profile_)
-        ->RemoveInstallerViewStatusObserver(this);
-  }
+      ->RemoveCrostiniDialogStatusObserver(this);
   if (chromeos::CrosUsbDetector::Get()) {
     chromeos::CrosUsbDetector::Get()->RemoveUsbDeviceObserver(this);
   }
@@ -277,8 +279,8 @@ void CrostiniHandler::HandleCrostiniInstallerStatusRequest(
   AllowJavascript();
   CHECK_EQ(0U, args->GetSize());
   bool status = crostini::CrostiniManager::GetForProfile(profile_)
-                    ->GetInstallerViewStatus();
-  OnCrostiniInstallerViewStatusChanged(status);
+                    ->GetCrostiniDialogStatus(crostini::DialogType::INSTALLER);
+  OnCrostiniDialogStatusChanged(crostini::DialogType::INSTALLER, status);
 }
 
 void CrostiniHandler::HandleCrostiniExportImportOperationStatusRequest(
@@ -290,12 +292,30 @@ void CrostiniHandler::HandleCrostiniExportImportOperationStatusRequest(
   OnCrostiniExportImportOperationStatusChanged(in_progress);
 }
 
-void CrostiniHandler::OnCrostiniInstallerViewStatusChanged(bool status) {
+void CrostiniHandler::OnCrostiniDialogStatusChanged(
+    crostini::DialogType dialog_type,
+    bool status) {
   // It's technically possible for this to be called before Javascript is
   // enabled, in which case we must not call FireWebUIListener
   if (IsJavascriptAllowed()) {
     // Other side listens with cr.addWebUIListener
-    FireWebUIListener("crostini-installer-status-changed", base::Value(status));
+    switch (dialog_type) {
+      case crostini::DialogType::INSTALLER:
+        FireWebUIListener("crostini-installer-status-changed",
+                          base::Value(status));
+        break;
+      case crostini::DialogType::UPGRADER:
+        FireWebUIListener("crostini-upgrader-status-changed",
+                          base::Value(status));
+        break;
+      case crostini::DialogType::REMOVER:
+        FireWebUIListener("crostini-remover-status-changed",
+                          base::Value(status));
+        break;
+      default:
+        NOTREACHED();
+        break;
+    }
   }
 }
 
@@ -398,5 +418,14 @@ void CrostiniHandler::HandleQueryArcAdbRequest(const base::ListValue* args) {
                                       weak_ptr_factory_.GetWeakPtr()));
 }
 
+void CrostiniHandler::HandleCrostiniUpgraderDialogStatusRequest(
+    const base::ListValue* args) {
+  AllowJavascript();
+  CHECK_EQ(0U, args->GetSize());
+  bool is_open = crostini::CrostiniManager::GetForProfile(profile_)
+                     ->GetCrostiniDialogStatus(crostini::DialogType::UPGRADER);
+  OnCrostiniDialogStatusChanged(crostini::DialogType::UPGRADER, is_open);
+}
+
 }  // namespace settings
 }  // namespace chromeos

chrome/browser/ui/webui/settings/chromeos/crostini_handler.h

@@ -24,7 +24,7 @@ namespace chromeos {
 namespace settings {
 
 class CrostiniHandler : public ::settings::SettingsPageUIHandler,
-                        public crostini::InstallerViewStatusObserver,
+                        public crostini::CrostiniDialogStatusObserver,
                         public crostini::CrostiniExportImport::Observer,
                         public chromeos::CrosUsbDeviceObserver {
  public:
@@ -61,8 +61,9 @@ class CrostiniHandler : public ::settings::SettingsPageUIHandler,
   void HandleImportCrostiniContainer(const base::ListValue* args);
   // Handle a request for the CrostiniInstallerView status.
   void HandleCrostiniInstallerStatusRequest(const base::ListValue* args);
-  // Handle the CrostiniInstallerView opening/closing.
-  void OnCrostiniInstallerViewStatusChanged(bool open) override;
+  // crostini::CrostiniDialogStatusObserver
+  void OnCrostiniDialogStatusChanged(crostini::DialogType dialog_type,
+                                     bool open) override;
   // Handle a request for the CrostiniExportImport operation status.
   void HandleCrostiniExportImportOperationStatusRequest(
       const base::ListValue* args);
@@ -85,6 +86,8 @@ class CrostiniHandler : public ::settings::SettingsPageUIHandler,
   // Returns whether the current user can change adb sideloading configuration
   // on current device.
   bool CheckEligibilityToChangeArcAdbSideloading() const;
+  // Handle a request for the CrostiniUpgraderDialog status.
+  void HandleCrostiniUpgraderDialogStatusRequest(const base::ListValue* args);
 
   Profile* profile_;
   // weak_ptr_factory_ should always be last member.

chrome/test/data/webui/settings/chromeos/crostini_page_test.js

@@ -157,6 +157,45 @@ suite('CrostiniPageTests', function() {
               'requestCrostiniContainerUpgradeView'));
     });
 
+    test('ContainerUpgradeButtonDisabledOnUpgradeDialog', function() {
+      const button = subpage.$$('#container-upgrade cr-button');
+      assertTrue(!!button);
+      return flushAsync()
+          .then(() => {
+            assertFalse(button.disabled);
+            cr.webUIListenerCallback('crostini-upgrader-status-changed', true);
+            return flushAsync();
+          })
+          .then(() => {
+            assertTrue(button.disabled);
+            cr.webUIListenerCallback('crostini-upgrader-status-changed', false);
+            return flushAsync();
+          })
+          .then(() => {
+            assertFalse(button.disabled);
+          });
+    });
+
+    test('ContainerUpgradeButtonDisabledOnInstall', function() {
+      const button = subpage.$$('#container-upgrade cr-button');
+      assertTrue(!!button);
+      return flushAsync()
+          .then(() => {
+            assertFalse(button.disabled);
+            cr.webUIListenerCallback('crostini-installer-status-changed', true);
+            return flushAsync();
+          })
+          .then(() => {
+            assertTrue(button.disabled);
+            cr.webUIListenerCallback(
+                'crostini-installer-status-changed', false);
+            return flushAsync();
+          })
+          .then(() => {
+            assertFalse(button.disabled);
+          });
+    });
+
     test('Export', function() {
       assertTrue(!!subpage.$$('#crostini-export-import'));
       subpage.$$('#crostini-export-import').click();

chrome/test/data/webui/settings/chromeos/test_crostini_browser_proxy.js

@@ -78,4 +78,9 @@ class TestCrostiniBrowserProxy extends TestBrowserProxy {
   requestCrostiniContainerUpgradeView() {
     this.methodCalled('requestCrostiniContainerUpgradeView');
   }
+
+  /** @override */
+  requestCrostiniUpgraderDialogStatus() {
+    cr.webUIListenerCallback('crostini-upgrader-status-changed', false);
+  }
 }