[SyncInvalidations] Plumb info through DeviceInfoSyncClient.

Before this CL, LocalDeviceInfoProvider was observing SyncInvalidationsService and caching state internally.
After this CL, it instead queries the same information on demand through DeviceInfoSyncClient.

This change is needed because with the current implementation, it's not possible to resend device info when FCM token or interested data types change.

Bug: 1102336
Change-Id: Ic18f164014b899daa38b1570b2a984805365f384
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2396142Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Paula Vidas <paulavidas@google.com>
Cr-Commit-Position: refs/heads/master@{#804875}

chrome/browser/sync/device_info_sync_service_factory.cc

@@ -24,6 +24,7 @@
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/send_tab_to_self/features.h"
 #include "components/sync/base/sync_prefs.h"
+#include "components/sync/invalidations/switches.h"
 #include "components/sync/invalidations/sync_invalidations_service.h"
 #include "components/sync/model/model_type_store_service.h"
 #include "components/sync_device_info/device_info_prefs.h"
@@ -65,6 +66,26 @@ class DeviceInfoSyncClient : public syncer::DeviceInfoSyncClient {
         profile_->GetPrefs());
   }
 
+  // syncer::DeviceInfoSyncClient:
+  std::string GetFCMRegistrationToken() const override {
+    syncer::SyncInvalidationsService* service =
+        SyncInvalidationsServiceFactory::GetForProfile(profile_);
+    if (service) {
+      return service->GetFCMRegistrationToken();
+    }
+    return std::string();
+  }
+
+  // syncer::DeviceInfoSyncClient:
+  syncer::ModelTypeSet GetInterestedDataTypes() const override {
+    syncer::SyncInvalidationsService* service =
+        SyncInvalidationsServiceFactory::GetForProfile(profile_);
+    if (service) {
+      return service->GetInterestedDataTypes();
+    }
+    return syncer::ModelTypeSet();
+  }
+
  private:
   Profile* const profile_;
 };
@@ -123,8 +144,7 @@ KeyedService* DeviceInfoSyncServiceFactory::BuildServiceInstanceFor(
   auto local_device_info_provider =
       std::make_unique<syncer::LocalDeviceInfoProviderImpl>(
           chrome::GetChannel(), chrome::GetVersionString(),
-          device_info_sync_client.get(),
-          SyncInvalidationsServiceFactory::GetForProfile(profile));
+          device_info_sync_client.get());
 
   auto device_prefs = std::make_unique<syncer::DeviceInfoPrefs>(
       profile->GetPrefs(), base::DefaultClock::GetInstance());

components/sync_device_info/device_info_sync_client.h

@@ -8,6 +8,7 @@
 #include <string>
 #include "base/macros.h"
 #include "base/optional.h"
