[CrOS PhoneHub] Trigger a browser tabs sync when PhoneHub UI is opened.

Uses the SyncService to refresh the syncer::SESSIONS sync type. This
updates the browser tabs model almost immediately instead of waiting
for the server to lazily update the existing subscription.

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

ash/system/phonehub/phone_hub_ui_controller.cc

@@ -15,6 +15,7 @@
 #include "ash/system/phonehub/phone_disconnected_view.h"
 #include "ash/system/phonehub/phone_hub_content_view.h"
 #include "base/logging.h"
+#include "chromeos/components/phonehub/browser_tabs_model_provider.h"
 #include "chromeos/components/phonehub/connection_scheduler.h"
 #include "chromeos/components/phonehub/phone_hub_manager.h"
 #include "chromeos/components/phonehub/user_action_recorder.h"
@@ -95,6 +96,7 @@ void PhoneHubUiController::HandleBubbleOpened() {
   if (feature_status == FeatureStatus::kEnabledButDisconnected)
     phone_hub_manager_->GetConnectionScheduler()->ScheduleConnectionNow();
 
+  phone_hub_manager_->GetBrowserTabsModelProvider()->TriggerRefresh();
   phone_hub_manager_->GetUserActionRecorder()->RecordUiOpened();
 }
 

chrome/browser/chromeos/phonehub/browser_tabs_model_provider_impl.cc

@@ -7,6 +7,8 @@
 #include "chromeos/components/multidevice/remote_device_ref.h"
 #include "chromeos/components/phonehub/browser_tabs_metadata_fetcher.h"
 #include "chromeos/components/phonehub/browser_tabs_model.h"
+#include "components/sync/base/model_type.h"
+#include "components/sync/driver/sync_service.h"
 #include "components/sync_sessions/open_tabs_ui_delegate.h"
 #include "components/sync_sessions/session_sync_service.h"
 
