[CrOS MultiDevice] Only start up Instant Tethering when enabled.

This CL updates TetherService and TetherHostFetcher to use the
MultiDeviceSetupClient's GetFeatureState() function to determine whether
the feature should be active or not. This fixes a few edge cases:
  (1) If the Instant Tethering user preference is enabled, but the
      Better Together suite is disabled, we should not start up the
      feature.
  (2) If the preference is enabled, but the host device does not support
      Instant Tethering, we should not start up the feature.

This CL eliminates the MULTIDEVICE_HOST_UNVERIFIED metrics enum since
an unverified host results in NO_AVAILABLE_HOSTS; it replaces that enum
value with BETTER_TOGETHER_SUITE_DISABLED, which can occur in practice.
Note that because the old metric value has not yet been pushed to users,
it is okay to replace it with a new value instead of deprecating it.

[email protected]

(cherry picked from commit 7bddbb1)

Bug: 875502, 824568
Change-Id: Ia85f0ba31871c94ed9eb0cb2ef3c4699bb0000dd
Reviewed-on: https://chromium-review.googlesource.com/1196172
Reviewed-by: Zentaro Kavanagh <[email protected]>
Commit-Queue: Kyle Horimoto <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#587894}
Reviewed-on: https://chromium-review.googlesource.com/1208139
Reviewed-by: Ryan Hansberry <[email protected]>
Cr-Commit-Position: refs/branch-heads/3538@{#68}
Cr-Branched-From: 79f7c91-refs/heads/master@{#587811}

Author: Ryan Hansberry

chrome/browser/chromeos/tether/tether_service.cc

@@ -110,8 +110,8 @@ std::string TetherService::TetherFeatureStateToString(
       return "[Wi-Fi is not present on the device]";
     case (TetherFeatureState::SUSPENDED):
       return "[Suspended]";
-    case (TetherFeatureState::MULTIDEVICE_HOST_UNVERIFIED):
-      return "[MultiDevice host unverified]";
+    case (TetherFeatureState::BETTER_TOGETHER_SUITE_DISABLED):
+      return "[Better Together suite is disabled]";
     case (TetherFeatureState::TETHER_FEATURE_STATE_MAX):
       // |previous_feature_state_| is initialized to TETHER_FEATURE_STATE_MAX,
       // and this value is never actually used in practice.
@@ -267,10 +267,17 @@ void TetherService::StopTetherIfNecessary() {
       shutdown_reason = chromeos::tether::TetherComponent::ShutdownReason::
           BLUETOOTH_CONTROLLER_DISAPPEARED;
       break;
-    case MULTIDEVICE_HOST_UNVERIFIED:
+    case NO_AVAILABLE_HOSTS:
+      // If |tether_component_| was previously active but now has been shut down
+      // due to no longer having a host, this means that the host became
+      // unverified.
       shutdown_reason = chromeos::tether::TetherComponent::ShutdownReason::
           MULTIDEVICE_HOST_UNVERIFIED;
       break;
+    case BETTER_TOGETHER_SUITE_DISABLED:
+      shutdown_reason = chromeos::tether::TetherComponent::ShutdownReason::
+          BETTER_TOGETHER_SUITE_DISABLED;
+      break;
     default:
       PA_LOG(ERROR) << "Unexpected shutdown reason. FeatureState is "
                     << GetTetherFeatureState() << ".";
@@ -422,17 +429,15 @@ void TetherService::OnReady() {
 
   if (base::FeatureList::IsEnabled(
           chromeos::features::kEnableUnifiedMultiDeviceSetup)) {
-    OnHostStatusChanged(multidevice_setup_client_->GetHostStatus());
+    OnFeatureStatesChanged(multidevice_setup_client_->GetFeatureStates());
   } else {
     GetBluetoothAdapter();
   }
 }
 
-void TetherService::OnHostStatusChanged(
-    const chromeos::multidevice_setup::MultiDeviceSetupClient::
-        HostStatusWithDevice& host_status_with_device) {
-  host_status_ = host_status_with_device.first;
-
+void TetherService::OnFeatureStatesChanged(
+    const chromeos::multidevice_setup::MultiDeviceSetupClient::FeatureStatesMap&
+        feature_states_map) {
   if (adapter_)
     UpdateTetherTechnologyState();
   else
@@ -494,7 +499,7 @@ TetherService::GetTetherTechnologyState() {
     case WIFI_NOT_PRESENT:
     case NO_AVAILABLE_HOSTS:
     case CELLULAR_DISABLED:
-    case MULTIDEVICE_HOST_UNVERIFIED:
+    case BETTER_TOGETHER_SUITE_DISABLED:
       return chromeos::NetworkStateHandler::TechnologyState::
           TECHNOLOGY_UNAVAILABLE;
 
@@ -649,23 +654,52 @@ TetherService::TetherFeatureState TetherService::GetTetherFeatureState() {
   if (IsCellularAvailableButNotEnabled())
     return CELLULAR_DISABLED;
 
-  if (!IsAllowedByPolicy())
-    return PROHIBITED;
-
   if (!IsBluetoothPowered())
     return BLUETOOTH_DISABLED;
 
-  if (!IsEnabledByPreference())
-    return USER_PREFERENCE_DISABLED;
-
+  // For the cases below, the state is computed differently depending on whether
+  // the MultiDeviceSetup service is active.
   if (base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi) &&
       base::FeatureList::IsEnabled(
-          chromeos::features::kEnableUnifiedMultiDeviceSetup) &&
-      host_status_ !=
-          chromeos::multidevice_setup::mojom::HostStatus::kHostVerified) {
-    return MULTIDEVICE_HOST_UNVERIFIED;
+          chromeos::features::kEnableUnifiedMultiDeviceSetup)) {
+    chromeos::multidevice_setup::mojom::FeatureState tether_multidevice_state =
+        multidevice_setup_client_->GetFeatureState(
+            chromeos::multidevice_setup::mojom::Feature::kInstantTethering);
+    switch (tether_multidevice_state) {
+      case chromeos::multidevice_setup::mojom::FeatureState::
+          kProhibitedByPolicy:
+        return PROHIBITED;
+      case chromeos::multidevice_setup::mojom::FeatureState::kDisabledByUser:
+        return USER_PREFERENCE_DISABLED;
+      case chromeos::multidevice_setup::mojom::FeatureState::kEnabledByUser:
+        return ENABLED;
+      case chromeos::multidevice_setup::mojom::FeatureState::
+          kUnavailableSuiteDisabled:
+        return BETTER_TOGETHER_SUITE_DISABLED;
+      case chromeos::multidevice_setup::mojom::FeatureState::
+          kNotSupportedByPhone:
+        FALLTHROUGH;
+      case chromeos::multidevice_setup::mojom::FeatureState::
+          kUnavailableNoVerifiedHost:
+        no_available_hosts_false_positive_encountered_ = true;
+        return NO_AVAILABLE_HOSTS;
+      default:
+        // Other FeatureStates:
+        //   *kNotSupportedByChromebook: Would result in TetherService not being
+        //      created at all.
+        //   *kUnavailableInsufficientSecurity: Should never occur.
+        PA_LOG(ERROR) << "Invalid MultiDevice FeatureState: "
+                      << tether_multidevice_state;
+        NOTREACHED();
+    }
   }
 
+  if (!IsAllowedByPolicy())
+    return PROHIBITED;
+
+  if (!IsEnabledByPreference())
+    return USER_PREFERENCE_DISABLED;
+
   return ENABLED;
 }
 

chrome/browser/chromeos/tether/tether_service.h

@@ -124,9 +124,9 @@ class TetherService
   void OnReady() override;
 
   // chromeos::multidevice_setup::MultiDeviceSetupClient::Observer:
-  void OnHostStatusChanged(
+  void OnFeatureStatesChanged(
       const chromeos::multidevice_setup::MultiDeviceSetupClient::
-          HostStatusWithDevice& host_status_with_device) override;
+          FeatureStatesMap& feature_states_map) override;
 
   // Callback when the controlling pref changes.
   void OnPrefsChanged();
@@ -154,6 +154,10 @@ class TetherService
       TestMultiDeviceSetupClientInitiallyHasNoVerifiedHost);
   FRIEND_TEST_ALL_PREFIXES(TetherServiceTest,
                            TestMultiDeviceSetupClientLosesVerifiedHost);
+  FRIEND_TEST_ALL_PREFIXES(TetherServiceTest,
+                           TestBetterTogetherSuiteInitiallyDisabled);
+  FRIEND_TEST_ALL_PREFIXES(TetherServiceTest,
+                           TestBetterTogetherSuiteBecomesDisabled);
   FRIEND_TEST_ALL_PREFIXES(TetherServiceTest, TestBleAdvertisingNotSupported);
   FRIEND_TEST_ALL_PREFIXES(
       TetherServiceTest,
@@ -183,8 +187,7 @@ class TetherService
   FRIEND_TEST_ALL_PREFIXES(TetherServiceTest, TestMetricsFalsePositives);
   FRIEND_TEST_ALL_PREFIXES(TetherServiceTest, TestWifiNotPresent);
 
-  // Reflects InstantTethering_TechnologyStateAndReason enum in enums.xml. Do
-  // not rearrange.
+  // Reflects InstantTethering_FeatureState enum in enums.xml. Do not rearrange.
   enum TetherFeatureState {
     // Note: Value 0 was previously OTHER_OR_UNKNOWN, but this was a vague
     // description.
@@ -201,18 +204,14 @@ class TetherService
     BLE_NOT_PRESENT = 9,
     WIFI_NOT_PRESENT = 10,
     SUSPENDED = 11,
-    MULTIDEVICE_HOST_UNVERIFIED = 12,
+    BETTER_TOGETHER_SUITE_DISABLED = 12,
     TETHER_FEATURE_STATE_MAX
   };
 
   // For debug logs.
   static std::string TetherFeatureStateToString(
       const TetherFeatureState& state);
 
-  void OnHostStatusReceived(
-      chromeos::multidevice_setup::mojom::HostStatus host_status,
-      const base::Optional<cryptauth::RemoteDeviceRef>& host_device);
-
   void GetBluetoothAdapter();
   void OnBluetoothAdapterFetched(
       scoped_refptr<device::BluetoothAdapter> adapter);

chrome/browser/chromeos/tether/tether_service_unittest.cc

@@ -368,9 +368,8 @@ class TetherServiceTest : public chromeos::NetworkStateTest {
     chromeos::multidevice_setup::MultiDeviceSetupClientImpl::Factory::
         SetInstanceForTesting(
             fake_multidevice_setup_client_impl_factory_.get());
-    initial_host_status_ =
-        chromeos::multidevice_setup::mojom::HostStatus::kHostVerified;
-    initial_host_device_ = test_devices_[0];
+    initial_feature_state_ =
+        chromeos::multidevice_setup::mojom::FeatureState::kEnabledByUser;
 
     fake_cryptauth_service_ =
         std::make_unique<cryptauth::FakeCryptAuthService>();
@@ -453,8 +452,9 @@ class TetherServiceTest : public chromeos::NetworkStateTest {
   }
 
   void CreateTetherService() {
-    fake_multidevice_setup_client_->SetHostStatusWithDevice(
-        std::make_pair(initial_host_status_, initial_host_device_));
+    fake_multidevice_setup_client_->SetFeatureState(
+        chromeos::multidevice_setup::mojom::Feature::kInstantTethering,
+        initial_feature_state_);
 
     tether_service_ = base::WrapUnique(new TestTetherService(
         profile_.get(), fake_power_manager_client_.get(),
@@ -589,8 +589,7 @@ class TetherServiceTest : public chromeos::NetworkStateTest {
   std::unique_ptr<cryptauth::FakeCryptAuthEnrollmentManager>
       fake_enrollment_manager_;
 
-  chromeos::multidevice_setup::mojom::HostStatus initial_host_status_;
-  base::Optional<cryptauth::RemoteDeviceRef> initial_host_device_;
+  chromeos::multidevice_setup::mojom::FeatureState initial_feature_state_;
 
   scoped_refptr<MockExtendedBluetoothAdapter> mock_adapter_;
   bool is_adapter_present_;
@@ -723,9 +722,8 @@ TEST_F(TetherServiceTest,
            kEnableUnifiedMultiDeviceSetup} /* enabled_features */,
       {} /* disabled_features */);
 
-  initial_host_status_ =
-      chromeos::multidevice_setup::mojom::HostStatus::kNoEligibleHosts;
-  initial_host_device_ = base::nullopt;
+  initial_feature_state_ = chromeos::multidevice_setup::mojom::FeatureState::
+      kUnavailableNoVerifiedHost;
 
   CreateTetherService();
 
@@ -735,10 +733,9 @@ TEST_F(TetherServiceTest,
           chromeos::NetworkTypePattern::Tether()));
   VerifyTetherActiveStatus(false /* expected_active */);
 
-  fake_multidevice_setup_client_->SetHostStatusWithDevice(std::make_pair(
-      chromeos::multidevice_setup::mojom::HostStatus::kHostVerified,
-      test_devices_[0]));
-  base::RunLoop().RunUntilIdle();
+  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(
@@ -765,9 +762,10 @@ TEST_F(TetherServiceTest, TestMultiDeviceSetupClientLosesVerifiedHost) {
                 chromeos::NetworkTypePattern::Tether()));
   VerifyTetherActiveStatus(true /* expected_active */);
 
-  fake_multidevice_setup_client_->SetHostStatusWithDevice(std::make_pair(
-      chromeos::multidevice_setup::mojom::HostStatus::kNoEligibleHosts,
-      base::nullopt));
+  fake_multidevice_setup_client_->SetFeatureState(
+      chromeos::multidevice_setup::mojom::Feature::kInstantTethering,
+      chromeos::multidevice_setup::mojom::FeatureState::
+          kUnavailableNoVerifiedHost);
 
   EXPECT_EQ(
       chromeos::NetworkStateHandler::TechnologyState::TECHNOLOGY_UNAVAILABLE,
@@ -777,12 +775,79 @@ TEST_F(TetherServiceTest, TestMultiDeviceSetupClientLosesVerifiedHost) {
 
   ShutdownTetherService();
   VerifyTetherFeatureStateRecorded(
-      TetherService::TetherFeatureState::MULTIDEVICE_HOST_UNVERIFIED,
+      TetherService::TetherFeatureState::NO_AVAILABLE_HOSTS,
       1 /* expected_count */);
   VerifyLastShutdownReason(chromeos::tether::TetherComponent::ShutdownReason::
                                MULTIDEVICE_HOST_UNVERIFIED);
 }
 
+TEST_F(TetherServiceTest, TestBetterTogetherSuiteInitiallyDisabled) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitWithFeatures(
+      {chromeos::features::kMultiDeviceApi,
+       chromeos::features::
+           kEnableUnifiedMultiDeviceSetup} /* enabled_features */,
+      {} /* disabled_features */);
+
+  initial_feature_state_ = chromeos::multidevice_setup::mojom::FeatureState::
+      kUnavailableSuiteDisabled;
+
+  CreateTetherService();
+
+  EXPECT_EQ(
+      chromeos::NetworkStateHandler::TechnologyState::TECHNOLOGY_UNAVAILABLE,
+      network_state_handler()->GetTechnologyState(
+          chromeos::NetworkTypePattern::Tether()));
+  VerifyTetherActiveStatus(false /* expected_active */);
+
+  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 */);
+
+  ShutdownTetherService();
+  VerifyLastShutdownReason(
+      chromeos::tether::TetherComponent::ShutdownReason::USER_LOGGED_OUT);
+}
+
+TEST_F(TetherServiceTest, TestBetterTogetherSuiteBecomesDisabled) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitWithFeatures(
+      {chromeos::features::kMultiDeviceApi,
+       chromeos::features::
+           kEnableUnifiedMultiDeviceSetup} /* enabled_features */,
+      {} /* disabled_features */);
+
+  CreateTetherService();
+
+  EXPECT_EQ(chromeos::NetworkStateHandler::TechnologyState::TECHNOLOGY_ENABLED,
+            network_state_handler()->GetTechnologyState(
+                chromeos::NetworkTypePattern::Tether()));
+  VerifyTetherActiveStatus(true /* expected_active */);
+
+  fake_multidevice_setup_client_->SetFeatureState(
+      chromeos::multidevice_setup::mojom::Feature::kInstantTethering,
+      chromeos::multidevice_setup::mojom::FeatureState::
+          kUnavailableSuiteDisabled);
+
+  EXPECT_EQ(
+      chromeos::NetworkStateHandler::TechnologyState::TECHNOLOGY_UNAVAILABLE,
+      network_state_handler()->GetTechnologyState(
+          chromeos::NetworkTypePattern::Tether()));
+  VerifyTetherActiveStatus(false /* expected_active */);
+
+  ShutdownTetherService();
+  VerifyTetherFeatureStateRecorded(
+      TetherService::TetherFeatureState::BETTER_TOGETHER_SUITE_DISABLED,
+      1 /* expected_count */);
+  VerifyLastShutdownReason(chromeos::tether::TetherComponent::ShutdownReason::
+                               BETTER_TOGETHER_SUITE_DISABLED);
+}
+
 TEST_F(TetherServiceTest, TestBleAdvertisingNotSupported) {
   mock_adapter_->set_is_ble_advertising_supported(false);
 
@@ -960,9 +1025,9 @@ TEST_F(
   ASSERT_TRUE(tether_service);
 
   fake_multidevice_setup_client_impl_factory_->fake_multidevice_setup_client()
-      ->SetHostStatusWithDevice(std::make_pair(
-          chromeos::multidevice_setup::mojom::HostStatus::kHostVerified,
-          test_devices_[0]));
+      ->SetFeatureState(
+          chromeos::multidevice_setup::mojom::Feature::kInstantTethering,
+          chromeos::multidevice_setup::mojom::FeatureState::kEnabledByUser);
 
   base::RunLoop().RunUntilIdle();
   tether_service->Shutdown();

chromeos/components/tether/tether_component.h

@@ -33,7 +33,8 @@ class TetherComponent {
     BLUETOOTH_DISABLED = 6,
     CELLULAR_DISABLED = 7,
     BLUETOOTH_CONTROLLER_DISAPPEARED = 8,
-    MULTIDEVICE_HOST_UNVERIFIED = 9
+    MULTIDEVICE_HOST_UNVERIFIED = 9,
+    BETTER_TOGETHER_SUITE_DISABLED = 10
   };
 
   TetherComponent();

chromeos/components/tether/tether_component_impl.cc

@@ -58,6 +58,9 @@ GetSessionCompletionReasonFromShutdownReason(
     case TetherComponent::ShutdownReason::MULTIDEVICE_HOST_UNVERIFIED:
       return TetherSessionCompletionLogger::SessionCompletionReason::
           MULTIDEVICE_HOST_UNVERIFIED;
+    case TetherComponent::ShutdownReason::BETTER_TOGETHER_SUITE_DISABLED:
+      return TetherSessionCompletionLogger::SessionCompletionReason::
+          BETTER_TOGETHER_SUITE_DISABLED;
     default:
       break;
   }