+#include "components/sync/base/model_type.h"
 #include "components/sync_device_info/device_info.h"
 
 namespace syncer {
@@ -22,6 +23,8 @@ class DeviceInfoSyncClient {
   virtual bool GetSendTabToSelfReceivingEnabled() const = 0;
   virtual base::Optional<DeviceInfo::SharingInfo> GetLocalSharingInfo()
       const = 0;
+  virtual std::string GetFCMRegistrationToken() const = 0;
+  virtual ModelTypeSet GetInterestedDataTypes() const = 0;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(DeviceInfoSyncClient);

components/sync_device_info/local_device_info_provider_impl.cc

@@ -8,7 +8,6 @@
 #include "components/sync/base/sync_prefs.h"
 #include "components/sync/base/sync_util.h"
 #include "components/sync/invalidations/switches.h"
-#include "components/sync/invalidations/sync_invalidations_service.h"
 #include "components/sync_device_info/device_info_sync_client.h"
 #include "components/sync_device_info/device_info_util.h"
 #include "components/sync_device_info/local_device_info_util.h"
@@ -18,25 +17,13 @@ namespace syncer {
 LocalDeviceInfoProviderImpl::LocalDeviceInfoProviderImpl(
     version_info::Channel channel,
     const std::string& version,
-    const DeviceInfoSyncClient* sync_client,
-    SyncInvalidationsService* sync_invalidations_service)
-    : channel_(channel),
-      version_(version),
-      sync_client_(sync_client),
-      sync_invalidations_service_(sync_invalidations_service) {
+    const DeviceInfoSyncClient* sync_client)
+    : channel_(channel), version_(version), sync_client_(sync_client) {
   DCHECK(sync_client);
-  if (sync_invalidations_service_) {
-    sync_invalidations_service_->AddTokenObserver(this);
-    sync_invalidations_service_->AddInterestedDataTypesObserver(this);
-  }
 }
 
 LocalDeviceInfoProviderImpl::~LocalDeviceInfoProviderImpl() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (sync_invalidations_service_) {
-    sync_invalidations_service_->RemoveTokenObserver(this);
-    sync_invalidations_service_->RemoveInterestedDataTypesObserver(this);
-  }
 }
 
 version_info::Channel LocalDeviceInfoProviderImpl::GetChannel() const {
@@ -54,6 +41,14 @@ const DeviceInfo* LocalDeviceInfoProviderImpl::GetLocalDeviceInfo() const {
   local_device_info_->set_send_tab_to_self_receiving_enabled(
       sync_client_->GetSendTabToSelfReceivingEnabled());
   local_device_info_->set_sharing_info(sync_client_->GetLocalSharingInfo());
+
+  if (base::FeatureList::IsEnabled(switches::kSubscribeForSyncInvalidations)) {
+    local_device_info_->set_fcm_registration_token(
+        sync_client_->GetFCMRegistrationToken());
+    local_device_info_->set_interested_data_types(
+        sync_client_->GetInterestedDataTypes());
+  }
+
   return local_device_info_.get();
 }
 
@@ -65,30 +60,6 @@ LocalDeviceInfoProviderImpl::RegisterOnInitializedCallback(
   return callback_list_.Add(callback);
 }
 
-void LocalDeviceInfoProviderImpl::OnFCMRegistrationTokenChanged() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(
-      base::FeatureList::IsEnabled(switches::kSubscribeForSyncInvalidations));
-  DCHECK(sync_invalidations_service_);
-  if (local_device_info_) {
-    local_device_info_->set_fcm_registration_token(
-        sync_invalidations_service_->GetFCMRegistrationToken());
-  }
-  // TODO(crbug.com/1102336): nudge device info update.
-}
-
-void LocalDeviceInfoProviderImpl::OnInterestedDataTypesChanged() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(
-      base::FeatureList::IsEnabled(switches::kSubscribeForSyncInvalidations));
-  DCHECK(sync_invalidations_service_);
-  if (local_device_info_) {
-    local_device_info_->set_interested_data_types(
-        sync_invalidations_service_->GetInterestedDataTypes());
-  }
-  // TODO(crbug.com/1102336): nudge device info update.
-}
-
 void LocalDeviceInfoProviderImpl::Initialize(
     const std::string& cache_guid,
     const std::string& client_name,
@@ -106,8 +77,9 @@ void LocalDeviceInfoProviderImpl::Initialize(
       /*last_updated_timestamp=*/base::Time(),
       DeviceInfoUtil::GetPulseInterval(),
       sync_client_->GetSendTabToSelfReceivingEnabled(),
-      sync_client_->GetLocalSharingInfo(), GetFCMRegistrationToken(),
-      GetInterestedDataTypes());
+      sync_client_->GetLocalSharingInfo(),
+      sync_client_->GetFCMRegistrationToken(),
+      sync_client_->GetInterestedDataTypes());
 
   // Notify observers.
   callback_list_.Notify();
@@ -124,18 +96,4 @@ void LocalDeviceInfoProviderImpl::UpdateClientName(
   local_device_info_->set_client_name(client_name);
 }
 
-std::string LocalDeviceInfoProviderImpl::GetFCMRegistrationToken() const {
-  if (sync_invalidations_service_) {
-    return sync_invalidations_service_->GetFCMRegistrationToken();
-  }
-  return std::string();
-}
-
-ModelTypeSet LocalDeviceInfoProviderImpl::GetInterestedDataTypes() const {
-  if (sync_invalidations_service_) {
-    return sync_invalidations_service_->GetInterestedDataTypes();
-  }
-  return ModelTypeSet();
-}
-
 }  // namespace syncer

components/sync_device_info/local_device_info_provider_impl.h

@@ -13,8 +13,6 @@
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
 #include "components/sync/base/model_type.h"
-#include "components/sync/invalidations/fcm_registration_token_observer.h"
-#include "components/sync/invalidations/interested_data_types_observer.h"
 #include "components/sync_device_info/device_info.h"
 #include "components/sync_device_info/local_device_info_provider.h"
 #include "components/version_info/version_info.h"