@@ -15,9 +17,11 @@ namespace phonehub {
 
 BrowserTabsModelProviderImpl::BrowserTabsModelProviderImpl(
     multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
+    syncer::SyncService* sync_service,
     sync_sessions::SessionSyncService* session_sync_service,
     std::unique_ptr<BrowserTabsMetadataFetcher> browser_tabs_metadata_fetcher)
     : multidevice_setup_client_(multidevice_setup_client),
+      sync_service_(sync_service),
       session_sync_service_(session_sync_service),
       browser_tabs_metadata_fetcher_(std::move(browser_tabs_metadata_fetcher)) {
   multidevice_setup_client_->AddObserver(this);
@@ -50,6 +54,10 @@ void BrowserTabsModelProviderImpl::OnHostStatusChanged(
   AttemptBrowserTabsModelUpdate();
 }
 
+void BrowserTabsModelProviderImpl::TriggerRefresh() {
+  sync_service_->TriggerRefresh({syncer::SESSIONS});
+}
+
 void BrowserTabsModelProviderImpl::AttemptBrowserTabsModelUpdate() {
   base::Optional<std::string> session_name = GetSessionName();
   sync_sessions::OpenTabsUIDelegate* open_tabs =
@@ -100,7 +108,7 @@ void BrowserTabsModelProviderImpl::AttemptBrowserTabsModelUpdate() {
 void BrowserTabsModelProviderImpl::InvalidateWeakPtrsAndClearTabMetadata(
     bool is_tab_sync_enabled) {
   weak_ptr_factory_.InvalidateWeakPtrs();
-  BrowserTabsModelProvider::NotifyBrowserTabsUpdated(
+  NotifyBrowserTabsUpdated(
       /*is_tab_sync_enabled=*/is_tab_sync_enabled, {});
 }
 
@@ -110,7 +118,7 @@ void BrowserTabsModelProviderImpl::OnMetadataFetched(
   // The operation to fetch metadata was cancelled.
   if (!metadata)
     return;
-  BrowserTabsModelProvider::NotifyBrowserTabsUpdated(
+  NotifyBrowserTabsUpdated(
       /*is_tab_sync_enabled=*/true, *metadata);
 }
 

chrome/browser/chromeos/phonehub/browser_tabs_model_provider_impl.h

@@ -17,21 +17,39 @@ namespace sync_sessions {
 class SessionSyncService;
 }  // namespace sync_sessions
 
+namespace syncer {
+class SyncService;
+}  // namespace syncer
+
 namespace chromeos {
 namespace phonehub {
 
-// Responsible for providing the BrowserTabsModel to observers.
+// Gets the browser tab model info by finding a SyncedSession (provided lazily
+// by a SessionService) with a |session_name| that matches the |pii_free_name|
+// of the phone provided by a MultiDeviceSetupClient. If sync is enabled, the
+// class uses a BrowserTabsMetadataFetcher to actually fetch the browser tab
+// metadata once it finds the correct SyncedSession.
+//
+// Uses a SyncService in TriggerRefresh() to manually request updates for the
+// latest SyncedSessions. If updated SyncSessions exist on the server, all
+// SessionSyncService subscriptions will be updated almost immediately, instead
+// of being lazily updated and eventually consistent with the latest browser tab
+// info on the server.
 class BrowserTabsModelProviderImpl
     : public BrowserTabsModelProvider,
       public multidevice_setup::MultiDeviceSetupClient::Observer {
  public:
   BrowserTabsModelProviderImpl(
       multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
+      syncer::SyncService* sync_service,
       sync_sessions::SessionSyncService* session_sync_service,
       std::unique_ptr<BrowserTabsMetadataFetcher>
           browser_tabs_metadata_fetcher);
   ~BrowserTabsModelProviderImpl() override;
 
+  // BrowserTabsModelProvider:
+  void TriggerRefresh() override;
+
  private:
   friend class BrowserTabsModelProviderImplTest;
 
@@ -48,6 +66,7 @@ class BrowserTabsModelProviderImpl
   base::Optional<std::string> GetSessionName() const;
 
   multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client_;
+  syncer::SyncService* sync_service_;
   sync_sessions::SessionSyncService* session_sync_service_;
   std::unique_ptr<BrowserTabsMetadataFetcher> browser_tabs_metadata_fetcher_;
   base::CallbackListSubscription session_updated_subscription_;

chrome/browser/chromeos/phonehub/browser_tabs_model_provider_impl_unittest.cc

@@ -9,6 +9,7 @@
 #include "chromeos/components/phonehub/mutable_phone_model.h"
 #include "chromeos/components/phonehub/phone_model_test_util.h"
 #include "chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.h"
+#include "components/sync/driver/mock_sync_service.h"
 #include "components/sync_sessions/open_tabs_ui_delegate.h"
 #include "components/sync_sessions/session_sync_service.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -119,7 +120,8 @@ class BrowserTabsModelProviderImplTest
                                         MockSubscribeToForeignSessionsChanged));
 
     provider_ = std::make_unique<BrowserTabsModelProviderImpl>(
-        &fake_multidevice_setup_client_, &mock_session_sync_service_,
+        &fake_multidevice_setup_client_, &mock_sync_service_,
+        &mock_session_sync_service_,
         std::make_unique<FakeBrowserTabsMetadataFetcher>());
     provider_->AddObserver(this);
   }
@@ -165,6 +167,7 @@ class BrowserTabsModelProviderImplTest
 
   MutablePhoneModel phone_model_;
   multidevice_setup::FakeMultiDeviceSetupClient fake_multidevice_setup_client_;
+  testing::NiceMock<syncer::MockSyncService> mock_sync_service_;
   testing::NiceMock<SessionSyncServiceMock> mock_session_sync_service_;
   std::unique_ptr<BrowserTabsModelProviderImpl> provider_;
 

chrome/browser/chromeos/phonehub/phone_hub_manager_factory.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/chromeos/secure_channel/secure_channel_client_provider.h"
 #include "chrome/browser/favicon/history_ui_favicon_request_handler_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/sync/session_sync_service_factory.h"
 #include "chrome/browser/ui/webui/chromeos/multidevice_setup/multidevice_setup_dialog.h"
 #include "chromeos/components/phonehub/multidevice_setup_state_updater.h"
@@ -70,6 +71,7 @@ PhoneHubManagerFactory::PhoneHubManagerFactory()
   DependsOn(secure_channel::NearbyConnectorFactory::GetInstance());
   DependsOn(SessionSyncServiceFactory::GetInstance());
   DependsOn(HistoryUiFaviconRequestHandlerFactory::GetInstance());
+  DependsOn(ProfileSyncServiceFactory::GetInstance());
 }
 
 PhoneHubManagerFactory::~PhoneHubManagerFactory() = default;
@@ -96,6 +98,7 @@ KeyedService* PhoneHubManagerFactory::BuildServiceInstanceFor(
       std::make_unique<BrowserTabsModelProviderImpl>(
           multidevice_setup::MultiDeviceSetupClientFactory::GetForProfile(
               profile),
+          ProfileSyncServiceFactory::GetInstance()->GetForProfile(profile),
           SessionSyncServiceFactory::GetInstance()->GetForProfile(profile),
           std::make_unique<BrowserTabsMetadataFetcherImpl>(
               HistoryUiFaviconRequestHandlerFactory::GetInstance()

chromeos/components/phonehub/BUILD.gn

@@ -114,6 +114,8 @@ static_library("phonehub") {
 # Sources only include files used in the debug UI.
 static_library("debug") {
   sources = [
+    "fake_browser_tabs_model_provider.cc",
+    "fake_browser_tabs_model_provider.h",
     "fake_connection_scheduler.cc",
     "fake_connection_scheduler.h",
     "fake_do_not_disturb_controller.cc",

chromeos/components/phonehub/browser_tabs_model_provider.h

@@ -15,11 +15,6 @@ namespace chromeos {
 namespace phonehub {
 
 // Responsible for providing BrowserTabsModel information to observers.
-// Gets the browser tab model info by finding a SyncedSession (provided by
-// the SessionService) with a |session_name| that matches the |pii_free_name| of
-// the phone provided by a MultiDeviceSetupClient. If sync is enabled, the class
-// uses a BrowserTabsMetadataFetcher to actually fetch the browser tab metadata
-// once it finds the correct SyncedSession.
 class BrowserTabsModelProvider {
  public:
   class Observer : public base::CheckedObserver {
@@ -39,6 +34,12 @@ class BrowserTabsModelProvider {
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
+  // Used to manually request updates for the latest browser tab metadata,
+  // instead of lazily waiting for an update to occur on its own. This may be
+  // advantageous when the user opens the PhoneHub tray immediately after
+  // visiting a link on the connected phone's Chrome Browser.
+  virtual void TriggerRefresh() = 0;
+
  protected:
   BrowserTabsModelProvider();
 

chromeos/components/phonehub/fake_browser_tabs_model_provider.h

@@ -17,6 +17,9 @@ class FakeBrowserTabsModelProvider : public BrowserTabsModelProvider {
   FakeBrowserTabsModelProvider();
   ~FakeBrowserTabsModelProvider() override;
 
+  // BrowserTabsModelProvider:
+  void TriggerRefresh() override {}
+
   void NotifyBrowserTabsUpdated(
       bool is_sync_enabled,
       const std::vector<BrowserTabsModel::BrowserTabMetadata>

chromeos/components/phonehub/fake_phone_hub_manager.cc

@@ -11,6 +11,10 @@ FakePhoneHubManager::FakePhoneHubManager() = default;
 
 FakePhoneHubManager::~FakePhoneHubManager() = default;
 
+BrowserTabsModelProvider* FakePhoneHubManager::GetBrowserTabsModelProvider() {
+  return &fake_browser_tabs_model_provider_;
+}
+
 DoNotDisturbController* FakePhoneHubManager::GetDoNotDisturbController() {
   return &fake_do_not_disturb_controller_;
 }

chromeos/components/phonehub/fake_phone_hub_manager.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "chromeos/components/phonehub/fake_browser_tabs_model_provider.h"
 #include "chromeos/components/phonehub/fake_connection_scheduler.h"
 #include "chromeos/components/phonehub/fake_do_not_disturb_controller.h"
 #include "chromeos/components/phonehub/fake_feature_status_provider.h"
@@ -66,8 +67,13 @@ class FakePhoneHubManager : public PhoneHubManager {
     return &fake_user_action_recorder_;
   }
 
+  FakeBrowserTabsModelProvider* fake_browser_tabs_model_provider() {
+    return &fake_browser_tabs_model_provider_;
+  }
+
  private:
   // PhoneHubManager:
+  BrowserTabsModelProvider* GetBrowserTabsModelProvider() override;
   DoNotDisturbController* GetDoNotDisturbController() override;
   FeatureStatusProvider* GetFeatureStatusProvider() override;
   FindMyDeviceController* GetFindMyDeviceController() override;
@@ -89,6 +95,7 @@ class FakePhoneHubManager : public PhoneHubManager {
   FakeTetherController fake_tether_controller_;
   FakeConnectionScheduler fake_connection_scheduler_;
   FakeUserActionRecorder fake_user_action_recorder_;
+  FakeBrowserTabsModelProvider fake_browser_tabs_model_provider_;
 };
 
 }  // namespace phonehub

chromeos/components/phonehub/phone_hub_manager.h

@@ -18,6 +18,7 @@ class OnboardingUiTracker;
 class PhoneModel;
 class TetherController;
 class UserActionRecorder;
+class BrowserTabsModelProvider;
 
 // Responsible for the core logic of the Phone Hub feature and exposes
 // interfaces via its public API. This class is intended to be a singleton.
@@ -29,6 +30,7 @@ class PhoneHubManager {
   PhoneHubManager& operator=(const PhoneHubManager&) = delete;
 
   // Getters for sub-elements.
+  virtual BrowserTabsModelProvider* GetBrowserTabsModelProvider() = 0;
   virtual ConnectionScheduler* GetConnectionScheduler() = 0;
   virtual DoNotDisturbController* GetDoNotDisturbController() = 0;
   virtual FeatureStatusProvider* GetFeatureStatusProvider() = 0;

chromeos/components/phonehub/phone_hub_manager_impl.cc

@@ -114,6 +114,10 @@ PhoneHubManagerImpl::PhoneHubManagerImpl(
 
 PhoneHubManagerImpl::~PhoneHubManagerImpl() = default;
 
+BrowserTabsModelProvider* PhoneHubManagerImpl::GetBrowserTabsModelProvider() {
+  return browser_tabs_model_provider_.get();
+}
+
 ConnectionScheduler* PhoneHubManagerImpl::GetConnectionScheduler() {
   return connection_scheduler_.get();
 }

chromeos/components/phonehub/phone_hub_manager_impl.h

@@ -55,6 +55,7 @@ class PhoneHubManagerImpl : public PhoneHubManager, public KeyedService {
   ~PhoneHubManagerImpl() override;
 
   // PhoneHubManager:
+  BrowserTabsModelProvider* GetBrowserTabsModelProvider() override;
   ConnectionScheduler* GetConnectionScheduler() override;
   DoNotDisturbController* GetDoNotDisturbController() override;
   FeatureStatusProvider* GetFeatureStatusProvider() override;