[CrOS PhoneHub] Attempt fetching browser tabs on new devices synced.

Sometimes, the browser tabs do not appear in PhoneHub. This may be
because the decrypted metadata (which includes the pii free name) may
not have been ready during initialization. This CL adds a refetch of
browser tab metadata when the decrypted metadata is ready in those
instances.

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

chrome/browser/chromeos/phonehub/browser_tabs_model_provider_impl.cc

@@ -14,12 +14,15 @@ namespace chromeos {
 namespace phonehub {
 
 BrowserTabsModelProviderImpl::BrowserTabsModelProviderImpl(
+    device_sync::DeviceSyncClient* device_sync_client,
     multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
     sync_sessions::SessionSyncService* session_sync_service,
     std::unique_ptr<BrowserTabsMetadataFetcher> browser_tabs_metadata_fetcher)
-    : multidevice_setup_client_(multidevice_setup_client),
+    : device_sync_client_(device_sync_client),
+      multidevice_setup_client_(multidevice_setup_client),
       session_sync_service_(session_sync_service),
       browser_tabs_metadata_fetcher_(std::move(browser_tabs_metadata_fetcher)) {
+  device_sync_client_->AddObserver(this);
   multidevice_setup_client_->AddObserver(this);
   session_updated_subscription_ =
       session_sync_service->SubscribeToForeignSessionsChanged(
@@ -30,6 +33,7 @@ BrowserTabsModelProviderImpl::BrowserTabsModelProviderImpl(
 }
 
 BrowserTabsModelProviderImpl::~BrowserTabsModelProviderImpl() {
+  device_sync_client_->RemoveObserver(this);
   multidevice_setup_client_->RemoveObserver(this);
 }
 
@@ -44,6 +48,12 @@ base::Optional<std::string> BrowserTabsModelProviderImpl::GetSessionName()
   return host_device_with_status.second->pii_free_name();
 }
 
+void BrowserTabsModelProviderImpl::OnNewDevicesSynced() {
+  // The decrypted metadata (which includes the pii free name) may not have been
+  // ready during initialization. Refetch when the decrypted metadata is ready.
+  AttemptBrowserTabsModelUpdate();
+}
+
 void BrowserTabsModelProviderImpl::OnHostStatusChanged(
     const multidevice_setup::MultiDeviceSetupClient::HostStatusWithDevice&
         host_device_with_status) {
@@ -54,7 +64,7 @@ void BrowserTabsModelProviderImpl::AttemptBrowserTabsModelUpdate() {
   base::Optional<std::string> session_name = GetSessionName();
   sync_sessions::OpenTabsUIDelegate* open_tabs =
       session_sync_service_->GetOpenTabsUIDelegate();
-  // Tab sync is disabled or no valid |pii_free_name_|.
+  // Tab sync is disabled or no valid |pii_free_name|.
   if (!open_tabs || !session_name) {
     InvalidateWeakPtrsAndClearTabMetadata(/*is_tab_sync_enabled=*/false);
     return;
@@ -100,8 +110,7 @@ void BrowserTabsModelProviderImpl::AttemptBrowserTabsModelUpdate() {
 void BrowserTabsModelProviderImpl::InvalidateWeakPtrsAndClearTabMetadata(
     bool is_tab_sync_enabled) {
   weak_ptr_factory_.InvalidateWeakPtrs();
-  BrowserTabsModelProvider::NotifyBrowserTabsUpdated(
-      /*is_tab_sync_enabled=*/is_tab_sync_enabled, {});
+  NotifyBrowserTabsUpdated(/*is_tab_sync_enabled=*/is_tab_sync_enabled, {});
 }
 
 void BrowserTabsModelProviderImpl::OnMetadataFetched(
@@ -110,8 +119,7 @@ void BrowserTabsModelProviderImpl::OnMetadataFetched(
   // The operation to fetch metadata was cancelled.
   if (!metadata)
     return;
-  BrowserTabsModelProvider::NotifyBrowserTabsUpdated(
-      /*is_tab_sync_enabled=*/true, *metadata);
+  NotifyBrowserTabsUpdated(/*is_tab_sync_enabled=*/true, *metadata);
 }
 
 }  // namespace phonehub

chrome/browser/chromeos/phonehub/browser_tabs_model_provider_impl.h

@@ -11,6 +11,7 @@
 #include "base/memory/weak_ptr.h"
 #include "chromeos/components/phonehub/browser_tabs_metadata_fetcher.h"
 #include "chromeos/components/phonehub/browser_tabs_model_provider.h"
+#include "chromeos/services/device_sync/public/cpp/device_sync_client.h"
 #include "chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.h"
 
 namespace sync_sessions {
@@ -23,9 +24,11 @@ namespace phonehub {
 // Responsible for providing the BrowserTabsModel to observers.
 class BrowserTabsModelProviderImpl
     : public BrowserTabsModelProvider,
+      public device_sync::DeviceSyncClient::Observer,
       public multidevice_setup::MultiDeviceSetupClient::Observer {
  public:
   BrowserTabsModelProviderImpl(
+      device_sync::DeviceSyncClient* device_sync_client,
       multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
       sync_sessions::SessionSyncService* session_sync_service,
       std::unique_ptr<BrowserTabsMetadataFetcher>
@@ -35,6 +38,9 @@ class BrowserTabsModelProviderImpl
  private:
   friend class BrowserTabsModelProviderImplTest;
 
+  // device_sync::DeviceSyncClient::Observer:
+  void OnNewDevicesSynced() override;
+
   // multidevice_setup::MultiDeviceSetupClient::Observer:
   void OnHostStatusChanged(
       const multidevice_setup::MultiDeviceSetupClient::HostStatusWithDevice&
@@ -47,6 +53,7 @@ class BrowserTabsModelProviderImpl
           metadata);
   base::Optional<std::string> GetSessionName() const;
 
+  device_sync::DeviceSyncClient* device_sync_client_;
   multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client_;
   sync_sessions::SessionSyncService* session_sync_service_;
   std::unique_ptr<BrowserTabsMetadataFetcher> browser_tabs_metadata_fetcher_;

chrome/browser/chromeos/phonehub/browser_tabs_model_provider_impl_unittest.cc

@@ -8,6 +8,7 @@
 #include "chromeos/components/phonehub/fake_browser_tabs_metadata_fetcher.h"
 #include "chromeos/components/phonehub/mutable_phone_model.h"
 #include "chromeos/components/phonehub/phone_model_test_util.h"
+#include "chromeos/services/device_sync/public/cpp/fake_device_sync_client.h"
 #include "chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.h"
 #include "components/sync_sessions/open_tabs_ui_delegate.h"
 #include "components/sync_sessions/session_sync_service.h"
@@ -95,11 +96,13 @@ class BrowserTabsModelProviderImplTest
       const BrowserTabsModelProviderImplTest&) = delete;
   ~BrowserTabsModelProviderImplTest() override = default;
 
+  // BrowserTabsModelProvider::Observer:
   void OnBrowserTabsUpdated(
       bool is_sync_enabled,
       const std::vector<BrowserTabsModel::BrowserTabMetadata>&
           browser_tabs_metadata) override {
-    phone_model_.SetBrowserTabsModel(BrowserTabsModel(
+    num_observer_calls_++;
+    phone_model()->SetBrowserTabsModel(BrowserTabsModel(
         /*is_tab_sync_enabled=*/is_sync_enabled, browser_tabs_metadata));
   }
 
@@ -118,13 +121,16 @@ class BrowserTabsModelProviderImplTest
         .WillByDefault(Invoke(this, &BrowserTabsModelProviderImplTest::
                                         MockSubscribeToForeignSessionsChanged));
 
+    fake_device_sync_client_.NotifyReady();
+
     provider_ = std::make_unique<BrowserTabsModelProviderImpl>(
-        &fake_multidevice_setup_client_, &mock_session_sync_service_,
+        &fake_device_sync_client_, &fake_multidevice_setup_client_,
+        &mock_session_sync_service_,
         std::make_unique<FakeBrowserTabsMetadataFetcher>());
     provider_->AddObserver(this);
   }
 
-  void SetPiiFreeName(const std::string& pii_free_name) {
+  void SetNewDeviceWithPiiFreeName(const std::string& pii_free_name) {
     fake_multidevice_setup_client_.SetHostStatusWithDevice(std::make_pair(
         multidevice_setup::mojom::HostStatus::kEligibleHostExistsButNoHostSet,
         CreatePhoneDevice(/*pii_name=*/pii_free_name)));
@@ -163,16 +169,26 @@ class BrowserTabsModelProviderImplTest
         provider_->browser_tabs_metadata_fetcher_.get());
   }
 
+  MutablePhoneModel* phone_model() { return &phone_model_; }
+
+  device_sync::FakeDeviceSyncClient* fake_device_sync_client() {
+    return &fake_device_sync_client_;
+  }
+
+  size_t GetNumObserverCalls() { return num_observer_calls_; }
+
+ private:
   MutablePhoneModel phone_model_;
+  device_sync::FakeDeviceSyncClient fake_device_sync_client_;
   multidevice_setup::FakeMultiDeviceSetupClient fake_multidevice_setup_client_;
   testing::NiceMock<SessionSyncServiceMock> mock_session_sync_service_;
   std::unique_ptr<BrowserTabsModelProviderImpl> provider_;
-
   testing::NiceMock<OpenTabsUIDelegateMock> open_tabs_ui_delegate_;
 
   bool enable_tab_sync_ = true;
   std::vector<const sync_sessions::SyncedSession*>* sessions_ = nullptr;
   base::RepeatingClosure foreign_sessions_changed_callback_;
+  size_t num_observer_calls_ = 0;
 };
 
 TEST_F(BrowserTabsModelProviderImplTest, AttemptBrowserTabsModelUpdate) {
@@ -180,31 +196,42 @@ TEST_F(BrowserTabsModelProviderImplTest, AttemptBrowserTabsModelUpdate) {
   set_enable_tab_sync(true);
   set_synced_sessions(nullptr);
   NotifySubscription();
-  EXPECT_FALSE(phone_model_.browser_tabs_model()->is_tab_sync_enabled());
-  EXPECT_TRUE(phone_model_.browser_tabs_model()->most_recent_tabs().empty());
+  EXPECT_FALSE(phone_model()->browser_tabs_model()->is_tab_sync_enabled());
+  EXPECT_TRUE(phone_model()->browser_tabs_model()->most_recent_tabs().empty());
   EXPECT_FALSE(
       fake_browser_tabs_metadata_fetcher()->DoesPendingCallbackExist());
+  EXPECT_EQ(GetNumObserverCalls(), 1);
+
+  fake_device_sync_client()->NotifyNewDevicesSynced();
+  EXPECT_EQ(GetNumObserverCalls(), 2);
 
-  // Set name of phone. Tests that OnHostStatusChanged causes name change.
-  SetPiiFreeName(kPhoneNameOne);
+  // Set new host device. Tests that OnHostStatusChanged causes a fetch attempt.
+  SetNewDeviceWithPiiFreeName(kPhoneNameOne);
+  EXPECT_TRUE(phone_model()->browser_tabs_model()->is_tab_sync_enabled());
+  EXPECT_TRUE(phone_model()->browser_tabs_model()->most_recent_tabs().empty());
+  EXPECT_FALSE(
+      fake_browser_tabs_metadata_fetcher()->DoesPendingCallbackExist());
+  EXPECT_EQ(GetNumObserverCalls(), 3);
 
   // Test disabling tab sync with no browser tab metadata.
   set_enable_tab_sync(false);
   set_synced_sessions(nullptr);
   NotifySubscription();
-  EXPECT_FALSE(phone_model_.browser_tabs_model()->is_tab_sync_enabled());
-  EXPECT_TRUE(phone_model_.browser_tabs_model()->most_recent_tabs().empty());
+  EXPECT_FALSE(phone_model()->browser_tabs_model()->is_tab_sync_enabled());
+  EXPECT_TRUE(phone_model()->browser_tabs_model()->most_recent_tabs().empty());
   EXPECT_FALSE(
       fake_browser_tabs_metadata_fetcher()->DoesPendingCallbackExist());
+  EXPECT_EQ(GetNumObserverCalls(), 4);
 
   // Test enabling tab sync with no browser tab metadata.
   set_enable_tab_sync(true);
   set_synced_sessions(nullptr);
   NotifySubscription();
-  EXPECT_TRUE(phone_model_.browser_tabs_model()->is_tab_sync_enabled());
-  EXPECT_TRUE(phone_model_.browser_tabs_model()->most_recent_tabs().empty());
+  EXPECT_TRUE(phone_model()->browser_tabs_model()->is_tab_sync_enabled());
+  EXPECT_TRUE(phone_model()->browser_tabs_model()->most_recent_tabs().empty());
   EXPECT_FALSE(
       fake_browser_tabs_metadata_fetcher()->DoesPendingCallbackExist());
+  EXPECT_EQ(GetNumObserverCalls(), 5);
 
   // Test enabling tab sync with no matching pii name with session_name.
   std::vector<const sync_sessions::SyncedSession*> sessions;
@@ -213,10 +240,11 @@ TEST_F(BrowserTabsModelProviderImplTest, AttemptBrowserTabsModelUpdate) {
   set_enable_tab_sync(true);
   set_synced_sessions(&sessions);
   NotifySubscription();
-  EXPECT_TRUE(phone_model_.browser_tabs_model()->is_tab_sync_enabled());
-  EXPECT_TRUE(phone_model_.browser_tabs_model()->most_recent_tabs().empty());
+  EXPECT_TRUE(phone_model()->browser_tabs_model()->is_tab_sync_enabled());
+  EXPECT_TRUE(phone_model()->browser_tabs_model()->most_recent_tabs().empty());
   EXPECT_FALSE(
       fake_browser_tabs_metadata_fetcher()->DoesPendingCallbackExist());
+  EXPECT_EQ(GetNumObserverCalls(), 6);
 
   // Test enabling tab sync with matching pii name with session_name, which
   // will cause the |fake_browser_tabs_metadata_fetcher()| to have a pending
@@ -226,8 +254,8 @@ TEST_F(BrowserTabsModelProviderImplTest, AttemptBrowserTabsModelUpdate) {
   set_enable_tab_sync(true);
   set_synced_sessions(&sessions);
   NotifySubscription();
-  EXPECT_TRUE(phone_model_.browser_tabs_model()->is_tab_sync_enabled());
-  EXPECT_TRUE(phone_model_.browser_tabs_model()->most_recent_tabs().empty());
+  EXPECT_TRUE(phone_model()->browser_tabs_model()->is_tab_sync_enabled());
+  EXPECT_TRUE(phone_model()->browser_tabs_model()->most_recent_tabs().empty());
   EXPECT_TRUE(fake_browser_tabs_metadata_fetcher()->DoesPendingCallbackExist());
 
   // Test that once |fake_browser_tabs_metadata_fetcher()| responds, the phone
@@ -236,11 +264,12 @@ TEST_F(BrowserTabsModelProviderImplTest, AttemptBrowserTabsModelUpdate) {
   metadata.push_back(CreateFakeBrowserTabMetadata());
   fake_browser_tabs_metadata_fetcher()->RespondToCurrentFetchAttempt(
       std::move(metadata));
-  EXPECT_EQ(phone_model_.browser_tabs_model()->most_recent_tabs().size(), 1U);
+  EXPECT_EQ(phone_model()->browser_tabs_model()->most_recent_tabs().size(), 1U);
+  EXPECT_EQ(GetNumObserverCalls(), 7);
 }
 
 TEST_F(BrowserTabsModelProviderImplTest, ClearTabMetadataDuringMetadataFetch) {
-  SetPiiFreeName(kPhoneNameOne);
+  SetNewDeviceWithPiiFreeName(kPhoneNameOne);
   sync_sessions::SyncedSession* new_session = CreateNewSession(kPhoneNameOne);
   std::vector<const sync_sessions::SyncedSession*> sessions({new_session});
 
@@ -259,11 +288,11 @@ TEST_F(BrowserTabsModelProviderImplTest, ClearTabMetadataDuringMetadataFetch) {
   metadata.push_back(CreateFakeBrowserTabMetadata());
   fake_browser_tabs_metadata_fetcher()->RespondToCurrentFetchAttempt(
       std::move(metadata));
-  EXPECT_TRUE(phone_model_.browser_tabs_model()->most_recent_tabs().empty());
+  EXPECT_TRUE(phone_model()->browser_tabs_model()->most_recent_tabs().empty());
 }
 
 TEST_F(BrowserTabsModelProviderImplTest, SessionCorrectlySelected) {
-  SetPiiFreeName(kPhoneNameOne);
+  SetNewDeviceWithPiiFreeName(kPhoneNameOne);
   sync_sessions::SyncedSession* session_a =
       CreateNewSession(kPhoneNameOne, base::Time::FromDoubleT(1));
   sync_sessions::SyncedSession* session_b =

chrome/browser/chromeos/phonehub/phone_hub_manager_factory.cc

@@ -91,6 +91,7 @@ KeyedService* PhoneHubManagerFactory::BuildServiceInstanceFor(
       multidevice_setup::MultiDeviceSetupClientFactory::GetForProfile(profile),
       secure_channel::SecureChannelClientProvider::GetInstance()->GetClient(),
       std::make_unique<BrowserTabsModelProviderImpl>(
+          device_sync::DeviceSyncClientFactory::GetForProfile(profile),
           multidevice_setup::MultiDeviceSetupClientFactory::GetForProfile(
               profile),
           SessionSyncServiceFactory::GetInstance()->GetForProfile(profile),