[Sync] Fix sending interested data types

The interested data type list will be sent on each change. Before this
CL, if sending of interested data types was just enabled, then the new
field containing the list wouldn't have been sent to the server after
the next browser startup.

Bug: 1155030
Change-Id: I50ffb01b8592432128f3929abd554d874023d47e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2598908
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843087}

chrome/browser/sync/test/integration/single_client_sync_invalidations_test.cc

@@ -19,6 +19,7 @@
 #include "components/sync/test/fake_server/entity_builder_factory.h"
 #include "components/sync_device_info/device_info_util.h"
 #include "content/public/test/browser_test.h"
+#include "content/public/test/test_launcher.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -36,6 +37,10 @@ using testing::UnorderedElementsAre;
 
 const char kSyncedBookmarkURL[] = "http://www.mybookmark.com";
 
+MATCHER_P(HasCacheGuid, expected_cache_guid, "") {
+  return arg.specifics().device_info().cache_guid() == expected_cache_guid;
+}
+
 MATCHER_P(InterestedDataTypesAre, expected_data_types, "") {
   syncer::ModelTypeSet data_types;
   for (const int field_number : arg.specifics()
@@ -384,4 +389,47 @@ IN_PROC_BROWSER_TEST_F(
 }
 #endif  // !OS_CHROMEOS
 
+class SingleClientSyncInvalidationsTestWithPreDisabledSendInterestedDataTypes
+    : public SyncTest {
+ public:
+  SingleClientSyncInvalidationsTestWithPreDisabledSendInterestedDataTypes()
+      : SyncTest(SINGLE_CLIENT) {
+    feature_list_.InitWithFeatureState(switches::kSyncSendInterestedDataTypes,
+                                       !content::IsPreTest());
+  }
+
+  std::string GetLocalCacheGuid() {
+    syncer::SyncPrefs prefs(GetProfile(0)->GetPrefs());
+    return prefs.GetCacheGuid();
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(
+    SingleClientSyncInvalidationsTestWithPreDisabledSendInterestedDataTypes,
+    PRE_ShouldResendDeviceInfoWithInterestedDataTypes) {
+  ASSERT_TRUE(SetupSync());
+  ASSERT_TRUE(ServerDeviceInfoMatchChecker(
+                  GetFakeServer(),
+                  UnorderedElementsAre(HasCacheGuid(GetLocalCacheGuid())))
+                  .Wait());
+}
+
+IN_PROC_BROWSER_TEST_F(
+    SingleClientSyncInvalidationsTestWithPreDisabledSendInterestedDataTypes,
+    ShouldResendDeviceInfoWithInterestedDataTypes) {
+  ASSERT_TRUE(SetupClients());
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+  // signin::SetRefreshTokenForPrimaryAccount() is needed on ChromeOS in order
+  // to get a non-empty refresh token on startup.
+  GetClient(0)->SignInPrimaryAccount();
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
+  ASSERT_TRUE(GetClient(0)->AwaitEngineInitialization());
+  ASSERT_TRUE(GetClient(0)->AwaitSyncSetupCompletion());
+
+  EXPECT_TRUE(ServerDeviceInfoMatchChecker(
+                  GetFakeServer(),
+                  ElementsAre(InterestedDataTypesContain(syncer::NIGORI)))
+                  .Wait());
+}
+
 }  // namespace

components/sync/invalidations/interested_data_types_manager.cc

@@ -24,24 +24,21 @@ void InterestedDataTypesManager::SetInterestedDataTypesHandler(
   interested_data_types_handler_ = handler;
 }
 
-const ModelTypeSet& InterestedDataTypesManager::GetInterestedDataTypes() const {
+base::Optional<ModelTypeSet>
+InterestedDataTypesManager::GetInterestedDataTypes() const {
   return data_types_;
 }
 
 void InterestedDataTypesManager::SetInterestedDataTypes(
     const ModelTypeSet& data_types,
     SyncInvalidationsService::InterestedDataTypesAppliedCallback callback) {
-  ModelTypeSet new_data_types = Difference(data_types, data_types_);
+  ModelTypeSet new_data_types =
+      Difference(data_types, data_types_.value_or(ModelTypeSet()));
   data_types_ = data_types;
   if (interested_data_types_handler_) {
     interested_data_types_handler_->OnInterestedDataTypesChanged(
         base::BindOnce(std::move(callback), new_data_types));
   }
-  initialized_ = true;
-}
-
-bool InterestedDataTypesManager::IsInitialized() const {
-  return initialized_;
 }
 
 }  // namespace syncer

components/sync/invalidations/interested_data_types_manager.h

@@ -5,6 +5,8 @@
 #ifndef COMPONENTS_SYNC_INVALIDATIONS_INTERESTED_DATA_TYPES_MANAGER_H_
 #define COMPONENTS_SYNC_INVALIDATIONS_INTERESTED_DATA_TYPES_MANAGER_H_
 
+#include "base/optional.h"
+
 #include "components/sync/base/model_type.h"
 #include "components/sync/invalidations/sync_invalidations_service.h"
 
@@ -24,8 +26,9 @@ class InterestedDataTypesManager {
   // unregister any existing handler. There can be at most one handler.
   void SetInterestedDataTypesHandler(InterestedDataTypesHandler* handler);
 
-  // Get the interested data types.
-  const ModelTypeSet& GetInterestedDataTypes() const;
+  // Get the interested data types. Returns nullopt if SetInterestedDataTypes()
+  // has never been called.
+  base::Optional<ModelTypeSet> GetInterestedDataTypes() const;
 
   // Set interested data types. The first call of the method initializes this
   // object.
@@ -33,16 +36,10 @@ class InterestedDataTypesManager {
       const ModelTypeSet& data_types,
       SyncInvalidationsService::InterestedDataTypesAppliedCallback callback);
 
-  // Returns true if SetInterestedDataTypes() has been called at least once.
-  // Before that this object is considered to be uninitialized.
-  bool IsInitialized() const;
-
  private:
   InterestedDataTypesHandler* interested_data_types_handler_ = nullptr;
 
-  bool initialized_ = false;
-
-  ModelTypeSet data_types_;
+  base::Optional<ModelTypeSet> data_types_;
 };
 
 }  // namespace syncer

components/sync/invalidations/interested_data_types_manager_unittest.cc

@@ -49,13 +49,13 @@ TEST_F(InterestedDataTypesManagerTest, ShouldNotifyOnChange) {
 
 TEST_F(InterestedDataTypesManagerTest,
        ShouldInitializeOnFirstSetInterestedDataTypes) {
-  EXPECT_FALSE(manager_.IsInitialized());
+  EXPECT_FALSE(manager_.GetInterestedDataTypes().has_value());
   manager_.SetInterestedDataTypes(ModelTypeSet(BOOKMARKS, PREFERENCES),
                                   base::DoNothing());
-  EXPECT_TRUE(manager_.IsInitialized());
+  EXPECT_TRUE(manager_.GetInterestedDataTypes().has_value());
   manager_.SetInterestedDataTypes(ModelTypeSet(BOOKMARKS, PREFERENCES, NIGORI),
                                   base::DoNothing());
-  EXPECT_TRUE(manager_.IsInitialized());
+  EXPECT_TRUE(manager_.GetInterestedDataTypes().has_value());
 }
 
 }  // namespace

components/sync/invalidations/sync_invalidations_service_impl.cc

@@ -76,10 +76,7 @@ void SyncInvalidationsServiceImpl::SetInterestedDataTypesHandler(
 
 base::Optional<ModelTypeSet>
 SyncInvalidationsServiceImpl::GetInterestedDataTypes() const {
-  if (data_types_manager_.IsInitialized()) {
-    return data_types_manager_.GetInterestedDataTypes();
-  }
-  return base::nullopt;
+  return data_types_manager_.GetInterestedDataTypes();
 }
 
 void SyncInvalidationsServiceImpl::SetInterestedDataTypes(