[CrOS MultiDevice] Update Instant Tethering item (multi-device subpage).

Before this CL, the multi-device settings displayed Instant Tethering
settings using exactly the same mechanism used by the network settings.
However, this is undesirable because (1) it does not display the
"prohibited" icon when a device administrator prohibits the feature and
(2) it does not display the correct text as a label for the item.

This CL changes the settings subpage to use the same infrastructure as
the rest of the features. As a result, this also requires that
TetherService be updated; previously, TetherService was responsible for
changing the "enabled" user pref itself, but now it needs to support that
mode as well as responding to changes of the "enabled" pref.

Bug: 896324, 884830
Change-Id: I083a8e8e07725130058db35ee540dd6ad733d421
Reviewed-on: https://chromium-review.googlesource.com/c/1297594Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Jeremy Klein <jlklein@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602199}

chrome/browser/chromeos/tether/tether_service.cc

@@ -174,10 +174,14 @@ TetherService::TetherService(
     }
   }
 
-  registrar_.Init(profile_->GetPrefs());
-  registrar_.Add(chromeos::multidevice_setup::kInstantTetheringAllowedPrefName,
-                 base::BindRepeating(&TetherService::OnPrefsChanged,
-                                     weak_ptr_factory_.GetWeakPtr()));
+  if (!base::FeatureList::IsEnabled(
+          chromeos::features::kEnableUnifiedMultiDeviceSetup)) {
+    registrar_.Init(profile_->GetPrefs());
+    registrar_.Add(
+        chromeos::multidevice_setup::kInstantTetheringAllowedPrefName,
+        base::BindRepeating(&TetherService::OnPrefsChanged,
+                            weak_ptr_factory_.GetWeakPtr()));
+  }
 
   UMA_HISTOGRAM_BOOLEAN("InstantTethering.UserPreference.OnStartup",
                         IsEnabledByPreference());
@@ -309,9 +313,14 @@ void TetherService::Shutdown() {
       multidevice_setup_client_->RemoveObserver(this);
     }
   }
+
   if (adapter_)
     adapter_->RemoveObserver(this);
-  registrar_.RemoveAll();
+
+  if (!base::FeatureList::IsEnabled(
+          chromeos::features::kEnableUnifiedMultiDeviceSetup)) {
+    registrar_.RemoveAll();
+  }
 
   // Shut down the feature. Note that this does not change Tether's technology
   // state in NetworkStateHandler because doing so could cause visual jank just
@@ -408,13 +417,12 @@ void TetherService::UpdateEnabledState() {
       profile_->GetPrefs()->SetBoolean(
           chromeos::multidevice_setup::kInstantTetheringEnabledPrefName,
           is_enabled);
+      LogUserPreferenceChanged(is_enabled);
+      UpdateTetherTechnologyState();
     }
-
-    UMA_HISTOGRAM_BOOLEAN("InstantTethering.UserPreference.OnToggle",
-                          is_enabled);
-    PA_LOG(INFO) << "Tether user preference changed. New value: " << is_enabled;
+  } else {
+    UpdateTetherTechnologyState();
   }
