Use MetadataChangeList and EntityChangeList in SharedModelTypeProcessor

sync/internal_api/shared_model_type_processor.cc

Index: sync/internal_api/shared_model_type_processor.cc
diff --git a/sync/internal_api/shared_model_type_processor.cc b/sync/internal_api/shared_model_type_processor.cc
index a6ed683bfcb17dc93bb37882dbaa7eea20417461..ab05e725e8b5565a38b747b4dccb5684c1d7f3c4 100644
--- a/sync/internal_api/shared_model_type_processor.cc
+++ b/sync/internal_api/shared_model_type_processor.cc
@@ -151,8 +151,6 @@ void SharedModelTypeProcessor::OnConnect(scoped_ptr<CommitQueue> worker) {
 void SharedModelTypeProcessor::Put(const std::string& client_tag,
                                    scoped_ptr<EntityData> entity_data,
                                    MetadataChangeList* metadata_change_list) {
-  // TODO(skym): Add metadata to persist to MetadataChangeList, crbug/569636.
-
   DCHECK(entity_data.get());
   DCHECK(!entity_data->is_deleted());
   DCHECK(!entity_data->non_unique_name.empty());
@@ -171,7 +169,7 @@ void SharedModelTypeProcessor::Put(const std::string& client_tag,
   ModelTypeEntity* entity = nullptr;
   // TODO(stanisc): crbug/561818: Search by client_tag rather than
   // client_tag_hash.
-  EntityMap::const_iterator it = entities_.find(client_tag_hash);
+  auto it = entities_.find(client_tag_hash);
   if (it == entities_.end()) {
     // The service is creating a new entity.
     scoped_ptr<ModelTypeEntity> scoped_entity = ModelTypeEntity::CreateNew(
@@ -185,29 +183,35 @@ void SharedModelTypeProcessor::Put(const std::string& client_tag,
   }
 
   entity->MakeLocalChange(std::move(entity_data), now);
+  metadata_change_list->UpdateMetadata(client_tag, entity->metadata());
+
   FlushPendingCommitRequests();
 }
 
 void SharedModelTypeProcessor::Delete(
     const std::string& client_key,
     MetadataChangeList* metadata_change_list) {
-  // TODO(skym): Add metadata to persist to MetadataChangeList, crbug/569636.
-
   const std::string client_tag_hash(
       syncer::syncable::GenerateSyncableHash(type_, client_key));
 
   // TODO(skym): crbug/561818: Search by client_tag rather than client_tag_hash.
-  EntityMap::const_iterator it = entities_.find(client_tag_hash);
+  auto it = entities_.find(client_tag_hash);
   if (it == entities_.end()) {
     // That's unusual, but not necessarily a bad thing.
     // Missing is as good as deleted as far as the model is concerned.
     DLOG(WARNING) << "Attempted to delete missing item."
                   << " client tag: " << client_key;
-  } else {
-    ModelTypeEntity* entity = it->second.get();
-    entity->Delete();
+    return;
   }
 
+  ModelTypeEntity* entity = it->second.get();
+  entity->Delete();
+
+  // TODO(stanisc): crbug/573333: Delete optimization.

[skym, 2016/01/07 00:40:48]

Reword this sentence. It reads as if you're supposed to delete/remove some
optimization logic, which isn't what you intended.

[stanisc, 2016/01/11 19:56:47]

Done.

+  // A local only metadataentry that has never been committed to the worker

[skym, 2016/01/07 00:40:48]

I'm a bit worried about this entire approach. Trying to know right here (and in
all the other locations) what things are safe to delete and what things are not
safe seems like a good way to end up with leaks. Both memory leaks at runtime,
and leaks in our persistent storage.

How are we going to even know if something has been sent to the worker?

[stanisc, 2016/01/11 19:56:47]

Good question. I guess an alternative approach is to always send all deletes to
the worker regardless of whether the server has seen an item or not. The worker
then will decide whether this item has a server ID or not. If the item doesn't
have server it could be deleted and commit confirmation generated for it right
away. I guess that should allow to have this kind of logic in commit
confirmation code only. I'll remove this comment.

[skym, 2016/01/11 20:45:40]

Oooh, interesting, I like that idea. Can you make sure we have a bug tracking
this idea?

[stanisc, 2016/01/12 19:32:11]

This is still tracked by 573333. I've added comments to the bug.

+  // should be cleared at this point.
+  metadata_change_list->UpdateMetadata(client_key, entity->metadata());
+
   FlushPendingCommitRequests();
 }
 
@@ -223,8 +227,7 @@ void SharedModelTypeProcessor::FlushPendingCommitRequests() {
     return;
 
   // TODO(rlarocque): Do something smarter than iterate here.
-  for (EntityMap::const_iterator it = entities_.begin(); it != entities_.end();
-       ++it) {
+  for (auto it = entities_.begin(); it != entities_.end(); ++it) {
     if (it->second->RequiresCommitRequest()) {
       CommitRequestData request;
       it->second->InitializeCommitRequestData(&request);
@@ -240,14 +243,18 @@ void SharedModelTypeProcessor::FlushPendingCommitRequests() {
 void SharedModelTypeProcessor::OnCommitCompleted(
     const DataTypeState& type_state,
     const CommitResponseDataList& response_list) {
+  scoped_ptr<MetadataChangeList> change_list =
+      service_->CreateMetadataChangeList();
+
   data_type_state_ = type_state;
+  change_list->UpdateDataTypeState(data_type_state_);
 
-  for (CommitResponseDataList::const_iterator list_it = response_list.begin();
-       list_it != response_list.end(); ++list_it) {
+  for (auto list_it = response_list.begin(); list_it != response_list.end();
+       ++list_it) {
     const CommitResponseData& response_data = *list_it;
     const std::string& client_tag_hash = response_data.client_tag_hash;
 
-    EntityMap::const_iterator it = entities_.find(client_tag_hash);
+    auto it = entities_.find(client_tag_hash);
     if (it == entities_.end()) {
       NOTREACHED() << "Received commit response for missing item."
                    << " type: " << type_ << " client_tag: " << client_tag_hash;
@@ -256,21 +263,34 @@ void SharedModelTypeProcessor::OnCommitCompleted(
       it->second->ReceiveCommitResponse(
           response_data.id, response_data.sequence_number,
           response_data.response_version, data_type_state_.encryption_key_name);
+      // TODO(stanisc): crbug/573333: Delete case.

[skym, 2016/01/07 00:40:48]

Lots of crbugs in here, it looks like the prefered style is either
crbug.com/123456 or https://crbug.com/12345, see
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/whBCkcY8xtA

[stanisc, 2016/01/11 19:56:47]

Done.

+      // This might be the right place to clear a metadata entry that has
+      // been deleted locally and confirmed deleted by the server.
+      change_list->UpdateMetadata(it->second->client_key(),
+                                  it->second->metadata());
     }
   }
+
+  // TODO(stanisc): What is the right method to submit metadata changes to the
+  // service? Is using empty EntityChangeList OK?
+  service_->ApplySyncChanges(std::move(change_list), EntityChangeList());

[skym, 2016/01/07 00:40:48]

This seems reasonable. Make sure you update model_type_service.h's comment
making this expectation explicit.

[stanisc, 2016/01/11 19:56:47]

Done.

 }
 
 void SharedModelTypeProcessor::OnUpdateReceived(
     const DataTypeState& data_type_state,
     const UpdateResponseDataList& response_list,
     const UpdateResponseDataList& pending_updates) {
+  scoped_ptr<MetadataChangeList> change_list =

[pavely, 2016/01/07 20:51:46]

Variable names are confusing. Both of them have ChangeList in type name. Maybe
rename change_list to metadata_changes to contrast entity_changes.

[stanisc, 2016/01/11 19:56:48]

OK, renamed to metadata_changes.

+      service_->CreateMetadataChangeList();
+  EntityChangeList entity_changes;
+
   bool got_new_encryption_requirements = data_type_state_.encryption_key_name !=

[skym, 2016/01/07 00:40:48]

Can we move this variable down to where we actually use it? Or maybe just inline
the whole expression?

[stanisc, 2016/01/11 19:56:47]

This needs to be assigned before data_type_state_ is overwritten. 

                                          data_type_state.encryption_key_name;
 
   data_type_state_ = data_type_state;
 
-  for (UpdateResponseDataList::const_iterator list_it = response_list.begin();
-       list_it != response_list.end(); ++list_it) {
+  for (auto list_it = response_list.begin(); list_it != response_list.end();
+       ++list_it) {
     const UpdateResponseData& response_data = *list_it;
     const EntityData& data = response_data.entity.value();
     const std::string& client_tag_hash = data.client_tag_hash;
@@ -280,24 +300,47 @@ void SharedModelTypeProcessor::OnUpdateReceived(
     pending_updates_map_.erase(client_tag_hash);
 
     ModelTypeEntity* entity = nullptr;
-    EntityMap::const_iterator it = entities_.find(client_tag_hash);
+    auto it = entities_.find(client_tag_hash);
     if (it == entities_.end()) {
+      if (data.is_deleted()) {
+        DLOG(WARNING) << "Received remote delete for a non-existing item."
+                      << " client_tag_hash: " << client_tag_hash;
+        continue;
+      }
+
       // Let the service define |client_tag| based on the entity data.
-      std::string client_tag = service_->GetClientTag(data);
+      std::string client_key = service_->GetClientTag(data);
 
       scoped_ptr<ModelTypeEntity> scoped_entity = ModelTypeEntity::CreateNew(
-          client_tag, client_tag_hash, data.id, data.creation_time);
+          client_key, client_tag_hash, data.id, data.creation_time);
       entity = scoped_entity.get();
       entities_.insert(
           std::make_pair(client_tag_hash, std::move(scoped_entity)));
 
+      entity_changes.push_back(
+          EntityChange::CreateAdd(client_key, response_data.entity));
+
     } else {
       entity = it->second.get();
+      if (data.is_deleted()) {
+        entity_changes.push_back(
+            EntityChange::CreateDelete(entity->client_key()));
+      } else {
+        // TODO(stanisc): crbug/561829: Avoid sending an update to the service

[skym, 2016/01/07 00:40:48]

This same TODO and crbug should be on line ~181 as well, right?

[stanisc, 2016/01/11 19:56:47]

Done.

+        // if there is no actual change.
+        entity_changes.push_back(EntityChange::CreateUpdate(
+            entity->client_key(), response_data.entity));
+      }
     }
 
     entity->ApplyUpdateFromServer(response_data);
+    // TODO(stanisc): crbug/573333: Delete case.
+    // This might be the right place to clear metadata entry instead of
+    // updating it.
+    change_list->UpdateMetadata(entity->client_key(), entity->metadata());
 
-    // TODO(stanisc): Do something special when conflicts are detected.
+    // TODO(stanisc): crbug/521867: Do something special when conflicts are
+    // detected.
 
     // If the received entity has out of date encryption, we schedule another
     // commit to fix it.
@@ -306,20 +349,20 @@ void SharedModelTypeProcessor::OnUpdateReceived(
       DVLOG(2) << ModelTypeToString(type_) << ": Requesting re-encrypt commit "
                << response_data.encryption_key_name << " -> "
                << data_type_state_.encryption_key_name;
-      EntityMap::const_iterator it2 = entities_.find(client_tag_hash);
+      auto it2 = entities_.find(client_tag_hash);
       it2->second->UpdateDesiredEncryptionKey(
           data_type_state_.encryption_key_name);
     }
   }
 
+  // TODO: crbug/529498: stop saving pending updates.
   // Save pending updates in the appropriate data structure.
-  for (UpdateResponseDataList::const_iterator list_it = pending_updates.begin();
-       list_it != pending_updates.end(); ++list_it) {
+  for (auto list_it = pending_updates.begin(); list_it != pending_updates.end();
+       ++list_it) {
     const UpdateResponseData& update = *list_it;
     const std::string& client_tag_hash = update.entity->client_tag_hash;
 
-    UpdateMap::const_iterator lookup_it =
-        pending_updates_map_.find(client_tag_hash);
+    auto lookup_it = pending_updates_map_.find(client_tag_hash);
     if (lookup_it == pending_updates_map_.end()) {
       pending_updates_map_.insert(std::make_pair(
           client_tag_hash, make_scoped_ptr(new UpdateResponseData(update))));
@@ -333,8 +376,7 @@ void SharedModelTypeProcessor::OnUpdateReceived(
   }
 
   if (got_new_encryption_requirements) {
-    for (EntityMap::const_iterator it = entities_.begin();
-         it != entities_.end(); ++it) {
+    for (auto it = entities_.begin(); it != entities_.end(); ++it) {
       it->second->UpdateDesiredEncryptionKey(
           data_type_state_.encryption_key_name);
     }
@@ -343,33 +385,35 @@ void SharedModelTypeProcessor::OnUpdateReceived(
   // We may have new reasons to commit by the time this function is done.

[skym, 2016/01/07 00:40:48]

We do? Why's that exactly?

[stanisc, 2016/01/11 19:56:48]

This comment was here before. This has something to do with the re-encryption
when the key changes.

   FlushPendingCommitRequests();

[pavely, 2016/01/07 20:51:46]

If Flush... is needed then it probably should be last thing in the function.

[stanisc, 2016/01/11 19:56:48]

Done.

 
-  // TODO(rlarocque): Inform the model of the new or updated data.
-  // TODO(rlarocque): Persist the new data on disk.
+  // Inform the service of the new or updated data.
+  if (!entity_changes.empty()) {

[pavely, 2016/01/07 20:51:46]

Could you DCHECK that if entity_changes are empty then change_list is empty as
well?

[skym, 2016/01/07 21:31:23]

Is this really true? Once we filter out via specifics hash (crbug.com/561829) I
think should be able to have empty entity changes and non-empty metadata
changes.

[stanisc, 2016/01/11 19:56:47]

I am not sure. I'll remove the condition for now. We can always optimize this
later.

+    service_->ApplySyncChanges(std::move(change_list), entity_changes);
+  }
 }
 
 UpdateResponseDataList SharedModelTypeProcessor::GetPendingUpdates() {
   UpdateResponseDataList pending_updates_list;
-  for (UpdateMap::const_iterator it = pending_updates_map_.begin();
-       it != pending_updates_map_.end(); ++it) {
+  for (auto it = pending_updates_map_.begin(); it != pending_updates_map_.end();
+       ++it) {
     pending_updates_list.push_back(*it->second);
   }
   return pending_updates_list;
 }
 
 void SharedModelTypeProcessor::ClearTransientSyncState() {
-  for (EntityMap::const_iterator it = entities_.begin(); it != entities_.end();
-       ++it) {
+  for (auto it = entities_.begin(); it != entities_.end(); ++it) {
     it->second->ClearTransientSyncState();
   }
 }
 
 void SharedModelTypeProcessor::ClearSyncState() {
-  for (EntityMap::const_iterator it = entities_.begin(); it != entities_.end();
-       ++it) {
+  for (auto it = entities_.begin(); it != entities_.end(); ++it) {
     it->second->ClearSyncState();
   }
   pending_updates_map_.clear();
   data_type_state_ = DataTypeState();
+  // TODO(stanisc): crbug/561830, crbug/573333: Update the service to let it
+  // know that all metadata need to be cleared from the storage.
 }
 
 }  // namespace syncer_v2