Cleanup: Remove consent handling code from UserEventSyncBridge

Consents have been moved to their own data type and thus have their own
Sync bridge.

Bug: 905639
Change-Id: Idf4a9ee11b74eac38f158a0135a4a1998f04fe2a
Reviewed-on: https://chromium-review.googlesource.com/c/1349649Reviewed-by: vitaliii <vitaliii@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610644}

components/sync/user_events/user_event_sync_bridge.cc

@@ -77,10 +77,7 @@ UserEventSyncBridge::UserEventSyncBridge(
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
-UserEventSyncBridge::~UserEventSyncBridge() {
-  if (!deferred_user_events_while_initializing_.empty())
-    LOG(ERROR) << "Non-empty event queue at shutdown!";
-}
+UserEventSyncBridge::~UserEventSyncBridge() = default;
 
 std::unique_ptr<MetadataChangeList>
 UserEventSyncBridge::CreateMetadataChangeList() {
@@ -93,7 +90,6 @@ base::Optional<ModelError> UserEventSyncBridge::MergeSyncData(
   DCHECK(entity_data.empty());
   DCHECK(change_processor()->IsTrackingMetadata());
   DCHECK(!change_processor()->TrackedAccountId().empty());
-  ReadAllDataAndResubmit();
   return ApplySyncChanges(std::move(metadata_change_list),
                           std::move(entity_data));
 }
@@ -151,104 +147,21 @@ std::string UserEventSyncBridge::GetStorageKey(const EntityData& entity_data) {
 
 ModelTypeSyncBridge::StopSyncResponse UserEventSyncBridge::ApplyStopSyncChanges(
     std::unique_ptr<MetadataChangeList> delete_metadata_change_list) {
-  // Sync can only be stopped after initialization.
-  DCHECK(deferred_user_events_while_initializing_.empty());
-
   if (delete_metadata_change_list) {
-    // Delete everything except user consents. With DICE the signout may happen
-    // frequently. It is important to report all user consents, thus, they are
-    // persisted for some time even after signout.
-    store_->ReadAllData(
-        base::BindOnce(&UserEventSyncBridge::OnReadAllDataToDelete,
-                       weak_ptr_factory_.GetWeakPtr(),
-                       std::move(delete_metadata_change_list)));
+    store_->DeleteAllDataAndMetadata(base::BindOnce(
+        &UserEventSyncBridge::OnCommit, weak_ptr_factory_.GetWeakPtr()));
   }
 
   return StopSyncResponse::kModelStillReadyToSync;
 }
 
-void UserEventSyncBridge::OnReadAllDataToDelete(
-    std::unique_ptr<MetadataChangeList> delete_metadata_change_list,
-    const base::Optional<ModelError>& error,
-    std::unique_ptr<RecordList> data_records) {
-  if (error) {
-    LOG(WARNING) << "OnReadAllDataToDelete received a model error: "
-                 << error->ToString();
-    return;
-  }
-
-  std::unique_ptr<WriteBatch> batch = store_->CreateWriteBatch();
-  // We delete all the metadata (even for consents), because it may become
-  // invalid when the sync is reenabled. This may lead to the same consent
-  // being reported multiple times, which is allowed.
-  batch->TakeMetadataChangesFrom(std::move(delete_metadata_change_list));
-
-  UserEventSpecifics specifics;
-  for (const Record& r : *data_records) {
-    if (!specifics.ParseFromString(r.value) ||
-        specifics.event_case() != UserEventSpecifics::EventCase::kUserConsent) {
-      batch->DeleteData(r.id);
-    }
-  }
-
-  store_->CommitWriteBatch(std::move(batch),
-                           base::BindOnce(&UserEventSyncBridge::OnCommit,
-                                          weak_ptr_factory_.GetWeakPtr()));
-}
-
-void UserEventSyncBridge::ReadAllDataAndResubmit() {
-  DCHECK(change_processor()->IsTrackingMetadata());
-  DCHECK(store_);
-  store_->ReadAllData(
-      base::BindOnce(&UserEventSyncBridge::OnReadAllDataToResubmit,
-                     weak_ptr_factory_.GetWeakPtr()));
-}
-
-void UserEventSyncBridge::OnReadAllDataToResubmit(
-    const base::Optional<ModelError>& error,
-    std::unique_ptr<RecordList> data_records) {
-  if (change_processor()->TrackedAccountId().empty()) {
-    // Meanwhile the sync has been disabled. We will try next time.
-    return;
-  }
-  DCHECK(change_processor()->IsTrackingMetadata());
-
-  if (error) {
-    change_processor()->ReportError(*error);
-    return;
-  }
-
-  std::unique_ptr<WriteBatch> batch = store_->CreateWriteBatch();
-
-  for (const Record& r : *data_records) {
-    auto specifics = std::make_unique<UserEventSpecifics>();
-    if (specifics->ParseFromString(r.value) &&
-        specifics->event_case() ==
-            UserEventSpecifics::EventCase::kUserConsent &&
-        specifics->user_consent().account_id() ==
-            change_processor()->TrackedAccountId()) {
-      change_processor()->Put(r.id, MoveToEntityData(std::move(specifics)),
-                              batch->GetMetadataChangeList());
-    }
-  }
-
-  store_->CommitWriteBatch(std::move(batch),
-                           base::BindOnce(&UserEventSyncBridge::OnCommit,
-                                          weak_ptr_factory_.GetWeakPtr()));
-}
-
 void UserEventSyncBridge::RecordUserEvent(
     std::unique_ptr<UserEventSpecifics> specifics) {
-  // TODO(vitaliii): Sanity-check specifics->user_consent().account_id() against
-  // change_processor()->TrackedAccountId(), maybe DCHECK.
-  DCHECK(!specifics->has_user_consent() ||
-         !specifics->user_consent().account_id().empty());
+  DCHECK(!specifics->has_user_consent());
   if (store_) {
     RecordUserEventImpl(std::move(specifics));
     return;
   }
-  if (specifics->has_user_consent())
-    deferred_user_events_while_initializing_.push_back(std::move(specifics));
 }
 
 // static
@@ -265,8 +178,7 @@ void UserEventSyncBridge::RecordUserEventImpl(
     std::unique_ptr<UserEventSpecifics> specifics) {
   DCHECK(store_);
 
-  if (specifics->event_case() != UserEventSpecifics::EventCase::kUserConsent &&
-      !change_processor()->IsTrackingMetadata()) {
+  if (!change_processor()->IsTrackingMetadata()) {
     return;
   }
 
@@ -291,29 +203,15 @@ void UserEventSyncBridge::RecordUserEventImpl(
   std::unique_ptr<WriteBatch> batch = store_->CreateWriteBatch();
   batch->WriteData(storage_key, specifics->SerializeAsString());
 
-  // For consents, only Put() if account ID matches, and unconditionally for
-  // other event types.
-  if (specifics->event_case() != UserEventSpecifics::EventCase::kUserConsent ||
-      specifics->user_consent().account_id() ==
-          change_processor()->TrackedAccountId()) {
-    DCHECK(change_processor()->IsTrackingMetadata());
-    change_processor()->Put(storage_key, MoveToEntityData(std::move(specifics)),
-                            batch->GetMetadataChangeList());
-  }
+  DCHECK(change_processor()->IsTrackingMetadata());
+  change_processor()->Put(storage_key, MoveToEntityData(std::move(specifics)),
+                          batch->GetMetadataChangeList());
 
   store_->CommitWriteBatch(std::move(batch),
                            base::BindOnce(&UserEventSyncBridge::OnCommit,
                                           weak_ptr_factory_.GetWeakPtr()));
 }
 
-void UserEventSyncBridge::ProcessQueuedEvents() {
-  for (std::unique_ptr<sync_pb::UserEventSpecifics>& event :
-       deferred_user_events_while_initializing_) {
-    RecordUserEventImpl(std::move(event));
-  }
-  deferred_user_events_while_initializing_.clear();
-}
-
 void UserEventSyncBridge::OnStoreCreated(
     const base::Optional<ModelError>& error,
     std::unique_ptr<ModelTypeStore> store) {
@@ -334,10 +232,6 @@ void UserEventSyncBridge::OnReadAllMetadata(
     change_processor()->ReportError(*error);
   } else {
     change_processor()->ModelReadyToSync(std::move(metadata_batch));
-    if (!change_processor()->TrackedAccountId().empty()) {
-      ReadAllDataAndResubmit();
-    }
-    ProcessQueuedEvents();
   }
 }
 

components/sync/user_events/user_event_sync_bridge.h

@@ -54,8 +54,6 @@ class UserEventSyncBridge : public ModelTypeSyncBridge {
  private:
   void RecordUserEventImpl(
       std::unique_ptr<sync_pb::UserEventSpecifics> specifics);
-  // Record events in the deferred queue and clear the queue.
-  void ProcessQueuedEvents();
 
   void OnStoreCreated(const base::Optional<ModelError>& error,
                       std::unique_ptr<ModelTypeStore> store);
@@ -69,18 +67,6 @@ class UserEventSyncBridge : public ModelTypeSyncBridge {
   void OnReadAllData(DataCallback callback,
                      const base::Optional<ModelError>& error,
                      std::unique_ptr<ModelTypeStore::RecordList> data_records);
-  void OnReadAllDataToDelete(
-      std::unique_ptr<MetadataChangeList> delete_metadata_change_list,
-      const base::Optional<ModelError>& error,
-      std::unique_ptr<ModelTypeStore::RecordList> data_records);
-
-  // Resubmit all the events persisted in the store to sync events, which were
-  // preserved when sync was disabled. This may resubmit entities that the
-  // processor already knows about (i.e. with metadata), but it is allowed.
-  void ReadAllDataAndResubmit();
-  void OnReadAllDataToResubmit(
-      const base::Optional<ModelError>& error,
-      std::unique_ptr<ModelTypeStore::RecordList> data_records);
 
   void HandleGlobalIdChange(int64_t old_global_id, int64_t new_global_id);
 
@@ -88,11 +74,6 @@ class UserEventSyncBridge : public ModelTypeSyncBridge {
   // delete upon commit confirmation.
   std::unique_ptr<ModelTypeStore> store_;
 
-  // Used to store important events while the store or change processor are not
-  // ready. This currently only handles user consents.
-  std::vector<std::unique_ptr<sync_pb::UserEventSpecifics>>
-      deferred_user_events_while_initializing_;
-
   // The key is the global_id of the navigation the event is linked to.
   std::multimap<int64_t, sync_pb::UserEventSpecifics>
       in_flight_nav_linked_events_;

components/sync/user_events/user_event_sync_bridge_unittest.cc

@@ -225,27 +225,6 @@ TEST_F(UserEventSyncBridgeTest, ApplyStopSyncChanges) {
   EXPECT_THAT(GetAllData(), IsEmpty());
 }
 
-TEST_F(UserEventSyncBridgeTest, ApplyStopSyncChangesShouldKeepConsents) {
-  WaitUntilModelReadyToSync("account_id");
-  UserEventSpecifics user_event_specifics(CreateSpecifics(2u, 2u, 2u));
-  auto* consent = user_event_specifics.mutable_user_consent();
-  consent->set_feature(UserEventSpecifics::UserConsent::CHROME_SYNC);
-  consent->set_account_id("account_id");
-  bridge()->RecordUserEvent(
-      std::make_unique<UserEventSpecifics>(user_event_specifics));
-  ASSERT_THAT(GetAllData(), SizeIs(1));
-
-  EXPECT_THAT(
-      bridge()->ApplyStopSyncChanges(WriteBatch::CreateMetadataChangeList()),
-      Eq(ModelTypeSyncBridge::StopSyncResponse::kModelStillReadyToSync));
-  // The bridge may asynchronously query the store to choose what to delete.
-  base::RunLoop().RunUntilIdle();
-
-  // User consent specific must be persisted when sync is disabled.
-  EXPECT_THAT(GetAllData(),
-              ElementsAre(Pair(_, MatchesUserEvent(user_event_specifics))));
-}
-
 TEST_F(UserEventSyncBridgeTest, MultipleRecords) {
   WaitUntilModelReadyToSync();
   std::set<std::string> unique_storage_keys;
@@ -380,155 +359,6 @@ TEST_F(UserEventSyncBridgeTest, RecordBeforeMetadataLoads) {
   EXPECT_THAT(GetAllData(), IsEmpty());
 }
 
-// User consents should be buffered if the bridge is not fully initialized.
-// Other events should get dropped.
-TEST_F(UserEventSyncBridgeTest, RecordWithLateInitializedStore) {
-  UserEventSpecifics consent1 = CreateSpecifics(1u, 1u, 1u);
-  consent1.mutable_user_consent()->set_account_id("account_id");
-  UserEventSpecifics consent2 = CreateSpecifics(2u, 2u, 2u);
-  consent2.mutable_user_consent()->set_account_id("account_id");
-  UserEventSpecifics specifics1 = CreateSpecifics(3u, 3u, 3u);
-  UserEventSpecifics specifics2 = CreateSpecifics(4u, 4u, 4u);
-
-  bridge()->RecordUserEvent(std::make_unique<UserEventSpecifics>(consent1));
-  bridge()->RecordUserEvent(std::make_unique<UserEventSpecifics>(specifics1));
-
-  // Initialize the store.
-  EXPECT_CALL(*processor(), ModelReadyToSync(NotNull()));
-  WaitUntilModelReadyToSync("account_id");
-
-  // Record events after metadata is ready.
-  bridge()->RecordUserEvent(std::make_unique<UserEventSpecifics>(consent2));
-  bridge()->RecordUserEvent(std::make_unique<UserEventSpecifics>(specifics2));
-
-  EXPECT_THAT(
-      GetAllData(),
-      UnorderedElementsAre(
-          Pair(GetStorageKey(consent1), MatchesUserEvent(consent1)),
-          Pair(GetStorageKey(consent2), MatchesUserEvent(consent2)),
-          Pair(GetStorageKey(specifics2), MatchesUserEvent(specifics2))));
-}
-
-TEST_F(UserEventSyncBridgeTest,
-       ShouldReportPreviouslyPersistedConsentsWhenSyncIsReenabled) {
-  WaitUntilModelReadyToSync("account_id");
-
-  UserEventSpecifics consent = CreateSpecifics(1u, 1u, 1u);
-  consent.mutable_user_consent()->set_account_id("account_id");
-
-  bridge()->RecordUserEvent(std::make_unique<UserEventSpecifics>(consent));
-
-  // User disables sync, however, the consent hasn't been submitted yet. It is
-  // preserved to be submitted when sync is re-enabled.
-  EXPECT_THAT(
-      bridge()->ApplyStopSyncChanges(WriteBatch::CreateMetadataChangeList()),
-      Eq(ModelTypeSyncBridge::StopSyncResponse::kModelStillReadyToSync));
-  // The bridge may asynchronously query the store to choose what to delete.
-  base::RunLoop().RunUntilIdle();
-
-  ASSERT_THAT(GetAllData(), SizeIs(1));
-
-  // Reenable sync.
-  EXPECT_CALL(*processor(), Put(GetStorageKey(consent), _, _));
-  ON_CALL(*processor(), IsTrackingMetadata()).WillByDefault(Return(true));
-  ON_CALL(*processor(), TrackedAccountId()).WillByDefault(Return("account_id"));
-  bridge()->MergeSyncData(WriteBatch::CreateMetadataChangeList(),
-                          EntityChangeList());
-
-  // The bridge may asynchronously query the store to choose what to resubmit.
-  base::RunLoop().RunUntilIdle();
-}
-
-TEST_F(UserEventSyncBridgeTest,
-       ShouldReportPersistedConsentsOnStartupWithSyncAlreadyEnabled) {
-  // Persist a consent while sync is enabled.
-  WaitUntilModelReadyToSync("account_id");
-  UserEventSpecifics consent = CreateSpecifics(1u, 1u, 1u);
-  consent.mutable_user_consent()->set_account_id("account_id");
-  bridge()->RecordUserEvent(std::make_unique<UserEventSpecifics>(consent));
-  base::RunLoop().RunUntilIdle();
-  ASSERT_THAT(GetAllData(), SizeIs(1));
-
-  // Restart the bridge, mimic-ing a browser restart.
-  EXPECT_CALL(*processor(), Put(GetStorageKey(consent), _, _));
-  ResetBridge();
-
-  // The bridge may asynchronously query the store to choose what to resubmit.
-  base::RunLoop().RunUntilIdle();
-}
-
-TEST_F(UserEventSyncBridgeTest, ShouldReportPersistedConsentsOnSyncEnabled) {
-  // Persist a consent before sync is enabled.
-  WaitUntilModelReadyToSync(/*account=id=*/"");
-  UserEventSpecifics consent = CreateSpecifics(1u, 1u, 1u);
-  consent.mutable_user_consent()->set_account_id("account_id");
-  bridge()->RecordUserEvent(std::make_unique<UserEventSpecifics>(consent));
-  base::RunLoop().RunUntilIdle();
-  ASSERT_THAT(GetAllData(), SizeIs(1));
-
-  // Restart the bridge, mimic-ing a browser restart. We expect no Put()
-  // until sync is enabled.
-  EXPECT_CALL(*processor(), Put(_, _, _)).Times(0);
-  ResetBridge();
-  WaitUntilModelReadyToSync(/*account_id=*/"");
-
-  // Enable sync.
-  EXPECT_CALL(*processor(), Put(GetStorageKey(consent), _, _));
-  ON_CALL(*processor(), IsTrackingMetadata()).WillByDefault(Return(true));
-  ON_CALL(*processor(), TrackedAccountId()).WillByDefault(Return("account_id"));
-  bridge()->MergeSyncData(WriteBatch::CreateMetadataChangeList(),
-                          EntityChangeList());
-
-  // The bridge may asynchronously query the store to choose what to resubmit.
-  base::RunLoop().RunUntilIdle();
-}
-
-TEST_F(UserEventSyncBridgeTest, ShouldSubmitPersistedConsentOnlyIfSameAccount) {
-  WaitUntilModelReadyToSync("first_account");
-  UserEventSpecifics user_event_specifics(CreateSpecifics(2u, 2u, 2u));
-  auto* consent = user_event_specifics.mutable_user_consent();
-  consent->set_account_id("first_account");
-  bridge()->RecordUserEvent(
-      std::make_unique<UserEventSpecifics>(user_event_specifics));
-  ASSERT_THAT(GetAllData(), SizeIs(1));
-
-  EXPECT_THAT(
-      bridge()->ApplyStopSyncChanges(WriteBatch::CreateMetadataChangeList()),
-      Eq(ModelTypeSyncBridge::StopSyncResponse::kModelStillReadyToSync));
-  // The bridge may asynchronously query the store to choose what to delete.
-  base::RunLoop().RunUntilIdle();
-
-  ASSERT_THAT(GetAllData(),
-              ElementsAre(Pair(_, MatchesUserEvent(user_event_specifics))));
-
-  // A new user signs in and enables sync.
-  // The previous account consent should not be resubmited, because the new sync
-  // account is different.
-  EXPECT_CALL(*processor(), Put(_, _, _)).Times(0);
-  ON_CALL(*processor(), IsTrackingMetadata()).WillByDefault(Return(true));
-  ON_CALL(*processor(), TrackedAccountId())
-      .WillByDefault(Return("second_account"));
-  bridge()->MergeSyncData(WriteBatch::CreateMetadataChangeList(),
-                          EntityChangeList());
-  base::RunLoop().RunUntilIdle();
-
-  EXPECT_THAT(
-      bridge()->ApplyStopSyncChanges(WriteBatch::CreateMetadataChangeList()),
-      Eq(ModelTypeSyncBridge::StopSyncResponse::kModelStillReadyToSync));
-  base::RunLoop().RunUntilIdle();
-
-  // The previous user signs in again and enables sync.
-  EXPECT_CALL(*processor(), Put(GetStorageKey(user_event_specifics), _, _));
-  ON_CALL(*processor(), IsTrackingMetadata()).WillByDefault(Return(true));
-  ON_CALL(*processor(), TrackedAccountId())
-      .WillByDefault(Return("first_account"));
-  bridge()->MergeSyncData(WriteBatch::CreateMetadataChangeList(),
-                          EntityChangeList());
-
-  // The bridge may asynchronously query the store to choose what to resubmit.
-  base::RunLoop().RunUntilIdle();
-}
-
 }  // namespace
 
 }  // namespace syncer