[SyncInvalidations] Introduce new Sync Invalidations feature flags.

SubscribeForSyncInvalidations flag is removed, and mostly replaced by
the new flag SyncSendInterestedDataTypes. If that flag is enabled,
SyncInvalidationsService is created, and the set of interested data
types is sent to the server (as part of DeviceInfo).

New flag UseSyncInvalidations is introduced. If both
SyncSendInterestedDataTypes and this flag are enabled, FCMHandler can
register with FCM and listen to new invalidations. Also, FCM token will
be included in DeviceInfo.

Unsubscribing from old invalidations behind these flags will be added
in a separate CL.

Bug: 1082115
Change-Id: I66eb2f254473d2f897cc15f054dcd6dafdc76708
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2429008Reviewed-by: Rushan Suleymanov <rushans@google.com>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Paula Vidas <paulavidas@google.com>
Cr-Commit-Position: refs/heads/master@{#811150}

chrome/browser/sync/sync_invalidations_service_factory.cc

@@ -37,7 +37,7 @@ SyncInvalidationsServiceFactory::~SyncInvalidationsServiceFactory() = default;
 
 KeyedService* SyncInvalidationsServiceFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
-  if (!base::FeatureList::IsEnabled(switches::kSubscribeForSyncInvalidations)) {
+  if (!base::FeatureList::IsEnabled(switches::kSyncSendInterestedDataTypes)) {
     return nullptr;
   }
 

components/sync/driver/glue/sync_engine_backend.cc

@@ -588,8 +588,8 @@ void SyncEngineBackend::DoOnInvalidatorClientIdChange(
 
 void SyncEngineBackend::DoOnInvalidationReceived(const std::string& payload) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(
-      base::FeatureList::IsEnabled(switches::kSubscribeForSyncInvalidations));
+  DCHECK(base::FeatureList::IsEnabled(switches::kSyncSendInterestedDataTypes) &&
+         base::FeatureList::IsEnabled(switches::kUseSyncInvalidations));
 
   sync_pb::SyncInvalidationsPayload payload_message;
   // TODO(crbug.com/1119804): Track parsing failures in a histogram.

components/sync/driver/glue/sync_engine_impl_unittest.cc

@@ -325,8 +325,10 @@ class SyncEngineImplTest : public testing::Test {
 class SyncEngineImplWithSyncInvalidationsTest : public SyncEngineImplTest {
  public:
   SyncEngineImplWithSyncInvalidationsTest() {
-    override_features_.InitAndEnableFeature(
-        switches::kSubscribeForSyncInvalidations);
+    override_features_.InitWithFeatures(
+        /*enabled_features=*/{switches::kSyncSendInterestedDataTypes,
+                              switches::kUseSyncInvalidations},
+        /*disabled_features=*/{});
   }
 
  protected:

components/sync/driver/profile_sync_service_bundle.h

@@ -64,8 +64,7 @@ class ProfileSyncServiceBundle {
   }
 
   MockSyncInvalidationsService* sync_invalidations_service() {
-    if (base::FeatureList::IsEnabled(
-            switches::kSubscribeForSyncInvalidations)) {
+    if (base::FeatureList::IsEnabled(switches::kSyncSendInterestedDataTypes)) {
       return &sync_invalidations_service_;
     } else {
       return nullptr;

components/sync/driver/profile_sync_service_unittest.cc

@@ -360,15 +360,16 @@ class ProfileSyncServiceTestWithStopSyncInPausedState
   base::test::ScopedFeatureList override_features_;
 };
 
-class ProfileSyncServiceTestWithSubscribeForSyncInvalidations
+class ProfileSyncServiceTestWithSyncInvalidationsServiceCreated
     : public ProfileSyncServiceTest {
  public:
-  ProfileSyncServiceTestWithSubscribeForSyncInvalidations() {
+  ProfileSyncServiceTestWithSyncInvalidationsServiceCreated() {
     override_features_.InitAndEnableFeature(
-        switches::kSubscribeForSyncInvalidations);
+        switches::kSyncSendInterestedDataTypes);
   }
 
-  ~ProfileSyncServiceTestWithSubscribeForSyncInvalidations() override = default;
+  ~ProfileSyncServiceTestWithSyncInvalidationsServiceCreated() override =
+      default;
 
  private:
   base::test::ScopedFeatureList override_features_;
@@ -1621,7 +1622,7 @@ TEST_F(ProfileSyncServiceTest,
   EXPECT_EQ(signin::GetTestGaiaIdForEmail(kTestUser), sync_prefs.GetGaiaId());
 }
 
-TEST_F(ProfileSyncServiceTestWithSubscribeForSyncInvalidations,
+TEST_F(ProfileSyncServiceTestWithSyncInvalidationsServiceCreated,
        ShouldSendDataTypesToSyncInvalidationsService) {
   CreateService(ProfileSyncService::AUTO_START);
   SignIn();
@@ -1633,7 +1634,7 @@ MATCHER(ContainsSessions, "") {
   return arg.Has(SESSIONS);
 }
 
-TEST_F(ProfileSyncServiceTestWithSubscribeForSyncInvalidations,
+TEST_F(ProfileSyncServiceTestWithSyncInvalidationsServiceCreated,
        ShouldEnableAndDisableInvalidationsForSessions) {
   CreateService(ProfileSyncService::AUTO_START,
                 ModelTypeSet(SESSIONS, TYPED_URLS));
@@ -1648,7 +1649,7 @@ TEST_F(ProfileSyncServiceTestWithSubscribeForSyncInvalidations,
   service()->SetInvalidationsForSessionsEnabled(false);
 }
 
-TEST_F(ProfileSyncServiceTestWithSubscribeForSyncInvalidations,
+TEST_F(ProfileSyncServiceTestWithSyncInvalidationsServiceCreated,
        ShouldActivateSyncInvalidationsServiceWhenSyncIsInitialized) {
   CreateService(ProfileSyncService::AUTO_START);
   EXPECT_CALL(*sync_invalidations_service(), SetActive(true)).Times(0);
@@ -1657,7 +1658,7 @@ TEST_F(ProfileSyncServiceTestWithSubscribeForSyncInvalidations,
   InitializeForFirstSync();
 }
 
-TEST_F(ProfileSyncServiceTestWithSubscribeForSyncInvalidations,
+TEST_F(ProfileSyncServiceTestWithSyncInvalidationsServiceCreated,
        ShouldActivateSyncInvalidationsServiceOnSignIn) {
   CreateService(ProfileSyncService::AUTO_START);
   EXPECT_CALL(*sync_invalidations_service(), SetActive(false));
@@ -1668,7 +1669,7 @@ TEST_F(ProfileSyncServiceTestWithSubscribeForSyncInvalidations,
 
 // CrOS does not support signout.
 #if !defined(OS_CHROMEOS)
-TEST_F(ProfileSyncServiceTestWithSubscribeForSyncInvalidations,
+TEST_F(ProfileSyncServiceTestWithSyncInvalidationsServiceCreated,
        ShouldDectivateSyncInvalidationsServiceOnSignOut) {
   CreateService(ProfileSyncService::AUTO_START);
   SignIn();

components/sync/invalidations/fcm_handler.cc

@@ -14,6 +14,7 @@
 #include "components/gcm_driver/instance_id/instance_id_driver.h"
 #include "components/sync/invalidations/fcm_registration_token_observer.h"
 #include "components/sync/invalidations/invalidations_listener.h"
+#include "components/sync/invalidations/switches.h"
 
 namespace syncer {
 
@@ -39,6 +40,7 @@ FCMHandler::~FCMHandler() {
 void FCMHandler::StartListening() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(!IsListening());
+  DCHECK(base::FeatureList::IsEnabled(switches::kUseSyncInvalidations));
   gcm_driver_->AddAppHandler(app_id_, this);
   StartTokenFetch(base::BindOnce(&FCMHandler::DidRetrieveToken,
                                  weak_ptr_factory_.GetWeakPtr()));
@@ -101,6 +103,7 @@ void FCMHandler::OnMessage(const std::string& app_id,
                            const gcm::IncomingMessage& message) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK_EQ(app_id, app_id_);
+  DCHECK(base::FeatureList::IsEnabled(switches::kUseSyncInvalidations));
 
   auto it = message.data.find(kPayloadKey);
   std::string payload;

components/sync/invalidations/fcm_handler_unittest.cc

@@ -11,12 +11,14 @@
 #include <vector>
 
 #include "base/files/file_path.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "components/gcm_driver/fake_gcm_driver.h"
 #include "components/gcm_driver/gcm_driver.h"
 #include "components/gcm_driver/instance_id/instance_id_driver.h"
 #include "components/sync/invalidations/fcm_registration_token_observer.h"
 #include "components/sync/invalidations/invalidations_listener.h"
+#include "components/sync/invalidations/switches.h"
 #include "google_apis/gcm/engine/account_mapping.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -94,11 +96,16 @@ class FCMHandlerTest : public testing::Test {
     // This is called in the FCMHandler.
     ON_CALL(mock_instance_id_driver_, GetInstanceID(kSyncInvalidationsAppId))
         .WillByDefault(Return(&mock_instance_id_));
+    override_features_.InitWithFeatures(
+        /*enabled_features=*/{switches::kSyncSendInterestedDataTypes,
+                              switches::kUseSyncInvalidations},
+        /*disabled_features=*/{});
   }
 
  protected:
   base::test::SingleThreadTaskEnvironment task_environment_{
       base::test::TaskEnvironment::TimeSource::MOCK_TIME};
+  base::test::ScopedFeatureList override_features_;
 
   gcm::FakeGCMDriver fake_gcm_driver_;
   NiceMock<MockInstanceIDDriver> mock_instance_id_driver_;

components/sync/invalidations/switches.cc

@@ -6,7 +6,10 @@
 
 namespace switches {
 
-const base::Feature kSubscribeForSyncInvalidations = {
-    "SubscribeForSyncInvalidations", base::FEATURE_DISABLED_BY_DEFAULT};
+const base::Feature kSyncSendInterestedDataTypes = {
+    "SyncSendInterestedDataTypes", base::FEATURE_DISABLED_BY_DEFAULT};
+
+const base::Feature kUseSyncInvalidations = {"UseSyncInvalidations",
+                                             base::FEATURE_DISABLED_BY_DEFAULT};
 
 }  // namespace switches

components/sync/invalidations/switches.h

@@ -9,7 +9,15 @@
 
 namespace switches {
 
-extern const base::Feature kSubscribeForSyncInvalidations;
+// If enabled, interested data types will be sent to the Sync Server as part of
+// DeviceInfo.
+extern const base::Feature kSyncSendInterestedDataTypes;
+
+// If enabled, the device will register with FCM and listen to new
+// invalidations. Also, FCM token will be set in DeviceInfo, which signals to
+// the server that device listens to new invalidations.
+// SyncSendInterestedDataTypes must be enabled for this to take effect.
+extern const base::Feature kUseSyncInvalidations;
 
 }  // namespace switches
 

components/sync/invalidations/sync_invalidations_service_impl.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "components/sync/invalidations/fcm_handler.h"
+#include "components/sync/invalidations/switches.h"
 
 namespace syncer {
 
@@ -28,7 +29,8 @@ SyncInvalidationsServiceImpl::SyncInvalidationsServiceImpl(
 SyncInvalidationsServiceImpl::~SyncInvalidationsServiceImpl() = default;
 
 void SyncInvalidationsServiceImpl::SetActive(bool active) {
-  if (fcm_handler_->IsListening() == active) {
+  if (!base::FeatureList::IsEnabled(switches::kUseSyncInvalidations) ||
+      fcm_handler_->IsListening() == active) {
     return;
   }
 

components/sync_device_info/device_info_sync_bridge_unittest.cc

@@ -19,7 +19,6 @@
 #include "components/prefs/testing_pref_service.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/base/time.h"
-#include "components/sync/invalidations/switches.h"
 #include "components/sync/model/data_batch.h"
 #include "components/sync/model/data_type_activation_request.h"
 #include "components/sync/model/data_type_error_handler_mock.h"
@@ -234,17 +233,11 @@ sync_pb::SharingSpecificFields::EnabledFeatures SharingEnabledFeaturesForSuffix(
 }
 
 std::string SyncInvalidationsInstanceIdTokenForSuffix(int suffix) {
-  if (base::FeatureList::IsEnabled(switches::kSubscribeForSyncInvalidations)) {
-    return base::StringPrintf("instance id token %d", suffix);
-  }
-  return std::string();
+  return base::StringPrintf("instance id token %d", suffix);
 }
 
 ModelTypeSet SyncInvalidationsInterestedDataTypes() {
-  if (base::FeatureList::IsEnabled(switches::kSubscribeForSyncInvalidations)) {
-    return ModelTypeSet(BOOKMARKS);
-  }
-  return ModelTypeSet();
+  return ModelTypeSet(BOOKMARKS);
 }
 
 DataTypeActivationRequest TestDataTypeActivationRequest(SyncMode sync_mode) {
@@ -284,12 +277,8 @@ DeviceInfoSpecifics CreateSpecifics(
   specifics.mutable_sharing_fields()->add_enabled_features(
       SharingEnabledFeaturesForSuffix(suffix));
 
-  const std::string sync_invalidations_instance_id_token =
-      SyncInvalidationsInstanceIdTokenForSuffix(suffix);
-  if (!sync_invalidations_instance_id_token.empty()) {
-    specifics.mutable_invalidation_fields()->set_instance_id_token(
-        sync_invalidations_instance_id_token);
-  }
+  specifics.mutable_invalidation_fields()->set_instance_id_token(
+      SyncInvalidationsInstanceIdTokenForSuffix(suffix));
   for (const ModelType type : SyncInvalidationsInterestedDataTypes()) {
     specifics.mutable_invalidation_fields()->add_interested_data_type_ids(
         GetSpecificsFieldNumberFromModelType(type));
@@ -1298,10 +1287,6 @@ TEST_F(DeviceInfoSyncBridgeTest, RefreshLocalDeviceNameForSyncModeToggle) {
 }
 
 TEST_F(DeviceInfoSyncBridgeTest, ShouldSendInvalidationFields) {
-  base::test::ScopedFeatureList override_features;
-  override_features.InitAndEnableFeature(
-      switches::kSubscribeForSyncInvalidations);
-
   EXPECT_CALL(*processor(),
               Put(_,
                   HasSpecifics(

components/sync_device_info/local_device_info_provider_impl.cc

@@ -7,7 +7,6 @@
 #include "base/bind.h"
 #include "components/sync/base/sync_prefs.h"
 #include "components/sync/base/sync_util.h"
-#include "components/sync/invalidations/switches.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"
@@ -41,13 +40,10 @@ 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());
-  }
+  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();
 }

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/switches.h"
 #include "components/sync_device_info/device_info_sync_client.h"
 #include "components/version_info/version_string.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -80,18 +79,6 @@ class LocalDeviceInfoProviderImplTest : public testing::Test {
   std::unique_ptr<LocalDeviceInfoProviderImpl> provider_;
 };
 
-class LocalDeviceInfoProviderImplWithSyncInvalidationsTest
-    : public LocalDeviceInfoProviderImplTest {
- public:
-  LocalDeviceInfoProviderImplWithSyncInvalidationsTest() {
-    override_features_.InitAndEnableFeature(
-        switches::kSubscribeForSyncInvalidations);
-  }
-
- protected:
-  base::test::ScopedFeatureList override_features_;
-};
-
 TEST_F(LocalDeviceInfoProviderImplTest, GetLocalDeviceInfo) {
   ASSERT_EQ(nullptr, provider_->GetLocalDeviceInfo());
 
@@ -183,8 +170,7 @@ TEST_F(LocalDeviceInfoProviderImplTest, SharingInfo) {
   EXPECT_EQ(enabled_features, local_sharing_info->enabled_features);
 }
 
-TEST_F(LocalDeviceInfoProviderImplWithSyncInvalidationsTest,
-       ShouldPopulateFCMRegistrationToken) {
+TEST_F(LocalDeviceInfoProviderImplTest, ShouldPopulateFCMRegistrationToken) {
   InitializeProvider();
   ASSERT_THAT(provider_->GetLocalDeviceInfo(), NotNull());
   EXPECT_TRUE(
@@ -198,8 +184,7 @@ TEST_F(LocalDeviceInfoProviderImplWithSyncInvalidationsTest,
             kFCMRegistrationToken);
 }
 
-TEST_F(LocalDeviceInfoProviderImplWithSyncInvalidationsTest,
-       ShouldPopulateInterestedDataTypes) {
+TEST_F(LocalDeviceInfoProviderImplTest, ShouldPopulateInterestedDataTypes) {
   InitializeProvider();
   ASSERT_THAT(provider_->GetLocalDeviceInfo(), NotNull());
   EXPECT_TRUE(provider_->GetLocalDeviceInfo()->interested_data_types().Empty());

ios/chrome/browser/sync/sync_invalidations_service_factory.mm

@@ -45,7 +45,7 @@ SyncInvalidationsServiceFactory::~SyncInvalidationsServiceFactory() = default;
 std::unique_ptr<KeyedService>
 SyncInvalidationsServiceFactory::BuildServiceInstanceFor(
     web::BrowserState* context) const {
-  if (!base::FeatureList::IsEnabled(switches::kSubscribeForSyncInvalidations)) {
+  if (!base::FeatureList::IsEnabled(switches::kSyncSendInterestedDataTypes)) {
     return nullptr;
   }
 

ios/web_view/internal/sync/web_view_sync_invalidations_service_factory.mm

@@ -48,7 +48,7 @@ WebViewSyncInvalidationsServiceFactory::
 std::unique_ptr<KeyedService>
 WebViewSyncInvalidationsServiceFactory::BuildServiceInstanceFor(
     web::BrowserState* context) const {
-  if (!base::FeatureList::IsEnabled(switches::kSubscribeForSyncInvalidations)) {
+  if (!base::FeatureList::IsEnabled(switches::kSyncSendInterestedDataTypes)) {
     return nullptr;
   }