[Sync] Remove setting of cache GUID for old clients.

This patch removes old logic of setting cache GUID for old clients which
had no this field. Clients must have set this field after one and a half
years. Clients that haven't migrated will remove sync metadata (treat as
cache guid mismatch) and go through download and merge procedure.

There is one more little change: ConnectIfReady is removed from
OnPendingDataLoaded because it does nothing and seems to be obsolete.

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

components/autofill/core/browser/webdata/autofill_wallet_metadata_sync_bridge_unittest.cc

@@ -86,6 +86,8 @@ const char kLocalAddr2ServerId[] = "fa232b9a-f248-4e5a-8d76-d46f821c0c5f";
 
 const char kLocaleString[] = "en-US";
 
+const char kDefaultCacheGuid[] = "CacheGuid";
+
 base::Time UseDateFromProtoValue(int64_t use_date_proto_value) {
   return base::Time::FromDeltaSinceWindowsEpoch(
       base::TimeDelta::FromMicroseconds(use_date_proto_value));
@@ -288,6 +290,7 @@ class AutofillWalletMetadataSyncBridgeTest : public testing::Test {
     model_type_state.set_initial_sync_done(initial_sync_done);
     model_type_state.mutable_progress_marker()->set_data_type_id(
         GetSpecificsFieldNumberFromModelType(syncer::AUTOFILL_WALLET_METADATA));
+    model_type_state.set_cache_guid(kDefaultCacheGuid);
     EXPECT_TRUE(table()->UpdateModelTypeState(syncer::AUTOFILL_WALLET_METADATA,
                                               model_type_state));
     bridge_.reset(new AutofillWalletMetadataSyncBridge(
@@ -305,6 +308,7 @@ class AutofillWalletMetadataSyncBridgeTest : public testing::Test {
     base::RunLoop loop;
     syncer::DataTypeActivationRequest request;
     request.error_handler = base::DoNothing();
+    request.cache_guid = kDefaultCacheGuid;
     real_processor_->OnSyncStarting(
         request,
         base::BindLambdaForTesting(

components/autofill/core/browser/webdata/autofill_wallet_sync_bridge_unittest.cc

@@ -82,6 +82,8 @@ const char kCloudTokenDataClientTag[] = "token";
 const char kLocaleString[] = "en-US";
 const base::Time kJune2017 = base::Time::FromDoubleT(1497552271);
 
+const char kDefaultCacheGuid[] = "CacheGuid";
+
 void ExtractAutofillWalletSpecificsFromDataBatch(
     std::unique_ptr<DataBatch> batch,
     std::vector<AutofillWalletSpecifics>* output) {
@@ -235,6 +237,7 @@ class AutofillWalletSyncBridgeTest : public testing::Test {
     model_type_state.set_initial_sync_done(initial_sync_done);
     model_type_state.mutable_progress_marker()->set_data_type_id(
         GetSpecificsFieldNumberFromModelType(syncer::AUTOFILL_WALLET_DATA));
+    model_type_state.set_cache_guid(kDefaultCacheGuid);
     EXPECT_TRUE(table()->UpdateModelTypeState(syncer::AUTOFILL_WALLET_DATA,
                                               model_type_state));
     bridge_ = std::make_unique<AutofillWalletSyncBridge>(
@@ -246,6 +249,7 @@ class AutofillWalletSyncBridgeTest : public testing::Test {
     base::RunLoop loop;
     syncer::DataTypeActivationRequest request;
     request.error_handler = base::DoNothing();
+    request.cache_guid = kDefaultCacheGuid;
     real_processor_->OnSyncStarting(
         request,
         base::BindLambdaForTesting(

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -337,7 +337,9 @@ void ClientTagBasedModelTypeProcessor::Put(
     // The bridge is creating a new entity. The bridge may or may not populate
     // |data->client_tag_hash|, so let's ask for the client tag if needed.
     if (data->client_tag_hash.value().empty()) {
-      data->client_tag_hash = GetClientTagHash(storage_key, *data);
+      DCHECK(bridge_->SupportsGetClientTag());
+      data->client_tag_hash =
+          ClientTagHash::FromUnhashed(type_, bridge_->GetClientTag(*data));
     } else if (bridge_->SupportsGetClientTag()) {
       // If the Put() call already included the client tag, let's verify that
       // it's consistent with the bridge's regular GetClientTag() function (if
@@ -416,7 +418,7 @@ void ClientTagBasedModelTypeProcessor::UpdateStorageKey(
   DCHECK(!bridge_->SupportsGetStorageKey());
   DCHECK(entity_tracker_);
 
-  ProcessorEntity* entity =
+  const ProcessorEntity* entity =
       entity_tracker_->GetEntityForTagHash(client_tag_hash);
   DCHECK(entity);
 
@@ -832,10 +834,6 @@ void ClientTagBasedModelTypeProcessor::OnPendingDataLoaded(
     return;
 
   ConsumeDataBatch(std::move(storage_keys_to_load), std::move(data_batch));
-
-  // TODO(rushans): looks like some old logic, remove ConnectIfReady() in a
-  // separate patch.
-  ConnectIfReady();
   CommitLocalChanges(max_entries, std::move(callback));
 }
 
@@ -920,18 +918,6 @@ void ClientTagBasedModelTypeProcessor::CommitLocalChanges(
   std::move(callback).Run(std::move(commit_requests));
 }
 
-ClientTagHash ClientTagBasedModelTypeProcessor::GetClientTagHash(
-    const std::string& storage_key,
-    const EntityData& data) const {
-  DCHECK(entity_tracker_);
-  const base::Optional<ClientTagHash> client_tag_hash =
-      entity_tracker_->GetClientTagHash(storage_key);
-  DCHECK(bridge_->SupportsGetClientTag());
-  return client_tag_hash.has_value()
-             ? client_tag_hash.value()
-             : ClientTagHash::FromUnhashed(type_, bridge_->GetClientTag(data));
-}
-
 ProcessorEntity* ClientTagBasedModelTypeProcessor::CreateEntity(
     const std::string& storage_key,
     const EntityData& data) {
@@ -1095,27 +1081,11 @@ void ClientTagBasedModelTypeProcessor::MergeDataWithMetadataForDebugging(
   std::move(callback).Run(type_, std::move(all_nodes));
 }
 
-void ClientTagBasedModelTypeProcessor::InitializeCacheGuidForOldClients() {
-  DCHECK(entity_tracker_);
-
-  // TODO(rushans): remove this logic because cache GUID must have been
-  // initialized for most of clients. Clients that have not initialized yet will
-  // behave as in cache GUID mismatch situation.
-  if (!entity_tracker_->model_type_state().has_cache_guid()) {
-    sync_pb::ModelTypeState model_type_state =
-        entity_tracker_->model_type_state();
-    model_type_state.set_cache_guid(activation_request_.cache_guid);
-    entity_tracker_->set_model_type_state(model_type_state);
-  }
-}
-
 void ClientTagBasedModelTypeProcessor::CheckForInvalidPersistedMetadata() {
   if (!entity_tracker_) {
     return;
   }
 
-  InitializeCacheGuidForOldClients();
-
   const sync_pb::ModelTypeState& model_type_state =
       entity_tracker_->model_type_state();
   const bool invalid_cache_guid =

components/sync/model_impl/client_tag_based_model_type_processor.h

@@ -210,9 +210,6 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
   void MergeDataWithMetadataForDebugging(AllNodesCallback callback,
                                          std::unique_ptr<DataBatch> batch);
 
-  // Initialize the cache_guid for old clients that didn't persist it.
-  void InitializeCacheGuidForOldClients();
-
   // Checks for valid cache GUID and data type id. Resets state if metadata is
   // invalid.
   void CheckForInvalidPersistedMetadata();

components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

@@ -49,6 +49,8 @@ const char kValue1[] = "value1";
 const char kValue2[] = "value2";
 const char kValue3[] = "value3";
 
+const char kCacheGuid[] = "TestCacheGuid";
+
 ClientTagHash GetHash(const std::string& key) {
   return FakeModelTypeSyncBridge::TagHashFromKey(key);
 }
@@ -133,6 +135,7 @@ class TestModelTypeSyncBridge : public FakeModelTypeSyncBridge {
   void SetInitialSyncDone(bool is_done) {
     ModelTypeState model_type_state(db().model_type_state());
     model_type_state.set_initial_sync_done(is_done);
+    model_type_state.set_cache_guid(kCacheGuid);
     model_type_state.mutable_progress_marker()->set_data_type_id(
         GetSpecificsFieldNumberFromModelType(model_type_));
     model_type_state.set_authenticated_account_id(
@@ -278,7 +281,7 @@ class ClientTagBasedModelTypeProcessorTest : public ::testing::Test {
 
   void OnSyncStarting(const std::string& authenticated_account_id =
                           kDefaultAuthenticatedAccountId,
-                      const std::string& cache_guid = "TestCacheGuid",
+                      const std::string& cache_guid = kCacheGuid,
                       SyncMode sync_mode = SyncMode::kFull) {
     DataTypeActivationRequest request;
     request.error_handler = base::BindRepeating(
@@ -471,6 +474,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
   std::unique_ptr<MetadataBatch> metadata_batch = db()->CreateMetadataBatch();
   sync_pb::ModelTypeState model_type_state(metadata_batch->GetModelTypeState());
   model_type_state.set_initial_sync_done(true);
+  model_type_state.set_cache_guid(kCacheGuid);
   model_type_state.set_authenticated_account_id("PersistedAccountId");
   model_type_state.mutable_progress_marker()->set_data_type_id(
       GetSpecificsFieldNumberFromModelType(GetModelType()));
@@ -489,9 +493,9 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldExposeNewlyTrackedCacheGuid) {
   ModelReadyToSync();
   ASSERT_EQ("", type_processor()->TrackedCacheGuid());
-  OnSyncStarting(kDefaultAuthenticatedAccountId, "TestCacheGuid");
+  OnSyncStarting();
   worker()->UpdateFromServer();
-  EXPECT_EQ("TestCacheGuid", type_processor()->TrackedCacheGuid());
+  EXPECT_EQ(kCacheGuid, type_processor()->TrackedCacheGuid());
 }
 
 TEST_F(ClientTagBasedModelTypeProcessorTest,
@@ -1881,7 +1885,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
 TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldReportEphemeralConfigurationTime) {
   InitializeToMetadataLoaded(/*initial_sync_done=*/false);
-  OnSyncStarting(kDefaultAuthenticatedAccountId, "TestCacheGuid",
+  OnSyncStarting(kDefaultAuthenticatedAccountId, kCacheGuid,
                  SyncMode::kTransportOnly);
 
   base::HistogramTester histogram_tester;
@@ -1906,8 +1910,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
 TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldReportPersistentConfigurationTime) {
   InitializeToMetadataLoaded(/*initial_sync_done=*/false);
-  OnSyncStarting(kDefaultAuthenticatedAccountId, "TestCacheGuid",
-                 SyncMode::kFull);
+  OnSyncStarting();
 
   base::HistogramTester histogram_tester;
 
@@ -1976,7 +1979,7 @@ TEST_F(FullUpdateClientTagBasedModelTypeProcessorTest,
 TEST_F(FullUpdateClientTagBasedModelTypeProcessorTest,
        ShouldReportEphemeralConfigurationTimeOnlyForFirstFullUpdate) {
   InitializeToMetadataLoaded(/*initial_sync_done=*/false);
-  OnSyncStarting(kDefaultAuthenticatedAccountId, "TestCacheGuid",
+  OnSyncStarting(kDefaultAuthenticatedAccountId, kCacheGuid,
                  SyncMode::kTransportOnly);
 
   UpdateResponseDataList updates1;
@@ -2628,6 +2631,7 @@ TEST_F(CommitOnlyClientTagBasedModelTypeProcessorTest,
   std::unique_ptr<MetadataBatch> metadata_batch = db()->CreateMetadataBatch();
   sync_pb::ModelTypeState model_type_state(metadata_batch->GetModelTypeState());
   model_type_state.set_initial_sync_done(true);
+  model_type_state.set_cache_guid(kCacheGuid);
   model_type_state.set_authenticated_account_id("PersistedAccountId");
   model_type_state.mutable_progress_marker()->set_data_type_id(
       GetSpecificsFieldNumberFromModelType(GetModelType()));
@@ -2656,6 +2660,7 @@ TEST_F(CommitOnlyClientTagBasedModelTypeProcessorTest,
   std::unique_ptr<MetadataBatch> metadata_batch = db()->CreateMetadataBatch();
   sync_pb::ModelTypeState model_type_state(metadata_batch->GetModelTypeState());
   model_type_state.set_initial_sync_done(true);
+  model_type_state.set_cache_guid(kCacheGuid);
   model_type_state.set_authenticated_account_id("PersistedAccountId");
   model_type_state.mutable_progress_marker()->set_data_type_id(
       GetSpecificsFieldNumberFromModelType(GetModelType()));

components/sync/model_impl/processor_entity_tracker.cc

@@ -230,13 +230,4 @@ void ProcessorEntityTracker::UpdateOrOverrideStorageKey(
   storage_key_to_tag_hash_[storage_key] = client_tag_hash;
 }
 
-base::Optional<ClientTagHash> ProcessorEntityTracker::GetClientTagHash(
-    const std::string& storage_key) const {
-  auto iter = storage_key_to_tag_hash_.find(storage_key);
-  if (iter != storage_key_to_tag_hash_.end()) {
-    return iter->second;
-  }
-  return base::nullopt;
-}
-
 }  // namespace syncer

components/sync/model_impl/processor_entity_tracker.h

@@ -103,9 +103,6 @@ class ProcessorEntityTracker {
   void UpdateOrOverrideStorageKey(const ClientTagHash& client_tag_hash,
                                   const std::string& storage_key);
 
-  base::Optional<ClientTagHash> GetClientTagHash(
-      const std::string& storage_key) const;
-
  private:
   // A map of client tag hash to sync entities known to this tracker. This
   // should contain entries and metadata, although the entities may not always

components/sync_sessions/session_sync_bridge_unittest.cc

@@ -226,6 +226,7 @@ class SessionSyncBridgeTest : public ::testing::Test {
 
     sync_pb::ModelTypeState state;
     state.set_initial_sync_done(true);
+    state.set_cache_guid(request.cache_guid);
     state.mutable_progress_marker()->set_data_type_id(
         GetSpecificsFieldNumberFromModelType(syncer::SESSIONS));
     state.set_authenticated_account_id("SomeAccountId");