[USS] Rename UntrackEntity() to UntrackEntityForClientTagHash()

The new name better fits to the other existing function
UntrackEntityForStorageKey() and the updated comment makes it clear this
should be only used when storage key is not available.

Apart from the rename, the function now ignores if the provided
client_tag_hash does not exist or is not tracked any more.

Bug: NONE
Change-Id: Ic68a7ab939fd17436740df3ca75389c1ed34f329
Reviewed-on: https://chromium-review.googlesource.com/c/1347287Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610428}

components/history/core/browser/sync/typed_url_sync_bridge.cc

@@ -165,7 +165,8 @@ base::Optional<ModelError> TypedURLSyncBridge::MergeSyncData(
         GetStorageKeyInternal(entity_change.data().specifics.typed_url().url());
     if (storage_key.empty()) {
       // ignore entity change
-      change_processor()->UntrackEntity(entity_change.data());
+      change_processor()->UntrackEntityForClientTagHash(
+          entity_change.data().client_tag_hash);
     } else {
       change_processor()->UpdateStorageKey(entity_change.data(), storage_key,
                                            metadata_change_list.get());
@@ -245,7 +246,8 @@ base::Optional<ModelError> TypedURLSyncBridge::ApplySyncChanges(
           entity_change.data().specifics.typed_url().url());
       if (storage_key.empty()) {
         // ignore entity change
-        change_processor()->UntrackEntity(entity_change.data());
+        change_processor()->UntrackEntityForClientTagHash(
+            entity_change.data().client_tag_hash);
       } else {
         change_processor()->UpdateStorageKey(entity_change.data(), storage_key,
                                              metadata_change_list.get());

components/history/core/browser/sync/typed_url_sync_bridge_unittest.cc

@@ -1188,7 +1188,7 @@ TEST_F(TypedURLSyncBridgeTest, AddUrlAndVisits) {
 
   ASSERT_EQ(0U, processor().put_multimap().size());
   ASSERT_EQ(1U, processor().update_multimap().size());
-  ASSERT_EQ(0U, processor().untrack_set().size());
+  ASSERT_EQ(0U, processor().untrack_for_client_tag_hash_set().size());
 
   // Verify processor receive correct upate storage key.
   const auto& it = processor().update_multimap().begin();
@@ -1222,7 +1222,7 @@ TEST_F(TypedURLSyncBridgeTest, AddExpiredUrlAndVisits) {
 
   ASSERT_EQ(0U, processor().put_multimap().size());
   ASSERT_EQ(0U, processor().update_multimap().size());
-  ASSERT_EQ(1U, processor().untrack_set().size());
+  ASSERT_EQ(1U, processor().untrack_for_client_tag_hash_set().size());
 
   URLID url_id = fake_history_backend_->GetIdByUrl(GURL(kURL));
   ASSERT_EQ(0, url_id);

components/sync/model/fake_model_type_change_processor.cc

@@ -37,12 +37,12 @@ void FakeModelTypeChangeProcessor::UpdateStorageKey(
     const std::string& storage_key,
     MetadataChangeList* metadata_change_list) {}
 
-void FakeModelTypeChangeProcessor::UntrackEntity(
-    const EntityData& entity_data) {}
-
 void FakeModelTypeChangeProcessor::UntrackEntityForStorageKey(
     const std::string& storage_key) {}
 
+void FakeModelTypeChangeProcessor::UntrackEntityForClientTagHash(
+    const std::string& client_tag_hash) {}
+
 bool FakeModelTypeChangeProcessor::IsEntityUnsynced(
     const std::string& storage_key) {
   return false;

components/sync/model/fake_model_type_change_processor.h

@@ -35,8 +35,9 @@ class FakeModelTypeChangeProcessor : public ModelTypeChangeProcessor {
   void UpdateStorageKey(const EntityData& entity_data,
                         const std::string& storage_key,
                         MetadataChangeList* metadata_change_list) override;
-  void UntrackEntity(const EntityData& entity_data) override;
   void UntrackEntityForStorageKey(const std::string& storage_key) override;
+  void UntrackEntityForClientTagHash(
+      const std::string& client_tag_hash) override;
   bool IsEntityUnsynced(const std::string& storage_key) override;
   void OnModelStarting(ModelTypeSyncBridge* bridge) override;
   void ModelReadyToSync(std::unique_ptr<MetadataBatch> batch) override;

components/sync/model/fake_model_type_sync_bridge.cc

@@ -228,7 +228,8 @@ base::Optional<ModelError> FakeModelTypeSyncBridge::MergeSyncData(
     if (storage_key.empty()) {
       storage_key = GetStorageKeyImpl(change.data());
       if (base::ContainsKey(keys_to_ignore_, storage_key)) {
-        change_processor()->UntrackEntity(change.data());
+        change_processor()->UntrackEntityForClientTagHash(
+            change.data().client_tag_hash);
       } else {
         change_processor()->UpdateStorageKey(change.data(), storage_key,
                                              metadata_change_list.get());

components/sync/model/mock_model_type_change_processor.cc

@@ -40,14 +40,15 @@ class ForwardingModelTypeChangeProcessor : public ModelTypeChangeProcessor {
     other_->UpdateStorageKey(entity_data, storage_key, metadata_change_list);
   }
 
-  void UntrackEntity(const EntityData& entity_data) override {
-    other_->UntrackEntity(entity_data);
-  }
-
   void UntrackEntityForStorageKey(const std::string& storage_key) override {
     other_->UntrackEntityForStorageKey(storage_key);
   }
 
+  void UntrackEntityForClientTagHash(
+      const std::string& client_tag_hash) override {
+    other_->UntrackEntityForClientTagHash(client_tag_hash);
+  }
+
   bool IsEntityUnsynced(const std::string& storage_key) override {
     return other_->IsEntityUnsynced(storage_key);
   }
@@ -108,12 +109,12 @@ void MockModelTypeChangeProcessor::DelegateCallsByDefaultTo(
   ON_CALL(*this, UpdateStorageKey(_, _, _))
       .WillByDefault(
           Invoke(delegate, &ModelTypeChangeProcessor::UpdateStorageKey));
-  ON_CALL(*this, UntrackEntity(_))
-      .WillByDefault(
-          Invoke(delegate, &ModelTypeChangeProcessor::UntrackEntity));
   ON_CALL(*this, UntrackEntityForStorageKey(_))
       .WillByDefault(Invoke(
           delegate, &ModelTypeChangeProcessor::UntrackEntityForStorageKey));
+  ON_CALL(*this, UntrackEntityForClientTagHash(_))
+      .WillByDefault(Invoke(
+          delegate, &ModelTypeChangeProcessor::UntrackEntityForClientTagHash));
   ON_CALL(*this, IsEntityUnsynced(_))
       .WillByDefault(
           Invoke(delegate, &ModelTypeChangeProcessor::IsEntityUnsynced));

components/sync/model/mock_model_type_change_processor.h

@@ -30,9 +30,10 @@ class MockModelTypeChangeProcessor : public ModelTypeChangeProcessor {
                void(const EntityData& entity_data,
                     const std::string& storage_key,
                     MetadataChangeList* metadata_change_list));
-  MOCK_METHOD1(UntrackEntity, void(const EntityData& entity_data));
   MOCK_METHOD1(UntrackEntityForStorageKey,
                void(const std::string& storage_key));
+  MOCK_METHOD1(UntrackEntityForClientTagHash,
+               void(const std::string& client_tag_hash));
   MOCK_METHOD1(IsEntityUnsynced, bool(const std::string& storage_key));
   MOCK_METHOD1(OnModelStarting, void(ModelTypeSyncBridge* bridge));
   MOCK_METHOD1(ModelReadyToSync, void(std::unique_ptr<MetadataBatch> batch));

components/sync/model/model_type_change_processor.h

@@ -52,14 +52,6 @@ class ModelTypeChangeProcessor {
                                 const std::string& storage_key,
                                 MetadataChangeList* metadata_change_list) = 0;
 
-  // Remove entity metadata and do not track the entity. This function only
-  // applies to datatypes that can't generate storage key based on EntityData.
-  // Bridge should call this function when handling
-  // MergeSyncData/ApplySyncChanges to inform the processor that this entity
-  // should not been tracked. Datatypes that support GetStorageKey should call
-  // change_processor()->Delete() instead.
-  virtual void UntrackEntity(const EntityData& entity_data) = 0;
-
   // Remove entity metadata and do not track the entity. Applies to datatypes
   // that support local deletions that should not get synced up (e.g. TYPED_URL
   // does not sync up deletions of expired URLs). If the deletion should get
@@ -67,6 +59,13 @@ class ModelTypeChangeProcessor {
   // |storage_key| is unknown.
   virtual void UntrackEntityForStorageKey(const std::string& storage_key) = 0;
 
+  // Remove entity metadata and do not track the entity, exactly like
+  // UntrackEntityForStorageKey() above. This function should only be called by
+  // datatypes that can't generate storage keys. The call is ignored if
+  // |client_tag_hash| is unknown.
+  virtual void UntrackEntityForClientTagHash(
+      const std::string& client_tag_hash) = 0;
+
   // Returns true if a tracked entity has local changes. A commit may or may not
   // be in progress at this time.
   // TODO(mastiz): The only user of this is HISTORY_DELETE_DIRECTIVES which

components/sync/model/recording_model_type_change_processor.cc

@@ -38,16 +38,16 @@ void RecordingModelTypeChangeProcessor::UpdateStorageKey(
       storage_key, FakeModelTypeSyncBridge::CopyEntityData(entity_data)));
 }
 
-void RecordingModelTypeChangeProcessor::UntrackEntity(
-    const EntityData& entity_data) {
-  untrack_set_.insert(FakeModelTypeSyncBridge::CopyEntityData(entity_data));
-}
-
 void RecordingModelTypeChangeProcessor::UntrackEntityForStorageKey(
     const std::string& storage_key) {
   untrack_for_storage_key_set_.insert(storage_key);
 }
 
+void RecordingModelTypeChangeProcessor::UntrackEntityForClientTagHash(
+    const std::string& client_tag_hash) {
+  untrack_for_client_tag_hash_set_.insert(client_tag_hash);
+}
+
 void RecordingModelTypeChangeProcessor::ModelReadyToSync(
     std::unique_ptr<MetadataBatch> batch) {
   std::swap(metadata_, batch);

components/sync/model/recording_model_type_change_processor.h

@@ -31,8 +31,9 @@ class RecordingModelTypeChangeProcessor : public FakeModelTypeChangeProcessor {
   void UpdateStorageKey(const EntityData& entity_data,
                         const std::string& storage_key,
                         MetadataChangeList* metadata_change_list) override;
-  void UntrackEntity(const EntityData& entity_data) override;
   void UntrackEntityForStorageKey(const std::string& storage_key) override;
+  void UntrackEntityForClientTagHash(
+      const std::string& client_tag_hash) override;
   void ModelReadyToSync(std::unique_ptr<MetadataBatch> batch) override;
   bool IsTrackingMetadata() override;
   std::string TrackedAccountId() override;
@@ -51,14 +52,14 @@ class RecordingModelTypeChangeProcessor : public FakeModelTypeChangeProcessor {
 
   const std::set<std::string>& delete_set() const { return delete_set_; }
 
-  const std::set<std::unique_ptr<EntityData>>& untrack_set() const {
-    return untrack_set_;
-  }
-
   const std::set<std::string>& untrack_for_storage_key_set() const {
     return untrack_for_storage_key_set_;
   }
 
+  const std::set<std::string>& untrack_for_client_tag_hash_set() const {
+    return untrack_for_client_tag_hash_set_;
+  }
+
   MetadataBatch* metadata() const { return metadata_.get(); }
 
   // Constructs the processor and assigns its raw pointer to the given address.
@@ -72,8 +73,8 @@ class RecordingModelTypeChangeProcessor : public FakeModelTypeChangeProcessor {
   std::multimap<std::string, std::unique_ptr<EntityData>> put_multimap_;
   std::multimap<std::string, std::unique_ptr<EntityData>> update_multimap_;
   std::set<std::string> delete_set_;
-  std::set<std::unique_ptr<EntityData>> untrack_set_;
   std::set<std::string> untrack_for_storage_key_set_;
+  std::set<std::string> untrack_for_client_tag_hash_set_;
   std::unique_ptr<MetadataBatch> metadata_;
   bool is_tracking_metadata_ = true;
 };

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -479,15 +479,6 @@ void ClientTagBasedModelTypeProcessor::UpdateStorageKey(
   metadata_change_list->UpdateMetadata(storage_key, entity->metadata());
 }
 
-void ClientTagBasedModelTypeProcessor::UntrackEntity(
-    const EntityData& entity_data) {
-  const std::string& client_tag_hash = entity_data.client_tag_hash;
-  DCHECK(model_type_state_.initial_sync_done());
-  DCHECK(!client_tag_hash.empty());
-  DCHECK(GetEntityForTagHash(client_tag_hash)->storage_key().empty());
-  entities_.erase(client_tag_hash);
-}
-
 void ClientTagBasedModelTypeProcessor::UntrackEntityForStorageKey(
     const std::string& storage_key) {
   DCHECK(model_type_state_.initial_sync_done());
@@ -503,6 +494,16 @@ void ClientTagBasedModelTypeProcessor::UntrackEntityForStorageKey(
   storage_key_to_tag_hash_.erase(iter);
 }
 
+void ClientTagBasedModelTypeProcessor::UntrackEntityForClientTagHash(
+    const std::string& client_tag_hash) {
+  DCHECK(model_type_state_.initial_sync_done());
+  DCHECK(!client_tag_hash.empty());
+  // Is a no-op if no entity for |client_tag_hash| is tracked.
+  DCHECK(GetEntityForTagHash(client_tag_hash) == nullptr ||
+         GetEntityForTagHash(client_tag_hash)->storage_key().empty());
+  entities_.erase(client_tag_hash);
+}
+
 bool ClientTagBasedModelTypeProcessor::IsEntityUnsynced(
     const std::string& storage_key) {
   ProcessorEntityTracker* entity = GetEntityForStorageKey(storage_key);

components/sync/model_impl/client_tag_based_model_type_processor.h

@@ -66,8 +66,9 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
   void UpdateStorageKey(const EntityData& entity_data,
                         const std::string& storage_key,
                         MetadataChangeList* metadata_change_list) override;
-  void UntrackEntity(const EntityData& entity_data) override;
   void UntrackEntityForStorageKey(const std::string& storage_key) override;
+  void UntrackEntityForClientTagHash(
+      const std::string& client_tag_hash) override;
   bool IsEntityUnsynced(const std::string& storage_key) override;
   void OnModelStarting(ModelTypeSyncBridge* bridge) override;
   void ModelReadyToSync(std::unique_ptr<MetadataBatch> batch) override;