-  UpdateTetherTechnologyState();
 }
 
 void TetherService::OnShutdownComplete() {
@@ -446,6 +454,23 @@ void TetherService::OnReady() {
 void TetherService::OnFeatureStatesChanged(
     const chromeos::multidevice_setup::MultiDeviceSetupClient::FeatureStatesMap&
         feature_states_map) {
+  const chromeos::multidevice_setup::mojom::FeatureState new_state =
+      feature_states_map
+          .find(chromeos::multidevice_setup::mojom::Feature::kInstantTethering)
+          ->second;
+
+  // If the feature changed from enabled to disabled or vice-versa, log the
+  // associated metric.
+  if (new_state ==
+          chromeos::multidevice_setup::mojom::FeatureState::kEnabledByUser &&
+      previous_feature_state_ == TetherFeatureState::USER_PREFERENCE_DISABLED) {
+    LogUserPreferenceChanged(true /* is_now_enabled */);
+  } else if (new_state == chromeos::multidevice_setup::mojom::FeatureState::
+                              kDisabledByUser &&
+             previous_feature_state_ == TetherFeatureState::ENABLED) {
+    LogUserPreferenceChanged(false /* is_now_enabled */);
+  }
+
   if (adapter_)
     UpdateTetherTechnologyState();
   else
@@ -792,6 +817,13 @@ bool TetherService::HandleFeatureStateMetricIfUninitialized() {
   return true;
 }
 
+void TetherService::LogUserPreferenceChanged(bool is_now_enabled) {
+  UMA_HISTOGRAM_BOOLEAN("InstantTethering.UserPreference.OnToggle",
+                        is_now_enabled);
+  PA_LOG(INFO) << "Tether user preference changed. New value: "
+               << is_now_enabled;
+}
+
 void TetherService::SetTestDoubles(
     std::unique_ptr<chromeos::tether::NotificationPresenter>
         notification_presenter,

chrome/browser/chromeos/tether/tether_service.h

@@ -182,7 +182,10 @@ class TetherService
   FRIEND_TEST_ALL_PREFIXES(TetherServiceTest, TestCellularIsAvailable);
   FRIEND_TEST_ALL_PREFIXES(TetherServiceTest, TestDisabled);
   FRIEND_TEST_ALL_PREFIXES(TetherServiceTest, TestEnabled);
-  FRIEND_TEST_ALL_PREFIXES(TetherServiceTest, TestEnabled_MultiDeviceFlags);
+  FRIEND_TEST_ALL_PREFIXES(TetherServiceTest,
+                           TestUserPrefChangesViaFeatureStateChange);
+  FRIEND_TEST_ALL_PREFIXES(TetherServiceTest,
+                           TestUserPrefChangesViaTechnologyStateChange);
   FRIEND_TEST_ALL_PREFIXES(TetherServiceTest, TestBluetoothNotification);
   FRIEND_TEST_ALL_PREFIXES(TetherServiceTest, TestBluetoothNotPresent);
   FRIEND_TEST_ALL_PREFIXES(TetherServiceTest, TestMetricsFalsePositives);
@@ -260,6 +263,8 @@ class TetherService
   // are ephemeral. Returns whether a false positive case was handled.
   bool HandleFeatureStateMetricIfUninitialized();
 
+  void LogUserPreferenceChanged(bool is_now_enabled);
+
   void SetTestDoubles(std::unique_ptr<chromeos::tether::NotificationPresenter>
                           notification_presenter,
                       std::unique_ptr<base::OneShotTimer> timer);

chrome/browser/chromeos/tether/tether_service_unittest.cc

@@ -1327,7 +1327,69 @@ TEST_F(TetherServiceTest, TestEnabled) {
       chromeos::tether::TetherComponent::ShutdownReason::PREF_DISABLED);
 }
 
-TEST_F(TetherServiceTest, TestEnabled_MultiDeviceFlags) {
+TEST_F(TetherServiceTest, TestUserPrefChangesViaFeatureStateChange) {
+  // Start with the feature disabled.
+  initial_feature_state_ =
+      chromeos::multidevice_setup::mojom::FeatureState::kDisabledByUser;
+  profile_->GetPrefs()->SetBoolean(
+      chromeos::multidevice_setup::kInstantTetheringEnabledPrefName, false);
+  CreateTetherService(true /* enable_multidevice_apis */);
+
+  // Enable the feature.
+  profile_->GetPrefs()->SetBoolean(
+      chromeos::multidevice_setup::kInstantTetheringEnabledPrefName, true);
+  fake_multidevice_setup_client_->SetFeatureState(
+      chromeos::multidevice_setup::mojom::Feature::kInstantTethering,
+      chromeos::multidevice_setup::mojom::FeatureState::kEnabledByUser);
+  EXPECT_EQ(chromeos::NetworkStateHandler::TechnologyState::TECHNOLOGY_ENABLED,
+            network_state_handler()->GetTechnologyState(
+                chromeos::NetworkTypePattern::Tether()));
+  VerifyTetherActiveStatus(true /* expected_active */);
+  VerifyTetherFeatureStateRecorded(TetherService::TetherFeatureState::ENABLED,
+                                   1 /* expected_count */);
+  histogram_tester_.ExpectBucketCount(
+      "InstantTethering.UserPreference.OnToggle", true,
+      1u /* expected_count */);
+
+  // Disable the feature.
+  profile_->GetPrefs()->SetBoolean(
+      chromeos::multidevice_setup::kInstantTetheringEnabledPrefName, false);
+  fake_multidevice_setup_client_->SetFeatureState(
+      chromeos::multidevice_setup::mojom::Feature::kInstantTethering,
+      chromeos::multidevice_setup::mojom::FeatureState::kDisabledByUser);
+  EXPECT_EQ(
+      chromeos::NetworkStateHandler::TechnologyState::TECHNOLOGY_AVAILABLE,
+      network_state_handler()->GetTechnologyState(
+          chromeos::NetworkTypePattern::Tether()));
+  VerifyTetherActiveStatus(false /* expected_active */);
+  VerifyTetherFeatureStateRecorded(
+      TetherService::TetherFeatureState::USER_PREFERENCE_DISABLED,
+      2 /* expected_count */);
+  histogram_tester_.ExpectBucketCount(
+      "InstantTethering.UserPreference.OnToggle", false,
+      1u /* expected_count */);
+
+  // Enable the feature.
+  profile_->GetPrefs()->SetBoolean(
+      chromeos::multidevice_setup::kInstantTetheringEnabledPrefName, true);
+  fake_multidevice_setup_client_->SetFeatureState(
+      chromeos::multidevice_setup::mojom::Feature::kInstantTethering,
+      chromeos::multidevice_setup::mojom::FeatureState::kEnabledByUser);
+  EXPECT_EQ(chromeos::NetworkStateHandler::TechnologyState::TECHNOLOGY_ENABLED,
+            network_state_handler()->GetTechnologyState(
+                chromeos::NetworkTypePattern::Tether()));
+  VerifyTetherActiveStatus(true /* expected_active */);
+  VerifyTetherFeatureStateRecorded(TetherService::TetherFeatureState::ENABLED,
+                                   2 /* expected_count */);
+  histogram_tester_.ExpectBucketCount(
+      "InstantTethering.UserPreference.OnToggle", true,
+      2u /* expected_count */);
+
+  VerifyLastShutdownReason(
+      chromeos::tether::TetherComponent::ShutdownReason::PREF_DISABLED);
+}
+
+TEST_F(TetherServiceTest, TestUserPrefChangesViaTechnologyStateChange) {
   CreateTetherService(true /* enable_multidevice_apis */);
 
   EXPECT_EQ(chromeos::NetworkStateHandler::TechnologyState::TECHNOLOGY_ENABLED,

chrome/browser/resources/settings/multidevice_page/BUILD.gn

@@ -98,6 +98,8 @@ js_library("multidevice_subpage") {
 
 js_library("multidevice_tether_item") {
   deps = [
+    ":multidevice_feature_behavior",
+    "..:route",
     "//ui/webui/resources/cr_elements/chromeos/network:cr_onc_types",
   ]
   externs_list = [ "$externs_path/networking_private.js" ]

chrome/browser/resources/settings/multidevice_page/multidevice_feature_behavior.js

@@ -134,6 +134,8 @@ const MultiDeviceFeatureBehaviorImpl = {
     switch (feature) {
       case settings.MultiDeviceFeature.SMART_LOCK:
         return this.i18nAdvanced('multideviceSmartLockItemSummary');
+      case settings.MultiDeviceFeature.INSTANT_TETHERING:
+        return this.i18nAdvanced('multideviceInstantTetheringItemSummary');
       case settings.MultiDeviceFeature.MESSAGES:
         return this.i18nAdvanced('multideviceAndroidMessagesItemSummary');
       default:

chrome/browser/resources/settings/multidevice_page/multidevice_feature_item.html

@@ -33,8 +33,10 @@
         class="settings-box two-line"
         on-click="handleItemClick_"
         actionable$="[[hasSubpageClickHandler_(
-            subpageRoute, pageContentData, feature)]]">
-      <iron-icon icon="[[getIconName(feature)]]"></iron-icon>
+            feature, pageContentData, subpageRoute)]]">
+      <slot name="icon">
+        <iron-icon icon="[[getIconName(feature)]]"></iron-icon>
+      </slot>
       <div id="item-text-container" class="middle">
         <div id="featureName">[[getFeatureName(feature)]]</div>
         <div class="secondary"
@@ -44,7 +46,7 @@
       </div>
       <template is="dom-if"
           if="[[hasSubpageClickHandler_(
-              subpageRoute, pageContentData, feature)]]"
+              feature, pageContentData, subpageRoute)]]"
           restamp>
         <paper-icon-button-light class="subpage-arrow">
           <button aria-labelledby="featureName"

