[Sync USS] Ignore age watermark directive

This CL removes age watermark directive treatment from
ClientTagBasedModelTypeProcessor. This is not needed as all relevant
data types do expiry on their own.

Bug: 877951
Change-Id: I0f7b075b57de3e6cb1ac53f4223b6289b2acd8a7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1821157
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699846}

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -724,7 +724,6 @@ void ClientTagBasedModelTypeProcessor::OnUpdateReceived(
     error = OnFullUpdateReceived(model_type_state, std::move(updates));
   } else {
     error = OnIncrementalUpdateReceived(model_type_state, std::move(updates));
-    ExpireEntriesIfNeeded(model_type_state.progress_marker());
   }
 
   if (error) {
@@ -1342,49 +1341,6 @@ bool ClientTagBasedModelTypeProcessor::IsModelReadyToSyncForTest() const {
   return model_ready_to_sync_;
 }
 
-void ClientTagBasedModelTypeProcessor::ExpireEntriesIfNeeded(
-    const sync_pb::DataTypeProgressMarker& progress_marker) {
-  if (!progress_marker.has_gc_directive())
-    return;
-
-  const sync_pb::GarbageCollectionDirective& new_gc_directive =
-      progress_marker.gc_directive();
-  std::unique_ptr<MetadataChangeList> metadata_changes =
-      bridge_->CreateMetadataChangeList();
-  bool has_expired_changes = false;
-
-  if (new_gc_directive.has_age_watermark_in_days()) {
-    DCHECK(new_gc_directive.age_watermark_in_days());
-    // For saving resource purpose(ex. cpu, battery), We round up garbage
-    // collection age to day, so we only run GC once a day if server did not
-    // change the |age_watermark_in_days|.
-    base::Time to_be_expired =
-        base::Time::Now().LocalMidnight() -
-        base::TimeDelta::FromDays(new_gc_directive.age_watermark_in_days());
-    if (cached_gc_directive_aged_out_day_ != to_be_expired) {
-      ExpireEntriesByAge(new_gc_directive.age_watermark_in_days(),
-                         metadata_changes.get());
-      cached_gc_directive_aged_out_day_ = to_be_expired;
-      has_expired_changes = true;
-    }
-  }
-
-  if (has_expired_changes)
-    bridge_->ApplySyncChanges(std::move(metadata_changes), EntityChangeList());
-}
-
-void ClientTagBasedModelTypeProcessor::ClearMetadataForEntries(
-    const std::vector<std::string>& storage_key_to_be_deleted,
-    MetadataChangeList* metadata_changes) {
-  for (const std::string& key : storage_key_to_be_deleted) {
-    metadata_changes->ClearMetadata(key);
-    auto iter = storage_key_to_tag_hash_.find(key);
-    DCHECK(iter != storage_key_to_tag_hash_.end());
-    entities_.erase(iter->second);
-    storage_key_to_tag_hash_.erase(key);
-  }
-}
-
 void ClientTagBasedModelTypeProcessor::ExpireAllEntries(
     MetadataChangeList* metadata_changes) {
   DCHECK(metadata_changes);
@@ -1397,27 +1353,14 @@ void ClientTagBasedModelTypeProcessor::ExpireAllEntries(
     }
   }
 
-  ClearMetadataForEntries(storage_key_to_be_deleted, metadata_changes);
-}
-
-void ClientTagBasedModelTypeProcessor::ExpireEntriesByAge(
-    int32_t age_watermark_in_days,
-    MetadataChangeList* metadata_changes) {
-  DCHECK(metadata_changes);
-
-  base::Time to_be_expired =
-      base::Time::Now() - base::TimeDelta::FromDays(age_watermark_in_days);
-  std::vector<std::string> storage_key_to_be_deleted;
-  for (const auto& kv : entities_) {
-    ProcessorEntity* entity = kv.second.get();
-    if (!entity->IsUnsynced() &&
-        ProtoTimeToTime(entity->metadata().modification_time()) <=
-            to_be_expired) {
-      storage_key_to_be_deleted.push_back(entity->storage_key());
-    }
+  // Delete selected keys while not iterating over |entities_|.
+  for (const std::string& key : storage_key_to_be_deleted) {
+    metadata_changes->ClearMetadata(key);
+    auto iter = storage_key_to_tag_hash_.find(key);
+    DCHECK(iter != storage_key_to_tag_hash_.end());
+    entities_.erase(iter->second);
+    storage_key_to_tag_hash_.erase(iter);
   }
-
-  ClearMetadataForEntries(storage_key_to_be_deleted, metadata_changes);
 }
 
 void ClientTagBasedModelTypeProcessor::RemoveEntity(
@@ -1432,7 +1375,6 @@ void ClientTagBasedModelTypeProcessor::ResetState(
     SyncStopMetadataFate metadata_fate) {
   // This should reset all mutable fields (except for |bridge_|).
   worker_.reset();
-  cached_gc_directive_aged_out_day_ = base::Time::FromDoubleT(0);
 
   switch (metadata_fate) {
     case KEEP_METADATA:

components/sync/model_impl/client_tag_based_model_type_processor.h

@@ -215,27 +215,11 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
   // Returns true if all processor entities have non-empty storage keys.
   bool AllStorageKeysPopulated() const;
 
-  // Expires entries according to garbage collection directives.
-  void ExpireEntriesIfNeeded(
-      const sync_pb::DataTypeProgressMarker& progress_marker);
-
-  // Clear metadata for the entries in |storage_key_to_be_deleted|.
-  void ClearMetadataForEntries(
-      const std::vector<std::string>& storage_key_to_be_deleted,
-      MetadataChangeList* metadata_changes);
-
   // Removes metadata for all entries unless they are unsynced.
   // This is used to limit the amount of data stored in sync, and this does not
   // tell the bridge to delete the actual data.
   void ExpireAllEntries(MetadataChangeList* metadata_changes);
 
-  // Removes metadata for all entries whose ages are older than
-  // |age_watermark_in_days| unless they are unsynced.
-  // This is used to limit the amount of data stored in sync, and this does not
-  // tell the bridge to delete the actual data.
-  void ExpireEntriesByAge(int32_t age_watermark_in_days,
-                          MetadataChangeList* metadata_changes);
-
   // Removes |entity| and clears metadata for |entity| from
   // |metadata_change_list|.
   void RemoveEntity(ProcessorEntity* entity,
@@ -326,12 +310,6 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
   // intends to read it. This includes both data and metadata.
   const bool commit_only_;
 
-  // The day which processor already ran garbage collection against on.
-  // Cache this value is for saving resource purpose(ex. cpu, battery), we round
-  // up garbage collection age to day, so we only run GC once a day if server
-  // did not change the age out days.
-  base::Time cached_gc_directive_aged_out_day_;
-
   SEQUENCE_CHECKER(sequence_checker_);
 
   // WeakPtrFactory for this processor for ModelTypeController (only gets

components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

@@ -2184,7 +2184,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
 // Tests that the processor reports an error for updates with a version GC
 // directive that are received for types that support incremental updates.
 TEST_F(ClientTagBasedModelTypeProcessorTest,
-       ShouldApplyGarbageCollectionByVersion) {
+       ShouldNotApplyGarbageCollectionByVersion) {
   InitializeToReadyState();
 
   ExpectError();
@@ -2193,41 +2193,6 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
   worker()->UpdateWithGarbageCollection(garbage_collection_directive);
 }
 
-// Tests that ClientTagBasedModelTypeProcessor can do garbage collection by age.
-// Create 2 entries, one is 15-days-old, another is 5-days-old. Check if sync
-// will delete 15-days-old entry when server set expired age is 10 days.
-TEST_F(ClientTagBasedModelTypeProcessorTest,
-       ShouldApplyGarbageCollectionByAge) {
-  InitializeToReadyState();
-
-  // Create 2 entries, one is 15-days-old, another is 5-days-old.
-  std::unique_ptr<EntityData> entity_data =
-      bridge()->GenerateEntityData(kKey1, kValue1);
-  entity_data->modification_time =
-      base::Time::Now() - base::TimeDelta::FromDays(15);
-  WriteItemAndAck(kKey1, std::move(entity_data));
-  entity_data = bridge()->GenerateEntityData(kKey2, kValue2);
-  entity_data->modification_time =
-      base::Time::Now() - base::TimeDelta::FromDays(5);
-  WriteItemAndAck(kKey2, std::move(entity_data));
-
-  // Verify entries are created correctly.
-  EXPECT_EQ(2U, ProcessorEntityCount());
-  EXPECT_EQ(2U, db()->metadata_count());
-  EXPECT_EQ(2U, db()->data_count());
-  EXPECT_EQ(0U, worker()->GetNumPendingCommits());
-
-  // Expired the entries which are older than 10 days.
-  sync_pb::GarbageCollectionDirective garbage_collection_directive;
-  garbage_collection_directive.set_age_watermark_in_days(10);
-  worker()->UpdateWithGarbageCollection(garbage_collection_directive);
-
-  EXPECT_EQ(1U, ProcessorEntityCount());
-  EXPECT_EQ(1U, db()->metadata_count());
-  EXPECT_EQ(2U, db()->data_count());
-  EXPECT_EQ(0U, worker()->GetNumPendingCommits());
-}
-
 TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldDeleteMetadataWhenCacheGuidMismatch) {
   // Commit item.