[SyncInvalidations] Unregister from old invalidations.

If UseSyncInvalidations feature flag is enabled, we don't subscribe to
old invalidations for any data types besides Wallet and Offer.
And if UseSyncInvalidationsForWalletAndOffer is enabled, we don't
register for old invalidations at all.

Bug: 1082115, 1050970
Change-Id: Ifbda4e29c4ed22a1469f1482ba7be8f8cccbd881
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2435224
Commit-Queue: Paula Vidas <paulavidas@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Rushan Suleymanov <rushans@google.com>
Cr-Commit-Position: refs/heads/master@{#811657}

components/sync/driver/glue/sync_engine_impl.cc

@@ -31,6 +31,7 @@
 #include "components/sync/engine/sync_manager_factory.h"
 #include "components/sync/engine/sync_string_conversions.h"
 #include "components/sync/invalidations/fcm_handler.h"
+#include "components/sync/invalidations/switches.h"
 #include "components/sync/invalidations/sync_invalidations_service.h"
 
 namespace syncer {
@@ -44,9 +45,29 @@ SyncEngineImpl::SyncEngineImpl(
     : name_(name),
       sync_prefs_(sync_prefs),
       invalidator_(invalidator),
-      sync_invalidations_service_(sync_invalidations_service) {
+      sync_invalidations_service_(sync_invalidations_service),
+#if defined(OS_ANDROID)
+      sessions_invalidation_enabled_(false) {
+#else
+      sessions_invalidation_enabled_(true) {
+#endif
   backend_ = base::MakeRefCounted<SyncEngineBackend>(
       name_, sync_data_folder, weak_ptr_factory_.GetWeakPtr());
+
+  // If the new invalidations system (SyncInvalidationsService) is fully
+  // enabled, then the SyncService doesn't need to communicate with the old
+  // InvalidationService anymore.
+  if (invalidator_ &&
+      base::FeatureList::IsEnabled(switches::kSyncSendInterestedDataTypes) &&
+      base::FeatureList::IsEnabled(switches::kUseSyncInvalidations) &&
+      base::FeatureList::IsEnabled(
+          switches::kUseSyncInvalidationsForWalletAndOffer)) {
+    invalidator_->RegisterInvalidationHandler(this);
+    bool success = invalidator_->UpdateInterestedTopics(this, /*topics=*/{});
+    DCHECK(success);
+    invalidator_->UnregisterInvalidationHandler(this);
+    invalidator_ = nullptr;
+  }
 }
 
 SyncEngineImpl::~SyncEngineImpl() {
@@ -285,19 +306,7 @@ void SyncEngineImpl::FinishConfigureDataTypesOnFrontendLoop(
     const ModelTypeSet failed_configuration_types,
     base::OnceCallback<void(ModelTypeSet, ModelTypeSet)> ready_task) {
   last_enabled_types_ = enabled_types;
-  if (invalidator_) {
-    // No need to register invalidations for CommitOnlyTypes().
-    ModelTypeSet invalidation_enabled_types(
-        Difference(enabled_types, CommitOnlyTypes()));
-#if defined(OS_ANDROID)
-    if (!sessions_invalidation_enabled_) {
-      invalidation_enabled_types.Remove(syncer::SESSIONS);
-    }
-#endif
-    bool success = invalidator_->UpdateInterestedTopics(
-        this, ModelTypeSetToTopicSet(invalidation_enabled_types));
-    DCHECK(success);
-  }
+  SendInterestedTopicsToInvalidator();
 
   if (!ready_task.is_null()) {
     std::move(ready_task)
@@ -324,6 +333,7 @@ void SyncEngineImpl::HandleInitializationSuccessOnFrontendLoop(
 
     // Fake a state change to initialize the SyncManager's cached invalidator
     // state.
+    // TODO(crbug.com/1132868): Do this for the new invalidations as well.
     OnInvalidatorStateChange(invalidator_->GetInvalidatorState());
   }
 
@@ -445,20 +455,7 @@ void SyncEngineImpl::OnCookieJarChanged(bool account_mismatch,
 
 void SyncEngineImpl::SetInvalidationsForSessionsEnabled(bool enabled) {
   sessions_invalidation_enabled_ = enabled;
-  // TODO(crbug.com/1050970): unify logic here and in
-  // FinishConfigureDataTypesOnFrontedLoop() and factor it out.
-  // |last_enabled_types_| contains all datatypes, for which user
-  // has enabled Sync. There is no need to register invalidations for
-  // CommitOnlyTypes(), so they are filtered out.
-  ModelTypeSet enabled_for_invalidation(
-      Difference(last_enabled_types_, CommitOnlyTypes()));
-  if (!enabled) {
-    enabled_for_invalidation.Remove(syncer::SESSIONS);
-  }
-  bool success = invalidator_->UpdateInterestedTopics(
-      this, ModelTypeSetToTopicSet(enabled_for_invalidation));
-  DCHECK(success);
-  // TODO(crbug.com/1102312): update enabled data types for sync invalidations.
+  SendInterestedTopicsToInvalidator();
 }
 
 void SyncEngineImpl::GetNigoriNodeForDebugging(AllNodesCallback callback) {
@@ -490,4 +487,28 @@ void SyncEngineImpl::OnCookieJarChangedDoneOnFrontendLoop(
   std::move(callback).Run();
 }
 
+void SyncEngineImpl::SendInterestedTopicsToInvalidator() {
+  if (!invalidator_) {
+    return;
+  }
+
+  // No need to register invalidations for CommitOnlyTypes().
+  ModelTypeSet invalidation_enabled_types(
+      Difference(last_enabled_types_, CommitOnlyTypes()));
+  if (!sessions_invalidation_enabled_) {
+    invalidation_enabled_types.Remove(syncer::SESSIONS);
+  }
+  // switches::kUseSyncInvalidations means that the new invalidations system is
+  // used for all data types except Wallet and Offer, so only keep these types.
+  if (base::FeatureList::IsEnabled(switches::kSyncSendInterestedDataTypes) &&
+      base::FeatureList::IsEnabled(switches::kUseSyncInvalidations)) {
+    invalidation_enabled_types.RetainAll(
+        {AUTOFILL_WALLET_DATA, AUTOFILL_WALLET_OFFER});
+  }
+
+  bool success = invalidator_->UpdateInterestedTopics(
+      this, ModelTypeSetToTopicSet(invalidation_enabled_types));
+  DCHECK(success);
+}
+
 }  // namespace syncer

components/sync/driver/glue/sync_engine_impl.h

@@ -186,6 +186,8 @@ class SyncEngineImpl : public SyncEngine,
 
   void OnCookieJarChangedDoneOnFrontendLoop(base::OnceClosure callback);
 
+  void SendInterestedTopicsToInvalidator();
+
   // The task runner where all the sync engine operations happen.
   scoped_refptr<base::SequencedTaskRunner> sync_task_runner_;
 
@@ -222,7 +224,7 @@ class SyncEngineImpl : public SyncEngine,
   SyncInvalidationsService* sync_invalidations_service_ = nullptr;
 
   ModelTypeSet last_enabled_types_;
-  bool sessions_invalidation_enabled_ = false;
+  bool sessions_invalidation_enabled_;
 
   SyncStatus cached_status_;
 

components/sync/driver/glue/sync_engine_impl_unittest.cc

@@ -340,6 +340,21 @@ class SyncEngineImplWithSyncInvalidationsTest : public SyncEngineImplTest {
   NiceMock<MockSyncInvalidationsService> mock_instance_id_driver_;
 };
 
+class SyncEngineImplWithSyncInvalidationsForWalletAndOfferTest
+    : public SyncEngineImplTest {
+ public:
+  SyncEngineImplWithSyncInvalidationsForWalletAndOfferTest() {
+    override_features_.InitWithFeatures(
+        /*enabled_features=*/{switches::kSyncSendInterestedDataTypes,
+                              switches::kUseSyncInvalidations,
+                              switches::kUseSyncInvalidationsForWalletAndOffer},
+        /*disabled_features=*/{});
+  }
+
+ protected:
+  base::test::ScopedFeatureList override_features_;
+};
+
 // Test basic initialization with no initial types (first time initialization).
 // Only the nigori should be configured.
 TEST_F(SyncEngineImplTest, InitShutdown) {
@@ -678,6 +693,32 @@ TEST_F(SyncEngineImplWithSyncInvalidationsTest,
   EXPECT_EQ(1, fake_manager_->GetInvalidationCount(ModelType::PREFERENCES));
 }
 
+TEST_F(SyncEngineImplWithSyncInvalidationsTest,
+       UseOldInvalidationsOnlyForWalletAndOffer) {
+  enabled_types_.PutAll({AUTOFILL_WALLET_DATA, AUTOFILL_WALLET_OFFER});
+
+  InitializeBackend(/*expect_success=*/true);
+  EXPECT_CALL(
+      invalidator_,
+      UpdateInterestedTopics(
+          backend_.get(), ModelTypeSetToTopicSet(
+                              {AUTOFILL_WALLET_DATA, AUTOFILL_WALLET_OFFER})));
+  ConfigureDataTypes();
+
+  // At shutdown, we clear the registered invalidation ids.
+  EXPECT_CALL(invalidator_, UpdateInterestedTopics(backend_.get(), TopicSet()));
+}
+
+TEST_F(SyncEngineImplWithSyncInvalidationsForWalletAndOfferTest,
+       DoNotUseOldInvalidationsAtAll) {
+  enabled_types_.PutAll({AUTOFILL_WALLET_DATA, AUTOFILL_WALLET_OFFER});
+
+  InitializeBackend(/*expect_success=*/true);
+  EXPECT_CALL(invalidator_, UpdateInterestedTopics(testing::_, testing::_))
+      .Times(0);
+  ConfigureDataTypes();
+}
+
 }  // namespace
 
 }  // namespace syncer

components/sync/invalidations/switches.h

@@ -16,12 +16,15 @@ 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.
+// The device will not subscribe to old invalidations for any data types except
+// Wallet and Offer, since that will be covered by the new system.
 // SyncSendInterestedDataTypes must be enabled for this to take effect.
 extern const base::Feature kUseSyncInvalidations;
 
 // If enabled, types related to Wallet and Offer will be included in interested
 // data types, and the device will listen to new invalidations for those types
 // (if they are enabled).
+// The device will not register for old invalidations at all.
 // UseSyncInvalidations must be enabled for this to take effect.
 extern const base::Feature kUseSyncInvalidationsForWalletAndOffer;