chrome/browser/resources/settings/multidevice_page/multidevice_feature_item.js

@@ -28,6 +28,13 @@ Polymer({
      * @type {settings.Route|undefined}
      */
     subpageRoute: Object,
+
+    /**
+     * URLSearchParams for subpage route. No param is provided if it is
+     * undefined.
+     * @type {URLSearchParams|undefined}
+     */
+    subpageRouteUrlSearchParams: Object,
   },
 
   /**
@@ -49,6 +56,6 @@ Polymer({
       return;
     }
 
-    settings.navigateTo(this.subpageRoute);
+    settings.navigateTo(this.subpageRoute, this.subpageRouteUrlSearchParams);
   },
 });

chrome/browser/resources/settings/multidevice_page/multidevice_subpage.html

@@ -73,7 +73,8 @@
             if="[[isFeatureSupported(
                 MultiDeviceFeature.INSTANT_TETHERING, pageContentData)]]"
             restamp>
-          <settings-multidevice-tether-item id="instantTetheringItem">
+          <settings-multidevice-tether-item id="instantTetheringItem"
+              page-content-data="[[pageContentData]]">
           </settings-multidevice-tether-item>
         </template>
         <template is="dom-if"

chrome/browser/resources/settings/multidevice_page/multidevice_subpage.js

@@ -18,7 +18,10 @@ Polymer({
   ],
 
   properties: {
-    /** @type {?SettingsRoutes} */
+    /**
+     * Alias for allowing Polymer bindings to settings.routes.
+     * @type {?SettingsRoutes}
+     */
     routes: {
       type: Object,
       value: settings.routes,
@@ -61,16 +64,6 @@ Polymer({
     this.browserProxy_.setUpAndroidSms();
   },
 
-  listeners: {
-    'show-networks': 'onShowNetworks_',
-  },
-
-  onShowNetworks_: function() {
-    settings.navigateTo(
-        settings.routes.INTERNET_NETWORKS,
-        new URLSearchParams('type=' + CrOnc.Type.TETHER));
-  },
-
   /**
    * @return {boolean}
    * @private

chrome/browser/resources/settings/multidevice_page/multidevice_tether_item.html

 <link rel="import" href="chrome://resources/html/polymer.html">
 
 <link rel="import" href="chrome://resources/cr_elements/chromeos/network/cr_onc_types.html">
-<link rel="import" href="chrome://resources/cr_elements/policy/cr_policy_network_behavior.html">
+<link rel="import" href="chrome://resources/cr_elements/chromeos/network/cr_network_icon.html">
 <link rel="import" href="../i18n_setup.html">
-<link rel="import" href="../internet_page/network_summary_item.html">
+<link rel="import" href="../route.html">
 <link rel="import" href="../settings_shared_css.html">
 <link rel="import" href="../settings_vars_css.html">
+<link rel="import" href="multidevice_feature_behavior.html">
+<link rel="import" href="multidevice_feature_item.html">
 
 <dom-module id="settings-multidevice-tether-item">
   <style include="settings-shared">
-    network-summary-item {
-      --network-summary-item-outer-box: {
-        padding: var(--feature-item-row-padding);
-      };
-
-      --network-summary-item-title: {
-        color: var(--cr-primary-text-color);
-        font-weight: 400;
-      };
-    }
   </style>
   <template>
-    <network-summary-item id="networkSummaryItem"
-        active-network-state="[[activeNetworkState_]]"
-        device-state="[[deviceState_]]"
-        network-state-list="[[getNetworkStateList_(activeNetworkState_)]]"
-        networking-private="[[networkingPrivate]]"
-        tether-device-state="[[deviceState_]]"
-        network-title-text="$i18n{multideviceInstantTetheringItemTitle}">
-    </network-summary-item>
+    <settings-multidevice-feature-item page-content-data="[[pageContentData]]"
+        feature="[[MultiDeviceFeature.INSTANT_TETHERING]]"
+        subpage-route="[[routes.INTERNET_NETWORKS]]"
+        subpage-route-url-search-params=
+            "[[getTetherNetworkUrlSearchParams_()]]">
+      <cr-network-icon slot="icon"
+          network-state="[[activeNetworkState_]]"
+          device-state="[[deviceState_]]">
+      </cr-network-icon>
+    </settings-multidevice-feature-item>
   </template>
   <script src="multidevice_tether_item.js"></script>
 </dom-module>

chrome/browser/resources/settings/multidevice_page/multidevice_tether_item.js

@@ -14,6 +14,8 @@
 Polymer({
   is: 'settings-multidevice-tether-item',
 
+  behaviors: [MultiDeviceFeatureBehavior],
+
   properties: {
     /**
      * Interface for networkingPrivate calls.
@@ -37,10 +39,18 @@ Polymer({
      * @private {?CrOnc.NetworkStateProperties|undefined}
      */
     activeNetworkState_: Object,
+
+    /**
+     * Alias for allowing Polymer bindings to settings.routes.
+     * @type {?SettingsRoutes}
+     */
+    routes: {
+      type: Object,
+      value: settings.routes,
+    },
   },
 
   listeners: {
-    'device-enabled-toggled': 'onDeviceEnabledToggled_',
     'network-list-changed': 'updateTetherNetworkState_',
     // network-changed is fired by the settings-multidevice-subpage element's
     // CrNetworkListenerBehavior.
@@ -104,18 +114,6 @@ Polymer({
     });
   },
 
-  /**
-   * Event triggered by a device state enabled toggle.
-   * @param {!{detail: {enabled: boolean, type: CrOnc.Type}}} event
-   * @private
-   */
-  onDeviceEnabledToggled_: function(event) {
-    if (event.detail.enabled)
-      this.networkingPrivate_.enableNetworkType(CrOnc.Type.TETHER);
-    else
-      this.networkingPrivate_.disableNetworkType(CrOnc.Type.TETHER);
-  },
-
   /**
    * Retrieves device states (CrOnc.DeviceStateProperties) and sets
    * this.deviceState_ to the retrieved Instant Tethering state (or undefined if
@@ -160,4 +158,12 @@ Polymer({
   getNetworkStateList_: function() {
     return this.activeNetworkState_.GUID ? [this.activeNetworkState_] : [];
   },
+
+  /**
+   * @return {!URLSearchParams}
+   * @private
+   */
+  getTetherNetworkUrlSearchParams_: function() {
+    return new URLSearchParams('type=' + CrOnc.Type.TETHER);
+  },
 });

chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc

@@ -2722,6 +2722,8 @@ void AddMultideviceStrings(content::WebUIDataSource* html_source) {
       {"multideviceSmartLockItemTitle", IDS_SETTINGS_EASY_UNLOCK_SECTION_TITLE},
       {"multideviceInstantTetheringItemTitle",
        IDS_SETTINGS_MULTIDEVICE_INSTANT_TETHERING},
+      {"multideviceInstantTetheringItemSummary",
+       IDS_SETTINGS_MULTIDEVICE_INSTANT_TETHERING_SUMMARY},
       {"multideviceAndroidMessagesItemTitle",
        IDS_SETTINGS_MULTIDEVICE_ANDROID_MESSAGES},
       {"multideviceForgetDevice", IDS_SETTINGS_MULTIDEVICE_FORGET_THIS_DEVICE},