[CrOS PhoneHub] Add implementations to OnboardingUiTrackerImpl.

OnboardingUiTracker implementation that uses the
|kHasCompletedOnboardingBefore| pref to determine whether the
Onboarding UI should be shown. This class invokes
|show_multidevice_setup_dialog_callback| when the user proceeds with
the onboarding flow if Better Together is disabled. If Better Together
is enabled, but PhoneHub is disabled, this class enables the PhoneHub
feature via the MultiDeviceSetupClient instead.

Bug: 1106937
Change-Id: I9453754edf3a51a4056bfce16a7cf8c9c409d7b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2437732
Commit-Queue: Regan Hsu <hsuregan@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812282}

chrome/browser/chromeos/phonehub/phone_hub_manager_factory.cc

@@ -10,7 +10,9 @@
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/secure_channel/secure_channel_client_provider.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/webui/chromeos/multidevice_setup/multidevice_setup_dialog.h"
 #include "chromeos/components/phonehub/notification_access_manager_impl.h"
+#include "chromeos/components/phonehub/onboarding_ui_tracker_impl.h"
 #include "chromeos/components/phonehub/phone_hub_manager_impl.h"
 #include "chromeos/constants/chromeos_features.h"
 #include "chromeos/services/multidevice_setup/public/cpp/prefs.h"
@@ -80,7 +82,8 @@ KeyedService* PhoneHubManagerFactory::BuildServiceInstanceFor(
       profile->GetPrefs(),
       device_sync::DeviceSyncClientFactory::GetForProfile(profile),
       multidevice_setup::MultiDeviceSetupClientFactory::GetForProfile(profile),
-      secure_channel::SecureChannelClientProvider::GetInstance()->GetClient());
+      secure_channel::SecureChannelClientProvider::GetInstance()->GetClient(),
+      base::BindRepeating(&multidevice_setup::MultiDeviceSetupDialog::Show));
 
   // Provide |phone_hub_manager| to the system tray so that it can be used by
   // the UI.
@@ -96,6 +99,7 @@ bool PhoneHubManagerFactory::ServiceIsCreatedWithBrowserContext() const {
 void PhoneHubManagerFactory::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   NotificationAccessManagerImpl::RegisterPrefs(registry);
+  OnboardingUiTrackerImpl::RegisterPrefs(registry);
 }
 
 }  // namespace phonehub

chromeos/components/phonehub/fake_onboarding_ui_tracker.cc

@@ -28,5 +28,9 @@ void FakeOnboardingUiTracker::DismissSetupUi() {
   SetShouldShowOnboardingUi(false);
 }
 
+void FakeOnboardingUiTracker::HandleGetStarted() {
+  ++handle_get_started_call_count_;
+}
+
 }  // namespace phonehub
 }  // namespace chromeos

chromeos/components/phonehub/fake_onboarding_ui_tracker.h

@@ -17,12 +17,18 @@ class FakeOnboardingUiTracker : public OnboardingUiTracker {
 
   void SetShouldShowOnboardingUi(bool should_show_onboarding_ui);
 
+  size_t handle_get_started_call_count() {
+    return handle_get_started_call_count_;
+  }
+
  private:
   // OnboardingUiTracker:
   bool ShouldShowOnboardingUi() const override;
   void DismissSetupUi() override;
+  void HandleGetStarted() override;
 
   bool should_show_onboarding_ui_ = false;
+  size_t handle_get_started_call_count_ = 0;
 };
 
 }  // namespace phonehub

chromeos/components/phonehub/onboarding_ui_tracker.h

