[Sync] Implement support for updating storage key for new entities

components/sync/model_impl/shared_model_type_processor.cc

Index: components/sync/model_impl/shared_model_type_processor.cc
diff --git a/components/sync/model_impl/shared_model_type_processor.cc b/components/sync/model_impl/shared_model_type_processor.cc
index f7ecbdebecb2385e1fc866d53469d7fc616a8dbf..da660a6d9c916f39e691df17776798cdc2eef997 100644
--- a/components/sync/model_impl/shared_model_type_processor.cc
+++ b/components/sync/model_impl/shared_model_type_processor.cc
@@ -131,8 +131,8 @@ void SharedModelTypeProcessor::DisableSync() {
   DCHECK(CalledOnValidThread());
   std::unique_ptr<MetadataChangeList> change_list =
       bridge_->CreateMetadataChangeList();
-  for (auto it = entities_.begin(); it != entities_.end(); ++it) {
-    change_list->ClearMetadata(it->second->storage_key());
+  for (const auto& kv : entities_) {
+    change_list->ClearMetadata(kv.second->storage_key());
   }
   change_list->ClearModelTypeState();
   // Nothing to do if this fails, so just ignore the error it might return.
@@ -191,8 +191,8 @@ void SharedModelTypeProcessor::DisconnectSync() {
   weak_ptr_factory_.InvalidateWeakPtrs();
   worker_.reset();
 
-  for (auto it = entities_.begin(); it != entities_.end(); ++it) {
-    it->second->ClearTransientSyncState();
+  for (const auto& kv : entities_) {
+    kv.second->ClearTransientSyncState();
   }
 }
 
@@ -258,26 +258,21 @@ void SharedModelTypeProcessor::Delete(
 }
 
 void SharedModelTypeProcessor::UpdateStorageKey(
-    const std::string& old_storage_key,
-    const std::string& new_storage_key,
+    const EntityData& entity_data,
+    const std::string& storage_key,
     MetadataChangeList* metadata_change_list) {
-  ProcessorEntityTracker* entity = GetEntityForStorageKey(old_storage_key);
-  if (entity == nullptr) {
-    DLOG(WARNING) << "Attempted to update missing item."
-                  << " storage key: " << old_storage_key;
-    return;
-  }
-
-  entity->SetStorageKey(new_storage_key);
+  const std::string& client_tag_hash = entity_data.client_tag_hash;
+  DCHECK(!client_tag_hash.empty());
+  ProcessorEntityTracker* entity = GetEntityForTagHash(client_tag_hash);
+  DCHECK(entity);
 
-  // Update storage key to tag hash map.
-  storage_key_to_tag_hash_.erase(old_storage_key);
-  storage_key_to_tag_hash_[new_storage_key] =
-      entity->metadata().client_tag_hash();
+  DCHECK(entity->storage_key().empty());
+  DCHECK(storage_key_to_tag_hash_.find(storage_key) ==
+         storage_key_to_tag_hash_.end());
 
-  // Update metadata store.
-  metadata_change_list->ClearMetadata(old_storage_key);
-  metadata_change_list->UpdateMetadata(new_storage_key, entity->metadata());
+  storage_key_to_tag_hash_[storage_key] = client_tag_hash;
+  entity->SetStorageKey(storage_key);
+  metadata_change_list->UpdateMetadata(storage_key, entity->metadata());
 }
 
 void SharedModelTypeProcessor::FlushPendingCommitRequests() {
@@ -292,8 +287,8 @@ void SharedModelTypeProcessor::FlushPendingCommitRequests() {
     return;
 
   // TODO(rlarocque): Do something smarter than iterate here.
-  for (auto it = entities_.begin(); it != entities_.end(); ++it) {
-    ProcessorEntityTracker* entity = it->second.get();
+  for (const auto& kv : entities_) {
+    ProcessorEntityTracker* entity = kv.second.get();
     if (entity->RequiresCommitRequest() && !entity->RequiresCommitData()) {
       CommitRequestData request;
       entity->InitializeCommitRequestData(&request);
@@ -374,6 +369,8 @@ void SharedModelTypeProcessor::OnUpdateReceived(
       continue;
     }
 

[skym, 2017/05/31 19:53:11]

The newlines seem a bit odd here. Maybe group the continues together?

[pavely, 2017/06/02 18:23:46]

Done.

+    if (entity->storage_key().empty())
+      continue;
     if (entity->CanClearMetadata()) {
       metadata_changes->ClearMetadata(entity->storage_key());
       storage_key_to_tag_hash_.erase(entity->storage_key());
@@ -396,6 +393,10 @@ void SharedModelTypeProcessor::OnUpdateReceived(
   base::Optional<ModelError> error =
       bridge_->ApplySyncChanges(std::move(metadata_changes), entity_changes);
 
+  // If there were trackers with empty storage keys, they should have been
+  // updated by bridge as part of ApplySyncChanges.
+  DCHECK(AllStorageKeysPopulated());
+
   if (error) {
     ReportError(error.value());
   } else {
@@ -410,40 +411,43 @@ ProcessorEntityTracker* SharedModelTypeProcessor::ProcessUpdate(
   const EntityData& data = update.entity.value();
   const std::string& client_tag_hash = data.client_tag_hash;
   ProcessorEntityTracker* entity = GetEntityForTagHash(client_tag_hash);
-  if (entity == nullptr) {
-    if (data.is_deleted()) {
-      DLOG(WARNING) << "Received remote delete for a non-existing item."
-                    << " client_tag_hash: " << client_tag_hash;
-      return nullptr;
-    }
 
-    entity = CreateEntity(data);
-    entity_changes->push_back(
-        EntityChange::CreateAdd(entity->storage_key(), update.entity));
-    entity->RecordAcceptedUpdate(update);
-  } else if (entity->UpdateIsReflection(update.response_version)) {
+  // Handle corner cases first.
+  if (entity == nullptr && data.is_deleted()) {
+    // Local entity doesn't exist and update is tombstone.
+    DLOG(WARNING) << "Received remote delete for a non-existing item."
+                  << " client_tag_hash: " << client_tag_hash;
+    return nullptr;
+  }
+  if (entity && entity->UpdateIsReflection(update.response_version)) {
     // Seen this update before; just ignore it.
     return nullptr;
-  } else if (entity->IsUnsynced()) {
+  }
+
+  if (entity && entity->IsUnsynced()) {
+    // Handle conflict resolution.
     ConflictResolution::Type resolution_type =
         ResolveConflict(update, entity, entity_changes);
     UMA_HISTOGRAM_ENUMERATION("Sync.ResolveConflict", resolution_type,
                               ConflictResolution::TYPE_SIZE);
-  } else if (data.is_deleted()) {
-    // The entity was deleted; inform the bridge. Note that the local data
-    // can never be deleted at this point because it would have either been
-    // acked (the add case) or pending (the conflict case).
-    DCHECK(!entity->metadata().is_deleted());
-    entity_changes->push_back(
-        EntityChange::CreateDelete(entity->storage_key()));
-    entity->RecordAcceptedUpdate(update);
-  } else if (!entity->MatchesData(data)) {
-    // Specifics have changed, so update the bridge.
-    entity_changes->push_back(
-        EntityChange::CreateUpdate(entity->storage_key(), update.entity));
-    entity->RecordAcceptedUpdate(update);
   } else {
-    // No data change; still record that the update was received.
+    // Handle simple create/delete/update.
+    if (entity == nullptr) {
+      entity = CreateEntity(data);
+      entity_changes->push_back(
+          EntityChange::CreateAdd(entity->storage_key(), update.entity));
+    } else if (data.is_deleted()) {
+      // The entity was deleted; inform the bridge. Note that the local data
+      // can never be deleted at this point because it would have either been
+      // acked (the add case) or pending (the conflict case).
+      DCHECK(!entity->metadata().is_deleted());
+      entity_changes->push_back(
+          EntityChange::CreateDelete(entity->storage_key()));
+    } else if (!entity->MatchesData(data)) {
+      // Specifics have changed, so update the bridge.
+      entity_changes->push_back(
+          EntityChange::CreateUpdate(entity->storage_key(), update.entity));
+    }
     entity->RecordAcceptedUpdate(update);
   }
 
@@ -541,8 +545,8 @@ void SharedModelTypeProcessor::RecommitAllForEncryption(
     MetadataChangeList* metadata_changes) {
   ModelTypeSyncBridge::StorageKeyList entities_needing_data;
 
-  for (auto it = entities_.begin(); it != entities_.end(); ++it) {
-    ProcessorEntityTracker* entity = it->second.get();
+  for (const auto& kv : entities_) {
+    ProcessorEntityTracker* entity = kv.second.get();
     if (already_updated.find(entity->storage_key()) != already_updated.end()) {
       continue;
     }
@@ -572,7 +576,7 @@ void SharedModelTypeProcessor::OnInitialUpdateReceived(
 
   std::unique_ptr<MetadataChangeList> metadata_changes =
       bridge_->CreateMetadataChangeList();
-  EntityDataMap data_map;
+  EntityChangeList entity_data;
 
   model_type_state_ = model_type_state;
   metadata_changes->UpdateModelTypeState(model_type_state_);
@@ -586,13 +590,18 @@ void SharedModelTypeProcessor::OnInitialUpdateReceived(
     ProcessorEntityTracker* entity = CreateEntity(update.entity.value());
     const std::string& storage_key = entity->storage_key();
     entity->RecordAcceptedUpdate(update);
-    metadata_changes->UpdateMetadata(storage_key, entity->metadata());
-    data_map[storage_key] = update.entity;
+    if (!storage_key.empty())
+      metadata_changes->UpdateMetadata(storage_key, entity->metadata());
+    entity_data.push_back(EntityChange::CreateAdd(storage_key, update.entity));

[skym, 2017/05/31 19:53:11]

I think this would be easier to read if you did the entity_data.push_back first,
and then the if, wdyt?

[pavely, 2017/06/02 18:23:46]

Done.

   }
 
   // Let the bridge handle associating and merging the data.
   base::Optional<ModelError> error =
-      bridge_->MergeSyncData(std::move(metadata_changes), data_map);
+      bridge_->MergeSyncData(std::move(metadata_changes), entity_data);
+
+  // If there were trackers with empty storage keys, they should have been
+  // updated by bridge as part of MergeSyncData.
+  DCHECK(AllStorageKeysPopulated());

[skym, 2017/05/31 19:53:11]

Should this DCHECK be run if we get an error back from merge/apply?

[pavely, 2017/06/02 18:23:45]

Done.

 
   if (error) {
     ReportError(error.value());
@@ -671,14 +680,16 @@ ProcessorEntityTracker* SharedModelTypeProcessor::CreateEntity(
     const std::string& storage_key,
     const EntityData& data) {
   DCHECK(entities_.find(data.client_tag_hash) == entities_.end());
-  DCHECK(storage_key_to_tag_hash_.find(storage_key) ==
-         storage_key_to_tag_hash_.end());
+  DCHECK(!bridge_->SupportsGetStorageKey() || !storage_key.empty());
+  DCHECK(storage_key.empty() || storage_key_to_tag_hash_.find(storage_key) ==
+                                    storage_key_to_tag_hash_.end());
   std::unique_ptr<ProcessorEntityTracker> entity =
       ProcessorEntityTracker::CreateNew(storage_key, data.client_tag_hash,
                                         data.id, data.creation_time);
   ProcessorEntityTracker* entity_ptr = entity.get();
   entities_[data.client_tag_hash] = std::move(entity);
-  storage_key_to_tag_hash_[storage_key] = data.client_tag_hash;
+  if (!storage_key.empty())
+    storage_key_to_tag_hash_[storage_key] = data.client_tag_hash;
   return entity_ptr;
 }
 
@@ -686,7 +697,19 @@ ProcessorEntityTracker* SharedModelTypeProcessor::CreateEntity(
     const EntityData& data) {
   // Verify the tag hash matches, may be relaxed in the future.
   DCHECK_EQ(data.client_tag_hash, GetHashForTag(bridge_->GetClientTag(data)));
-  return CreateEntity(bridge_->GetStorageKey(data), data);
+  std::string storage_key;
+  if (bridge_->SupportsGetStorageKey())
+    storage_key = bridge_->GetStorageKey(data);
+  return CreateEntity(storage_key, data);
+}
+
+bool SharedModelTypeProcessor::AllStorageKeysPopulated() const {
+  for (const auto& kv : entities_) {
+    ProcessorEntityTracker* entity = kv.second.get();
+    if (entity->storage_key().empty())
+      return false;
+  }
+  return true;
 }
 
 size_t SharedModelTypeProcessor::EstimateMemoryUsage() const {