[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 e24917d5a505b915358c3b3a1bb7f5c3e688e419..322c6ca5320b660e0ec96e4deec24d7664c8dd69 100644
--- a/components/sync/model_impl/shared_model_type_processor.cc
+++ b/components/sync/model_impl/shared_model_type_processor.cc
@@ -133,8 +133,8 @@ void SharedModelTypeProcessor::DisableSync() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   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.
@@ -193,8 +193,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();
   }
 }
 
@@ -260,26 +260,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() {
@@ -294,8 +289,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);
@@ -372,10 +367,15 @@ void SharedModelTypeProcessor::OnUpdateReceived(
     ProcessorEntityTracker* entity = ProcessUpdate(update, &entity_changes);
 
     if (!entity) {
-      // The update should be ignored.
+      // The update is either tombstone of entity that didn't exist locally or
+      // reflection, thus should be ignored.
+      continue;
+    }
+    if (entity->storage_key().empty()) {
+      // Storage key of this entity is not known yet. Don't update metadata, it
+      // will be done from UpdateStorageKey.
       continue;
     }
-
     if (entity->CanClearMetadata()) {
       metadata_changes->ClearMetadata(entity->storage_key());
       storage_key_to_tag_hash_.erase(entity->storage_key());
@@ -391,19 +391,24 @@ void SharedModelTypeProcessor::OnUpdateReceived(
   }
 
   if (got_new_encryption_requirements) {
+    // TODO(pavely): Currently we recommit all entities. We should instead
+    // recommit only the ones whose encryption key doesn't match the one in
+    // DataTypeState. Work is tracked in http://crbug.com/727874.
     RecommitAllForEncryption(already_updated, metadata_changes.get());
   }
-
   // Inform the bridge of the new or updated data.
   base::Optional<ModelError> error =
       bridge_->ApplySyncChanges(std::move(metadata_changes), entity_changes);
-
   if (error) {
     ReportError(error.value());
-  } else {
-    // There may be new reasons to commit by the time this function is done.
-    FlushPendingCommitRequests();
+    return;
   }
+
+  // If there were trackers with empty storage keys, they should have been
+  // updated by bridge as part of ApplySyncChanges.
+  DCHECK(AllStorageKeysPopulated());
+  // There may be new reasons to commit by the time this function is done.
+  FlushPendingCommitRequests();
 }
 
 ProcessorEntityTracker* SharedModelTypeProcessor::ProcessUpdate(
@@ -412,40 +417,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);
   }
 
@@ -543,9 +551,14 @@ 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();
-    if (already_updated.find(entity->storage_key()) != already_updated.end()) {
+  for (const auto& kv : entities_) {
+    ProcessorEntityTracker* entity = kv.second.get();
+    if (entity->storage_key().empty() ||
+        (already_updated.find(entity->storage_key()) !=
+         already_updated.end())) {
+      // Entities with empty storage key were already processed. ProcessUpdate()
+      // incremented their sequence numbers and cached commit data. Their
+      // metadata will be persisted in UpdateStorageKey().
       continue;
     }
     entity->IncrementSequenceNumber();
@@ -574,7 +587,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,22 +599,27 @@ void SharedModelTypeProcessor::OnInitialUpdateReceived(
       continue;
     }
     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;
+    const std::string& storage_key = entity->storage_key();
+    entity_data.push_back(EntityChange::CreateAdd(storage_key, update.entity));
+    if (!storage_key.empty())
+      metadata_changes->UpdateMetadata(storage_key, entity->metadata());
   }
 
   // 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 (error) {
     ReportError(error.value());
-  } else {
-    // We may have new reasons to commit by the time this function is done.
-    FlushPendingCommitRequests();
+    return;
   }
+
+  // If there were trackers with empty storage keys, they should have been
+  // updated by bridge as part of MergeSyncData.
+  DCHECK(AllStorageKeysPopulated());
+
+  // We may have new reasons to commit by the time this function is done.
+  FlushPendingCommitRequests();
 }
 
 void SharedModelTypeProcessor::OnInitialPendingDataLoaded(
@@ -673,14 +691,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;
 }
 
@@ -688,7 +708,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 {