[CrOS PhoneHub] Enable flag by default

Additionally, add a few fixes which were needed to make tests pass now
that Phone Hub is active by default:
(1) Call SystemTray->SetPhoneHubManager(nullptr) when
    PhoneHubManagerFactory is deleted.
(2) Set service to NULL while testing.
(3) Do not create the service when the Profile is created; instead,
    Create it in UserSessionInitializer.
(4) Update PhoneHubNotificationController to behave correctly when a
    null PhoneHubManager is passed to it.
(5) Do not assume that there can only be 0 or 1 Tether networks. Though
    this is true in practice, there are tests which use multiple
    networks.

Fixed: 1106937
Change-Id: I15d02e114fc81284cdad072c25c3dca24ff5bd62
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2626103
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Regan Hsu <hsuregan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843761}

ash/system/phonehub/phone_hub_notification_controller.cc

@@ -216,43 +216,45 @@ PhoneHubNotificationController::PhoneHubNotificationController() {
 PhoneHubNotificationController::~PhoneHubNotificationController() {
   if (manager_)
     manager_->RemoveObserver(this);
+  if (feature_status_provider_)
+    feature_status_provider_->RemoveObserver(this);
   if (tether_controller_)
     tether_controller_->RemoveObserver(this);
 }
 
 void PhoneHubNotificationController::SetManager(
     chromeos::phonehub::PhoneHubManager* phone_hub_manager) {
-  chromeos::phonehub::NotificationManager* notification_manager =
-      phone_hub_manager->GetNotificationManager();
-  chromeos::phonehub::FeatureStatusProvider* feature_status_provider =
-      phone_hub_manager->GetFeatureStatusProvider();
-  chromeos::phonehub::TetherController* tether_controller =
-      phone_hub_manager->GetTetherController();
-  phone_model_ = phone_hub_manager->GetPhoneModel();
-
-  if (manager_ == notification_manager &&
-      tether_controller_ == tether_controller &&
-      feature_status_provider_ == feature_status_provider) {
-    return;
-  }
-
   if (manager_)
     manager_->RemoveObserver(this);
-
-  manager_ = notification_manager;
+  if (phone_hub_manager) {
+    manager_ = phone_hub_manager->GetNotificationManager();
     manager_->AddObserver(this);
+  } else {
+    manager_ = nullptr;
+  }
 
   if (feature_status_provider_)
     feature_status_provider_->RemoveObserver(this);
-
-  feature_status_provider_ = feature_status_provider;
+  if (phone_hub_manager) {
+    feature_status_provider_ = phone_hub_manager->GetFeatureStatusProvider();
     feature_status_provider_->AddObserver(this);
+  } else {
+    feature_status_provider_ = nullptr;
+  }
 
   if (tether_controller_)
     tether_controller_->RemoveObserver(this);
-
-  tether_controller_ = tether_controller;
+  if (phone_hub_manager) {
+    tether_controller_ = phone_hub_manager->GetTetherController();
     tether_controller_->AddObserver(this);
+  } else {
+    tether_controller_ = nullptr;
+  }
+
+  if (phone_hub_manager)
+    phone_model_ = phone_hub_manager->GetPhoneModel();
+  else
+    phone_model_ = nullptr;
 }
 
 const base::string16 PhoneHubNotificationController::GetPhoneName() const {

chrome/browser/chromeos/login/session/user_session_initializer.cc

@@ -21,6 +21,7 @@
 #include "chrome/browser/chromeos/crostini/crostini_manager.h"
 #include "chrome/browser/chromeos/lock_screen_apps/state_controller.h"
 #include "chrome/browser/chromeos/login/startup_utils.h"
+#include "chrome/browser/chromeos/phonehub/phone_hub_manager_factory.h"
 #include "chrome/browser/chromeos/plugin_vm/plugin_vm_manager.h"
 #include "chrome/browser/chromeos/plugin_vm/plugin_vm_manager_factory.h"
 #include "chrome/browser/chromeos/policy/app_install_event_log_manager_wrapper.h"
@@ -216,6 +217,9 @@ void UserSessionInitializer::OnUserSessionStarted(bool is_primary_user) {
   if (is_primary_user) {
     DCHECK_EQ(primary_profile_, profile);
 
+    // Ensure that PhoneHubManager is created for the primary profile.
+    phonehub::PhoneHubManagerFactory::GetForProfile(profile);
+
     plugin_vm::PluginVmManager* plugin_vm_manager =
         plugin_vm::PluginVmManagerFactory::GetForProfile(primary_profile_);
     if (plugin_vm_manager)

chrome/browser/chromeos/phonehub/phone_hub_manager_factory.cc

@@ -31,6 +31,8 @@ namespace chromeos {
 namespace phonehub {
 namespace {
 
+content::BrowserContext* g_context_for_service = nullptr;
+
 bool IsProhibitedByPolicy(Profile* profile) {
   return !multidevice_setup::IsFeatureAllowed(
       multidevice_setup::mojom::Feature::kPhoneHub, profile->GetPrefs());
@@ -109,13 +111,42 @@ KeyedService* PhoneHubManagerFactory::BuildServiceInstanceFor(
   // the UI.
   ash::SystemTray::Get()->SetPhoneHubManager(phone_hub_manager);
 
+  DCHECK(!g_context_for_service);
+  g_context_for_service = context;
+
   return phone_hub_manager;
 }
 
-bool PhoneHubManagerFactory::ServiceIsCreatedWithBrowserContext() const {
+bool PhoneHubManagerFactory::ServiceIsNULLWhileTesting() const {
   return true;
 }
 
+bool PhoneHubManagerFactory::ServiceIsCreatedWithBrowserContext() const {
+  // We do want the service to be created with the BrowserContext, but returning
+  // true here causes issues when opting into Chrome Sync in OOBE because it
+  // causes ProfileSyncService to be created before SyncConsentScreen. Instead,
+  // we return false here and initialize PhoneHubManager within
+  // UserSessionInitializer.
+  return false;
+}
+
+void PhoneHubManagerFactory::BrowserContextShutdown(
+    content::BrowserContext* context) {
+  // If the primary Profile is being deleted, notify SystemTray that
+  // PhoneHubManager is being deleted. Note that the SystemTray is normally
+  // expected to be deleted *before* the Profile, so this check is only expected
+  // to be necessary in tests.
+  if (g_context_for_service == context) {
+    ash::SystemTray* system_tray = ash::SystemTray::Get();
+    if (system_tray)
+      system_tray->SetPhoneHubManager(nullptr);
+
+    g_context_for_service = nullptr;
+  }
+
+  BrowserContextKeyedServiceFactory::BrowserContextShutdown(context);
+}
+
 void PhoneHubManagerFactory::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   MultideviceSetupStateUpdater::RegisterPrefs(registry);

chrome/browser/chromeos/phonehub/phone_hub_manager_factory.h

@@ -35,7 +35,9 @@ class PhoneHubManagerFactory : public BrowserContextKeyedServiceFactory {
   // BrowserContextKeyedServiceFactory:
   KeyedService* BuildServiceInstanceFor(
       content::BrowserContext* context) const override;
+  bool ServiceIsNULLWhileTesting() const override;
   bool ServiceIsCreatedWithBrowserContext() const override;
+  void BrowserContextShutdown(content::BrowserContext* context) override;
   void RegisterProfilePrefs(
       user_prefs::PrefRegistrySyncable* registry) override;
 };

chromeos/components/phonehub/tether_controller_impl.cc

@@ -340,11 +340,11 @@ void TetherControllerImpl::OnVisibleTetherNetworkFetched(
   network_config::mojom::NetworkStatePropertiesPtr previous_tether_network =
       std::move(tether_network_);
 
-  // The number of tether networks should only ever be at most 1.
-  if (networks.size() == 1) {
+  if (!networks.empty()) {
+    // The number of tether networks is expected to be at most 1, though some
+    // tests do use multiple networks.
     tether_network_ = std::move(networks[0]);
   } else {
-    DCHECK(networks.empty());
     tether_network_ = nullptr;
   }
 

chromeos/constants/chromeos_features.cc

@@ -498,7 +498,7 @@ const base::Feature kOsSettingsPolymer3{"OsSettingsPolymer3",
 
 // Provides a UI for users to view information about their Android phone
 // and perform phone-side actions within Chrome OS.
-const base::Feature kPhoneHub{"PhoneHub", base::FEATURE_DISABLED_BY_DEFAULT};
+const base::Feature kPhoneHub{"PhoneHub", base::FEATURE_ENABLED_BY_DEFAULT};
 
 // Controls whether Phone Hub will exclusively use BLE for its connection with
 // the user's phone.