crostini: Clean up UI for Crostini mic settings and enable flag by

default

Follow up from CL:2086359 to ensure that the Crostini mic sharing
dialog only shows when Crostini needs to be restarted. Tests were
added to check this and the flag regarding the mic sharing settings
is now enabled by default. Note that the mic is not shared by default.

Tests: Tested mannually and expanded the appropriate browser test
Bug: 1016193
Change-Id: I4b416feedddbe4e3b7f0c5fc638701f9f809ae66
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2123579
Commit-Queue: Daniel Ng <danielng@google.com>
Reviewed-by: Julian Watson <juwa@google.com>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754807}

chrome/browser/chromeos/crostini/crostini_manager.cc

@@ -1266,9 +1266,12 @@ void CrostiniManager::StartTerminaVm(std::string name,
   request.set_owner_id(owner_id_);
   if (base::FeatureList::IsEnabled(chromeos::features::kCrostiniGpuSupport))
     request.set_enable_gpu(true);
-  if (IsMicSharingEnabled(profile_)) {
+  bool is_mic_sharing_enabled = IsMicSharingEnabled(profile_);
+  if (is_mic_sharing_enabled) {
     request.set_enable_audio_capture(true);
   }
+  profile_->GetPrefs()->SetBoolean(
+      crostini::prefs::kCrostiniMicSharingAtLastLaunch, is_mic_sharing_enabled);
   const int32_t cpus = base::SysInfo::NumberOfProcessors() - num_cores_disabled;
   DCHECK_LT(0, cpus);
   request.set_cpus(cpus);

chrome/browser/chromeos/crostini/crostini_pref_names.cc

@@ -77,10 +77,15 @@ const char kCrostiniLastDiskSize[] = "crostini.last_disk_size";
 const char kCrostiniPortForwarding[] = "crostini.port_forwarding.ports";
 // A boolean preference indicating whether Crostini is able to access the mic.
 const char kCrostiniMicSharing[] = "crostini.mic_sharing";
+// A boolean preference indicating whether Crostini was given access to the mic
+// the last time it launched.
+const char kCrostiniMicSharingAtLastLaunch[] =
+    "crostini.mic_sharing_at_last_launch";
 
 void RegisterProfilePrefs(PrefRegistrySimple* registry) {
   registry->RegisterBooleanPref(kCrostiniEnabled, false);
-  registry->RegisterBooleanPref(kCrostiniMicSharing, true);
+  registry->RegisterBooleanPref(kCrostiniMicSharing, false);
+  registry->RegisterBooleanPref(kCrostiniMicSharingAtLastLaunch, false);
   registry->RegisterDictionaryPref(kCrostiniMimeTypes);
   registry->RegisterListPref(kCrostiniPortForwarding);
   registry->RegisterListPref(kCrostiniSharedUsbDevices);

chrome/browser/chromeos/crostini/crostini_pref_names.h

@@ -33,6 +33,7 @@ extern const char kCrostiniLastLaunchTimeWindowStart[];
 extern const char kCrostiniLastDiskSize[];
 extern const char kCrostiniPortForwarding[];
 extern const char kCrostiniMicSharing[];
+extern const char kCrostiniMicSharingAtLastLaunch[];
 
 void RegisterProfilePrefs(PrefRegistrySimple* registry);
 

chrome/browser/extensions/api/settings_private/prefs_util.cc

@@ -457,6 +457,8 @@ const PrefsUtil::TypedPrefMap& PrefsUtil::GetWhitelistedKeys() {
       settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)[crostini::prefs::kCrostiniMicSharing] =
       settings_api::PrefType::PREF_TYPE_BOOLEAN;
+  (*s_whitelist)[crostini::prefs::kCrostiniMicSharingAtLastLaunch] =
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)[crostini::prefs::kCrostiniSharedUsbDevices] =
       settings_api::PrefType::PREF_TYPE_LIST;
   (*s_whitelist)[crostini::prefs::kCrostiniContainers] =

chrome/browser/resources/settings/chromeos/crostini_page/BUILD.gn

@@ -94,6 +94,7 @@ js_library("crostini_subpage") {
     "..:os_route",
     "..:route_origin_behavior",
     "../..:router",
+    "../../controls:settings_toggle_button",
     "../../prefs:prefs_behavior",
   ]
 }

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

@@ -141,6 +141,15 @@ cr.define('settings', function() {
      * @return {!Promise<boolean>} Whether resizing succeeded(true) or failed.
      */
     resizeCrostiniDisk(vmName, newSizeBytes) {}
+
+    /**
+     * Checks if a proposed change to mic sharing requires Crostini to be
+     * restarted for it to take effect.
+     * @param {boolean} proposedValue Reflects what mic sharing is being set
+     *     to.
+     * @return {!Promise<boolean>} Whether Crostini requires a restart or not.
+     */
+    checkCrostiniMicSharingStatus(proposedValue) {}
   }
 
   /** @implements {settings.CrostiniBrowserProxy} */
@@ -242,6 +251,11 @@ cr.define('settings', function() {
     resizeCrostiniDisk(vmName, newSizeBytes) {
       return cr.sendWithPromise('resizeCrostiniDisk', vmName, newSizeBytes);
     }
+
+    /** @override */
+    checkCrostiniMicSharingStatus(proposedValue) {
+      return cr.sendWithPromise('checkCrostiniMicSharingStatus', proposedValue);
+    }
   }
 
   // The singleton instance_ can be replaced with a test version of this wrapper

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

@@ -254,13 +254,20 @@ Polymer({
   },
 
   /**
-   * Shows a dialog when adjusting mic settings.
-   * TODO(danielng): We should only show the dialog if termina is running and
-   * the settings change to be different to the current state of termina.
+   * If a change to the mic settings requires Crostini to be restarted, a
+   * dialog is shown.
    * @private
    */
   onMicSharingChange_: function() {
-    this.showCrostiniMicSharingDialog_ = true;
+    const proposedValue = /** @type {!SettingsToggleButtonElement} */
+        (this.$$('#crostini-mic-sharing')).checked;
+    settings.CrostiniBrowserProxyImpl.getInstance()
+        .checkCrostiniMicSharingStatus(proposedValue)
+        .then(requiresRestart => {
+          if (requiresRestart) {
+            this.showCrostiniMicSharingDialog_ = true;
+          }
+        });
   },
 
   /** @private */

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

@@ -16,6 +16,7 @@
 #include "chrome/browser/chromeos/crostini/crostini_features.h"
 #include "chrome/browser/chromeos/crostini/crostini_installer.h"
 #include "chrome/browser/chromeos/crostini/crostini_port_forwarder.h"
+#include "chrome/browser/chromeos/crostini/crostini_pref_names.h"
 #include "chrome/browser/chromeos/crostini/crostini_types.mojom.h"
 #include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/browser/chromeos/file_manager/path_util.h"
@@ -136,6 +137,10 @@ void CrostiniHandler::RegisterMessages() {
       "resizeCrostiniDisk",
       base::BindRepeating(&CrostiniHandler::HandleResizeCrostiniDisk,
                           weak_ptr_factory_.GetWeakPtr()));
+  web_ui()->RegisterMessageCallback(
+      "checkCrostiniMicSharingStatus",
+      base::BindRepeating(&CrostiniHandler::HandleCheckCrostiniMicSharingStatus,
+                          weak_ptr_factory_.GetWeakPtr()));
 }
 
 void CrostiniHandler::OnJavascriptAllowed() {
@@ -528,5 +533,19 @@ void CrostiniHandler::ResolveResizeCrostiniDiskCallback(
                             base::Value(succeeded));
 }
 
+void CrostiniHandler::HandleCheckCrostiniMicSharingStatus(
+    const base::ListValue* args) {
+  CHECK_EQ(2U, args->GetList().size());
+  std::string callback_id = args->GetList()[0].GetString();
+  bool proposed_value = args->GetList()[1].GetBool();
+  bool requiresRestart =
+      crostini::IsCrostiniRunning(profile_) &&
+      profile_->GetPrefs()->GetBoolean(
+          crostini::prefs::kCrostiniMicSharingAtLastLaunch) != proposed_value;
+
+  ResolveJavascriptCallback(base::Value(std::move(callback_id)),
+                            base::Value(requiresRestart));
+}
+
 }  // namespace settings
 }  // namespace chromeos

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

@@ -109,6 +109,8 @@ class CrostiniHandler : public ::settings::SettingsPageUIHandler,
   void HandleResizeCrostiniDisk(const base::ListValue* args);
   void ResolveResizeCrostiniDiskCallback(const std::string& callback_id,
                                          bool succeeded);
+  // Checks if a restart is required to update mic sharing settings.
+  void HandleCheckCrostiniMicSharingStatus(const base::ListValue* args);
 
   Profile* profile_;
   // weak_ptr_factory_ should always be last member.

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

@@ -269,6 +269,8 @@ suite('CrostiniPageTests', function() {
           });
 
       test('ToggleCrostiniMicSharing', async function() {
+        // Testing under the premise that Crostini is currently running and the
+        // mic is being shared with Crostini.
         assertTrue(!!subpage.$$('#crostini-mic-sharing'));
         assertFalse(!!subpage.$$('settings-crostini-mic-sharing-dialog'));
         setCrostiniPrefs(true, {micSharing: true});
@@ -279,14 +281,21 @@ suite('CrostiniPageTests', function() {
         await flushAsync();
         assertTrue(!!subpage.$$('settings-crostini-mic-sharing-dialog'));
         const dialog = subpage.$$('settings-crostini-mic-sharing-dialog');
+        const dialogClosedPromise = test_util.eventToPromise('close', dialog);
         dialog.$$('cr-dialog cr-button').click();
-        await flushAsync();
+        await Promise.all([dialogClosedPromise, flushAsync()]);
+        assertFalse(!!subpage.$$('settings-crostini-mic-sharing-dialog'));
         assertFalse(subpage.$$('#crostini-mic-sharing').checked);
         assertFalse(subpage.$$('#crostini-mic-sharing').pref.value);
 
         subpage.$$('#crostini-mic-sharing').click();
         assertTrue(subpage.$$('#crostini-mic-sharing').checked);
         assertTrue(subpage.$$('#crostini-mic-sharing').pref.value);
+        await flushAsync();
+        // Dialog should only appear when a restart is required, as the setting
+        // was initiated as true, changing the setting back to true does not
+        // require a restart.
+        assertFalse(!!subpage.$$('settings-crostini-mic-sharing-dialog'));
       });
 
       test('Remove', async function() {

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

@@ -6,20 +6,15 @@
 class TestCrostiniBrowserProxy extends TestBrowserProxy {
   constructor() {
     super([
-      'requestCrostiniInstallerView',
-      'requestRemoveCrostini',
-      'getCrostiniSharedPathsDisplayText',
-      'getCrostiniSharedUsbDevices',
-      'setCrostiniUsbDeviceShared',
-      'removeCrostiniSharedPath',
-      'exportCrostiniContainer',
-      'importCrostiniContainer',
+      'requestCrostiniInstallerView', 'requestRemoveCrostini',
+      'getCrostiniSharedPathsDisplayText', 'getCrostiniSharedUsbDevices',
+      'setCrostiniUsbDeviceShared', 'removeCrostiniSharedPath',
+      'exportCrostiniContainer', 'importCrostiniContainer',
       'requestCrostiniContainerUpgradeView',
       'requestCrostiniUpgraderDialogStatus',
-      'requestCrostiniContainerUpgradeAvailable',
-      'addCrostiniPortForward',
-      'getCrostiniDiskInfo',
-      'resizeCrostiniDisk',
+      'requestCrostiniContainerUpgradeAvailable', 'addCrostiniPortForward',
+      'getCrostiniDiskInfo', 'resizeCrostiniDisk',
+      'checkCrostiniMicSharingStatus'
     ]);
     this.sharedUsbDevices = [];
     this.removeSharedPathResult = true;
@@ -130,4 +125,10 @@ class TestCrostiniBrowserProxy extends TestBrowserProxy {
     this.methodCalled('resizeCrostiniDisk', vmName, newSizeBytes);
     return this.getNewPromiseFor('resizeCrostiniDisk');
   }
+
+  /** @override */
+  checkCrostiniMicSharingStatus(proposedValue) {
+    this.methodCalled('checkCrostiniMicSharingStatus', proposedValue);
+    return Promise.resolve(!proposedValue);
+  }
 }

chromeos/constants/chromeos_features.cc

@@ -87,7 +87,7 @@ const base::Feature kCrostiniUsername{"CrostiniUsername",
 
 // Enables the option to share the mic with Crostini or not
 const base::Feature kCrostiniShowMicSetting{"CrostiniShowMicSetting",
-                                            base::FEATURE_DISABLED_BY_DEFAULT};
+                                            base::FEATURE_ENABLED_BY_DEFAULT};
 
 // Enables or disables Crostini GPU support.
 const base::Feature kCrostiniGpuSupport{"CrostiniGpuSupport",