@@ -22,20 +20,12 @@
 namespace syncer {
 
 class DeviceInfoSyncClient;
-class SyncInvalidationsService;
 
-class LocalDeviceInfoProviderImpl : public MutableLocalDeviceInfoProvider,
-                                    public syncer::FCMRegistrationTokenObserver,
-                                    public InterestedDataTypesObserver {
+class LocalDeviceInfoProviderImpl : public MutableLocalDeviceInfoProvider {
  public:
-  // |sync_invalidations_service| is used to get an FCM registration token and
-  // interested data types. It may be nullptr if sync invalidations are
-  // disabled.
-  LocalDeviceInfoProviderImpl(
-      version_info::Channel channel,
-      const std::string& version,
-      const DeviceInfoSyncClient* sync_client,
-      SyncInvalidationsService* sync_invalidations_service);
+  LocalDeviceInfoProviderImpl(version_info::Channel channel,
+                              const std::string& version,
+                              const DeviceInfoSyncClient* sync_client);
   ~LocalDeviceInfoProviderImpl() override;
 
   // MutableLocalDeviceInfoProvider implementation.
@@ -50,17 +40,7 @@ class LocalDeviceInfoProviderImpl : public MutableLocalDeviceInfoProvider,
   std::unique_ptr<Subscription> RegisterOnInitializedCallback(
       const base::RepeatingClosure& callback) override;
 
-  // syncer::FCMRegistrationTokenObserver implementation.
-  void OnFCMRegistrationTokenChanged() override;
-
-  // InterestedDataTypesObserver implementation.
-  void OnInterestedDataTypesChanged() override;
-
  private:
-  std::string GetFCMRegistrationToken() const;
-
-  ModelTypeSet GetInterestedDataTypes() const;
-
   // The channel (CANARY, DEV, BETA, etc.) of the current client.
   const version_info::Channel channel_;
 
@@ -68,7 +48,6 @@ class LocalDeviceInfoProviderImpl : public MutableLocalDeviceInfoProvider,
   const std::string version_;
 
   const DeviceInfoSyncClient* const sync_client_;
-  SyncInvalidationsService* sync_invalidations_service_ = nullptr;
 
   std::unique_ptr<DeviceInfo> local_device_info_;
   base::CallbackList<void(void)> callback_list_;

components/sync_device_info/local_device_info_provider_impl_unittest.cc

@@ -8,7 +8,6 @@
 #include "base/test/scoped_feature_list.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/base/sync_util.h"
-#include "components/sync/invalidations/mock_sync_invalidations_service.h"
 #include "components/sync/invalidations/switches.h"
 #include "components/sync_device_info/device_info_sync_client.h"
 #include "components/version_info/version_string.h"
@@ -48,6 +47,8 @@ class MockDeviceInfoSyncClient : public DeviceInfoSyncClient {
   MOCK_CONST_METHOD0(GetSendTabToSelfReceivingEnabled, bool());
   MOCK_CONST_METHOD0(GetLocalSharingInfo,
                      base::Optional<DeviceInfo::SharingInfo>());
+  MOCK_CONST_METHOD0(GetFCMRegistrationToken, std::string());
+  MOCK_CONST_METHOD0(GetInterestedDataTypes, ModelTypeSet());
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockDeviceInfoSyncClient);
@@ -62,16 +63,12 @@ class LocalDeviceInfoProviderImplTest : public testing::Test {
     provider_ = std::make_unique<LocalDeviceInfoProviderImpl>(
         version_info::Channel::UNKNOWN,
         version_info::GetVersionStringWithModifier("UNKNOWN"),
-        &device_info_sync_client_, GetSyncInvalidationsService());
+        &device_info_sync_client_);
   }
 
   void TearDown() override { provider_.reset(); }
 
  protected:
-  virtual SyncInvalidationsService* GetSyncInvalidationsService() {
-    return nullptr;
-  }
-
   void InitializeProvider() { InitializeProvider(kLocalDeviceGuid); }
 
   void InitializeProvider(const std::string& guid) {
@@ -89,22 +86,10 @@ class LocalDeviceInfoProviderImplWithSyncInvalidationsTest
   LocalDeviceInfoProviderImplWithSyncInvalidationsTest() {
     override_features_.InitAndEnableFeature(
         switches::kSubscribeForSyncInvalidations);
-    ON_CALL(mock_sync_invalidations_service_, GetFCMRegistrationToken())
-        .WillByDefault(ReturnRef(kEmptyToken));
-    ON_CALL(mock_sync_invalidations_service_, GetInterestedDataTypes())
-        .WillByDefault(ReturnRef(kEmptyTypesSet));
   }
 
  protected:
-  SyncInvalidationsService* GetSyncInvalidationsService() override {
-    return &mock_sync_invalidations_service_;
-  }
-
-  const std::string kEmptyToken;
-  const ModelTypeSet kEmptyTypesSet;
-
   base::test::ScopedFeatureList override_features_;
-  NiceMock<MockSyncInvalidationsService> mock_sync_invalidations_service_;
 };
 
 TEST_F(LocalDeviceInfoProviderImplTest, GetLocalDeviceInfo) {
@@ -206,10 +191,9 @@ TEST_F(LocalDeviceInfoProviderImplWithSyncInvalidationsTest,
       provider_->GetLocalDeviceInfo()->fcm_registration_token().empty());
 
   const std::string kFCMRegistrationToken = "token";
-  EXPECT_CALL(mock_sync_invalidations_service_, GetFCMRegistrationToken())
-      .WillOnce(ReturnRef(kFCMRegistrationToken));
+  EXPECT_CALL(device_info_sync_client_, GetFCMRegistrationToken())
+      .WillOnce(Return(kFCMRegistrationToken));
 
-  provider_->OnFCMRegistrationTokenChanged();
   EXPECT_EQ(provider_->GetLocalDeviceInfo()->fcm_registration_token(),
             kFCMRegistrationToken);
 }
@@ -221,10 +205,9 @@ TEST_F(LocalDeviceInfoProviderImplWithSyncInvalidationsTest,
   EXPECT_TRUE(provider_->GetLocalDeviceInfo()->interested_data_types().Empty());
 
   const ModelTypeSet kTypes = ModelTypeSet(BOOKMARKS);
-  EXPECT_CALL(mock_sync_invalidations_service_, GetInterestedDataTypes())
-      .WillOnce(ReturnRef(kTypes));
+  EXPECT_CALL(device_info_sync_client_, GetInterestedDataTypes())
+      .WillOnce(Return(kTypes));
 
-  provider_->OnInterestedDataTypesChanged();
   EXPECT_EQ(provider_->GetLocalDeviceInfo()->interested_data_types(), kTypes);
 }
 

ios/chrome/browser/sync/device_info_sync_service_factory.mm

@@ -51,6 +51,14 @@ class DeviceInfoSyncClient : public syncer::DeviceInfoSyncClient {
     return base::nullopt;
   }
 
+  // syncer::DeviceInfoSyncClient:
+  std::string GetFCMRegistrationToken() const override { return std::string(); }
+
+  // syncer::DeviceInfoSyncClient:
+  syncer::ModelTypeSet GetInterestedDataTypes() const override {
+    return syncer::ModelTypeSet();
+  }
+
  private:
   PrefService* const prefs_;
 };
@@ -109,8 +117,7 @@ DeviceInfoSyncServiceFactory::BuildServiceInstanceFor(
       std::make_unique<DeviceInfoSyncClient>(browser_state->GetPrefs());
   auto local_device_info_provider =
       std::make_unique<syncer::LocalDeviceInfoProviderImpl>(
-          ::GetChannel(), ::GetVersionString(), device_info_sync_client.get(),
-          /*sync_invalidations_service=*/nullptr);
+          ::GetChannel(), ::GetVersionString(), device_info_sync_client.get());
   auto device_prefs = std::make_unique<syncer::DeviceInfoPrefs>(
       browser_state->GetPrefs(), base::DefaultClock::GetInstance());
 

ios/web_view/internal/sync/web_view_device_info_sync_service_factory.mm

@@ -45,6 +45,14 @@ class DeviceInfoSyncClient : public syncer::DeviceInfoSyncClient {
     return base::nullopt;
   }
 
+  // syncer::DeviceInfoSyncClient:
+  std::string GetFCMRegistrationToken() const override { return std::string(); }
+
+  // syncer::DeviceInfoSyncClient:
+  syncer::ModelTypeSet GetInterestedDataTypes() const override {
+    return syncer::ModelTypeSet();
+  }
+
  private:
   PrefService* const prefs_;
 };
@@ -87,8 +95,7 @@ WebViewDeviceInfoSyncServiceFactory::BuildServiceInstanceFor(
   auto local_device_info_provider =
       std::make_unique<syncer::LocalDeviceInfoProviderImpl>(
           version_info::Channel::STABLE, version_info::GetVersionNumber(),
-          device_info_sync_client.get(),
-          /*sync_invalidations_service=*/nullptr);
+          device_info_sync_client.get());
   auto device_prefs = std::make_unique<syncer::DeviceInfoPrefs>(
       browser_state->GetPrefs(), base::DefaultClock::GetInstance());