@@ -42,6 +42,9 @@ class OnboardingUiTracker {
   // device.
   virtual void DismissSetupUi() = 0;
 
+  // Handle for when the user clicks the get started button.
+  virtual void HandleGetStarted() = 0;
+
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 

chromeos/components/phonehub/onboarding_ui_tracker_impl.cc

@@ -3,22 +3,89 @@
 // found in the LICENSE file.
 
 #include "chromeos/components/phonehub/onboarding_ui_tracker_impl.h"
+#include "chromeos/components/phonehub/feature_status.h"
+#include "chromeos/components/phonehub/pref_names.h"
+#include "chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
 
 namespace chromeos {
 namespace phonehub {
 
-OnboardingUiTrackerImpl::OnboardingUiTrackerImpl() = default;
+void OnboardingUiTrackerImpl::RegisterPrefs(PrefRegistrySimple* registry) {
+  registry->RegisterBooleanPref(prefs::kHasDismissedUiAfterCompletingOnboarding,
+                                false);
+}
+
+OnboardingUiTrackerImpl::OnboardingUiTrackerImpl(
+    PrefService* pref_service,
+    FeatureStatusProvider* feature_status_provider,
+    multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
+    const base::RepeatingClosure& show_multidevice_setup_dialog_callback)
+    : pref_service_(pref_service),
+      feature_status_provider_(feature_status_provider),
+      multidevice_setup_client_(multidevice_setup_client),
+      show_multidevice_setup_dialog_callback_(
+          std::move(show_multidevice_setup_dialog_callback)) {
+  feature_status_provider_->AddObserver(this);
+  should_show_onboarding_ui_ = ComputeShouldShowOnboardingUi();
+}
 
-OnboardingUiTrackerImpl::~OnboardingUiTrackerImpl() = default;
+OnboardingUiTrackerImpl::~OnboardingUiTrackerImpl() {
+  feature_status_provider_->RemoveObserver(this);
+}
 
 bool OnboardingUiTrackerImpl::ShouldShowOnboardingUi() const {
-  // TODO(https://crbug.com/1106937): Return a real value.
-  return false;
+  return should_show_onboarding_ui_;
 }
 
 void OnboardingUiTrackerImpl::DismissSetupUi() {
-  // TODO(https://crbug.com/1106937): Store this in a pref so that we do not
-  // show this UI to the user again.
+  pref_service_->SetBoolean(prefs::kHasDismissedUiAfterCompletingOnboarding,
+                            true);
+  UpdateShouldShowOnboardingUi();
+}
+
+void OnboardingUiTrackerImpl::HandleGetStarted() {
+  FeatureStatus status = feature_status_provider_->GetStatus();
+
+  // The user is not opted into Better Together yet.
+  if (status == FeatureStatus::kEligiblePhoneButNotSetUp) {
+    show_multidevice_setup_dialog_callback_.Run();
+    return;
+  }
+
+  // The user is already opted into Better Together, but not Phone Hub.
+  if (status == FeatureStatus::kDisabled) {
+    multidevice_setup_client_->SetFeatureEnabledState(
+        multidevice_setup::mojom::Feature::kPhoneHub,
+        /*enabled=*/true, /*auth_token=*/base::nullopt, base::DoNothing());
+    return;
+  }
+  LOG(ERROR)
+      << "Cannot handle a GetStarted request because the current state is "
+      << status;
+}
+
+void OnboardingUiTrackerImpl::OnFeatureStatusChanged() {
+  UpdateShouldShowOnboardingUi();
+}
+
+bool OnboardingUiTrackerImpl::ComputeShouldShowOnboardingUi() {
+  FeatureStatus status = feature_status_provider_->GetStatus();
+  if (status == FeatureStatus::kEligiblePhoneButNotSetUp ||
+      status == FeatureStatus::kDisabled) {
+    return !pref_service_->GetBoolean(
+        prefs::kHasDismissedUiAfterCompletingOnboarding);
+  }
+  return false;
+}
+
+void OnboardingUiTrackerImpl::UpdateShouldShowOnboardingUi() {
+  bool should_show_onboarding_ui = ComputeShouldShowOnboardingUi();
+  if (should_show_onboarding_ui_ == should_show_onboarding_ui)
+    return;
+  should_show_onboarding_ui_ = should_show_onboarding_ui;
+  NotifyShouldShowOnboardingUiChanged();
 }
 
 }  // namespace phonehub

chromeos/components/phonehub/onboarding_ui_tracker_impl.h

@@ -5,21 +5,52 @@
 #ifndef CHROMEOS_COMPONENTS_PHONEHUB_ONBOARDING_UI_TRACKER_IMPL_H_
 #define CHROMEOS_COMPONENTS_PHONEHUB_ONBOARDING_UI_TRACKER_IMPL_H_
 
+#include "base/callback.h"
+#include "chromeos/components/phonehub/feature_status_provider.h"
 #include "chromeos/components/phonehub/onboarding_ui_tracker.h"
+#include "chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.h"
+
+class PrefRegistrySimple;
+class PrefService;
 
 namespace chromeos {
 namespace phonehub {
 
-// TODO(https://crbug.com/1106937): Add real implementation.
-class OnboardingUiTrackerImpl : public OnboardingUiTracker {
+// OnboardingUiTracker implementation that uses the
+// |kHasDismissedUiAfterCompletingOnboarding| pref to determine whether the
+// Onboarding UI should be shown. This class invokes
+// |show_multidevice_setup_dialog_callback| when the user proceeds with the
+// onboarding flow if Better Together is disabled. If Better Together is
+// enabled, but PhoneHub is disabled, this class enables the PhoneHub feature
+// via the MultiDeviceSetupClient instead.
+class OnboardingUiTrackerImpl : public OnboardingUiTracker,
+                                public FeatureStatusProvider::Observer {
  public:
-  OnboardingUiTrackerImpl();
+  static void RegisterPrefs(PrefRegistrySimple* registry);
+
+  OnboardingUiTrackerImpl(
+      PrefService* pref_service,
+      FeatureStatusProvider* feature_status_provider,
+      multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
+      const base::RepeatingClosure& show_multidevice_setup_dialog_callback);
   ~OnboardingUiTrackerImpl() override;
 
- private:
   // OnboardingUiTracker:
   bool ShouldShowOnboardingUi() const override;
   void DismissSetupUi() override;
+  void HandleGetStarted() override;
+
+ private:
+  // FeatureStatusProvider::Observer:
+  void OnFeatureStatusChanged() override;
+
+  bool ComputeShouldShowOnboardingUi();
+  void UpdateShouldShowOnboardingUi();
+  PrefService* pref_service_;
+  FeatureStatusProvider* feature_status_provider_;
+  multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client_;
+  bool should_show_onboarding_ui_;
+  base::RepeatingClosure show_multidevice_setup_dialog_callback_;
 };
 
 }  // namespace phonehub

chromeos/components/phonehub/onboarding_ui_tracker_impl_unittest.cc

@@ -6,12 +6,22 @@
 
 #include <memory>
 
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/logging.h"
+#include "chromeos/components/phonehub/fake_feature_status_provider.h"
+#include "chromeos/components/phonehub/feature_status.h"
+#include "chromeos/components/phonehub/pref_names.h"
+#include "chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.h"
+#include "components/prefs/testing_pref_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace chromeos {
 namespace phonehub {
 namespace {
 
+using multidevice_setup::mojom::Feature;
+
 class FakeObserver : public OnboardingUiTracker::Observer {
  public:
   FakeObserver() = default;
@@ -38,26 +48,115 @@ class OnboardingUiTrackerImplTest : public testing::Test {
 
   // testing::Test:
   void SetUp() override {
-    controller_ = std::make_unique<OnboardingUiTrackerImpl>();
+    OnboardingUiTrackerImpl::RegisterPrefs(pref_service_.registry());
+    fake_feature_status_provider_ =
+        std::make_unique<FakeFeatureStatusProvider>();
+    controller_ = std::make_unique<OnboardingUiTrackerImpl>(
+        &pref_service_, fake_feature_status_provider_.get(),
+        &fake_multidevice_setup_client_,
+        base::BindRepeating(
+            &OnboardingUiTrackerImplTest::ShowMultideviceSetupDialog,
+            base::Unretained(this)));
     controller_->AddObserver(&fake_observer_);
   }
 
   void TearDown() override { controller_->RemoveObserver(&fake_observer_); }
 
+  void ShowMultideviceSetupDialog() { ++setup_dialog_shown_count_; }
+
+  size_t setup_dialog_shown_count() { return setup_dialog_shown_count_; }
+
+  void SetStatus(FeatureStatus feature_status) {
+    fake_feature_status_provider_->SetStatus(feature_status);
+  }
+
+  void DismissSetupUi() { controller_->DismissSetupUi(); }
+
   bool ShouldShowOnboardingUi() const {
     return controller_->ShouldShowOnboardingUi();
   }
 
   size_t GetNumObserverCalls() const { return fake_observer_.num_calls(); }
 
+  void HandleGetStarted() { controller_->HandleGetStarted(); }
+
+  void InvokePendingSetFeatureEnabledStateCallback(bool expected_enabled) {
+    fake_multidevice_setup_client_.InvokePendingSetFeatureEnabledStateCallback(
+        Feature::kPhoneHub, expected_enabled, base::nullopt, true);
+  }
+
+  size_t GetOnShouldShowOnboardingUiChangedCallCount() {
+    return fake_observer_.num_calls();
+  }
+
  private:
+  size_t setup_dialog_shown_count_ = 0;
+  TestingPrefServiceSimple pref_service_;
+  multidevice_setup::FakeMultiDeviceSetupClient fake_multidevice_setup_client_;
+  std::unique_ptr<FakeFeatureStatusProvider> fake_feature_status_provider_;
   FakeObserver fake_observer_;
   std::unique_ptr<OnboardingUiTracker> controller_;
 };
 
-// TODO(https://crbug.com/1106937): Remove this test once we have real
-// functionality to test.
-TEST_F(OnboardingUiTrackerImplTest, Initialize) {
+TEST_F(OnboardingUiTrackerImplTest, ShouldShowUiWhenEligiblePhoneButNotSetup) {
+  SetStatus(FeatureStatus::kNotEligibleForFeature);
+  EXPECT_EQ(GetOnShouldShowOnboardingUiChangedCallCount(), 0U);
+
+  SetStatus(FeatureStatus::kEligiblePhoneButNotSetUp);
+  EXPECT_EQ(GetOnShouldShowOnboardingUiChangedCallCount(), 1U);
+  EXPECT_TRUE(ShouldShowOnboardingUi());
+
+  // User clicks get started button.
+  EXPECT_EQ(setup_dialog_shown_count(), 0U);
+  HandleGetStarted();
+  EXPECT_EQ(setup_dialog_shown_count(), 1U);
+  EXPECT_EQ(GetOnShouldShowOnboardingUiChangedCallCount(), 1U);
+
+  // User dismisses setup flow. After dismissal, ShouldShowOnboardingUi() should
+  // always be false.
+  DismissSetupUi();
+  EXPECT_EQ(GetOnShouldShowOnboardingUiChangedCallCount(), 2U);
+  EXPECT_FALSE(ShouldShowOnboardingUi());
+
+  // User clicks the get started button a second time; should still open.
+  EXPECT_EQ(setup_dialog_shown_count(), 1U);
+  HandleGetStarted();
+  EXPECT_EQ(setup_dialog_shown_count(), 2U);
+  EXPECT_EQ(GetOnShouldShowOnboardingUiChangedCallCount(), 2U);
+
+  // User dismisses setup flow a second time.
+  DismissSetupUi();
+  EXPECT_EQ(GetOnShouldShowOnboardingUiChangedCallCount(), 2U);
+  EXPECT_FALSE(ShouldShowOnboardingUi());
+}
+
+TEST_F(OnboardingUiTrackerImplTest, ShouldShowUiWhenDisabled) {
+  SetStatus(FeatureStatus::kNotEligibleForFeature);
+  EXPECT_EQ(GetOnShouldShowOnboardingUiChangedCallCount(), 0U);
+
+  SetStatus(FeatureStatus::kDisabled);
+  EXPECT_EQ(GetOnShouldShowOnboardingUiChangedCallCount(), 1U);
+  EXPECT_TRUE(ShouldShowOnboardingUi());
+
+  // User clicks get started button.
+  HandleGetStarted();
+  InvokePendingSetFeatureEnabledStateCallback(true);
+  EXPECT_EQ(GetOnShouldShowOnboardingUiChangedCallCount(), 1U);
+
+  // User dismisses setup flow. After dismissal, ShouldShowOnboardingUi() should
+  // always be false.
+  DismissSetupUi();
+  EXPECT_EQ(GetOnShouldShowOnboardingUiChangedCallCount(), 2U);
+  EXPECT_FALSE(ShouldShowOnboardingUi());
+
+  // User clicks the get started button a second time; should still open.
+  HandleGetStarted();
+  InvokePendingSetFeatureEnabledStateCallback(true);
+  EXPECT_EQ(GetOnShouldShowOnboardingUiChangedCallCount(), 2U);
+
+  // User dismisses setup flow a second time.
+  DismissSetupUi();
+  EXPECT_EQ(GetOnShouldShowOnboardingUiChangedCallCount(), 2U);
   EXPECT_FALSE(ShouldShowOnboardingUi());
 }
 

chromeos/components/phonehub/phone_hub_manager_impl.cc

@@ -24,7 +24,8 @@ PhoneHubManagerImpl::PhoneHubManagerImpl(
     PrefService* pref_service,
     device_sync::DeviceSyncClient* device_sync_client,
     multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
-    chromeos::secure_channel::SecureChannelClient* secure_channel_client)
+    chromeos::secure_channel::SecureChannelClient* secure_channel_client,
+    const base::RepeatingClosure& show_multidevice_setup_dialog_callback)
     : do_not_disturb_controller_(
           std::make_unique<DoNotDisturbControllerImpl>()),
       connection_manager_(
@@ -47,7 +48,11 @@ PhoneHubManagerImpl::PhoneHubManagerImpl(
       notification_access_manager_(
           std::make_unique<NotificationAccessManagerImpl>(pref_service)),
       notification_manager_(std::make_unique<NotificationManagerImpl>()),
-      onboarding_ui_tracker_(std::make_unique<OnboardingUiTrackerImpl>()),
+      onboarding_ui_tracker_(std::make_unique<OnboardingUiTrackerImpl>(
+          pref_service,
+          feature_status_provider_.get(),
+          multidevice_setup_client,
+          show_multidevice_setup_dialog_callback)),
       phone_model_(std::make_unique<MutablePhoneModel>()),
       tether_controller_(
           std::make_unique<TetherControllerImpl>(multidevice_setup_client)) {}

chromeos/components/phonehub/phone_hub_manager_impl.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "base/callback.h"
 #include "chromeos/components/phonehub/phone_hub_manager.h"
 #include "components/keyed_service/core/keyed_service.h"
 
@@ -39,7 +40,8 @@ class PhoneHubManagerImpl : public PhoneHubManager, public KeyedService {
       PrefService* pref_service,
       device_sync::DeviceSyncClient* device_sync_client,
       multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
-      chromeos::secure_channel::SecureChannelClient* secure_channel_client);
+      chromeos::secure_channel::SecureChannelClient* secure_channel_client,
+      const base::RepeatingClosure& show_multidevice_setup_dialog_callback);
   ~PhoneHubManagerImpl() override;
 
   // PhoneHubManager:

chromeos/components/phonehub/pref_names.cc

@@ -12,6 +12,10 @@ namespace prefs {
 const char kNotificationAccessGranted[] =
     "cros.phonehub.notification_access_granted";
 
+// Whether user has completed onboarding and dismissed the UI before.
+const char kHasDismissedUiAfterCompletingOnboarding[] =
+    "cros.phonehub.has_completed_onboarding_before";
+
 }  // namespace prefs
 }  // namespace phonehub
 }  // namespace chromeos

chromeos/components/phonehub/pref_names.h

@@ -10,6 +10,7 @@ namespace phonehub {
 namespace prefs {
 
 extern const char kNotificationAccessGranted[];
+extern const char kHasDismissedUiAfterCompletingOnboarding[];
 
 }  // namespace prefs
 }  // namespace phonehub