[Sync] Add a sanity integration test for USS.

components/sync/engine_impl/model_type_worker.cc

Index: components/sync/engine_impl/model_type_worker.cc
diff --git a/components/sync/engine_impl/model_type_worker.cc b/components/sync/engine_impl/model_type_worker.cc
index d428cfbffd064989405bed581c140dfe04f4fba7..b308c6064f4aad32857e34db19877b09e97d67e8 100644
--- a/components/sync/engine_impl/model_type_worker.cc
+++ b/components/sync/engine_impl/model_type_worker.cc
@@ -123,9 +123,6 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
     const std::string& client_tag_hash =
         update_entity->client_defined_unique_tag();
 
-    // TODO(stanisc): crbug.com/516866: this wouldn't be true for bookmarks.

[skym, 2016/09/12 19:57:50]

Did you mean to strip out this TODO?

[maxbogue, 2016/09/13 03:44:54]

I thought I should but I was wrong. Re-added.

-    DCHECK(!client_tag_hash.empty());
-
     // Prepare the message for the model thread.
     EntityData data;
     data.id = update_entity->id_string();
@@ -139,8 +136,19 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
 
     WorkerEntityTracker* entity = GetOrCreateEntityTracker(data);

[skym, 2016/09/12 19:57:50]

As discussed offline, this doesn't always work if the worker doesn't have the
mapping in cache yet, or the client has never seen the entity before getting a
tombstone.

[maxbogue, 2016/09/13 03:44:54]

I've reverted the search by ID portion of this change and done the FakeServer
fix instead.

 
+    if (data.client_tag_hash.empty()) {
+      data.client_tag_hash = entity->client_tag_hash();
+    }
+

[skym, 2016/09/12 19:57:50]

I personally think you should remove this newline.

[maxbogue, 2016/09/13 03:44:54]

Done.

+    DCHECK(!data.client_tag_hash.empty());
+
+    // Deleted entities must use the default instance of EntitySpecifics in

[skym, 2016/09/12 19:57:50]

Why? Says who? What's expecting the default instance? This is handling a GU
right? So this data is about to go to the proc, then the service.

[maxbogue, 2016/09/13 03:44:55]

https://cs.chromium.org/chromium/src/components/sync/api/entity_data.h?l=71

+    // order for EntityData to correctly reflect that they are deleted.
+    const sync_pb::EntitySpecifics& specifics =
+        update_entity->deleted() ? sync_pb::EntitySpecifics::default_instance()
+                                 : update_entity->specifics();
+
     // Check if specifics are encrypted and try to decrypt if so.
-    const sync_pb::EntitySpecifics& specifics = update_entity->specifics();
     if (!specifics.has_encrypted()) {
       // No encryption.
       entity->ReceiveUpdate(update_entity->version());
@@ -433,6 +441,14 @@ WorkerEntityTracker* ModelTypeWorker::CreateEntityTracker(
 WorkerEntityTracker* ModelTypeWorker::GetOrCreateEntityTracker(
     const EntityData& data) {
   WorkerEntityTracker* entity = GetEntityTracker(data.client_tag_hash);
+  if (!entity && !data.id.empty()) {
+    // Fall back to iterative search by id.

[skym, 2016/09/12 19:57:50]

Can you explain why this shouldn't happen very often and why it doesn't make
sense to maintain a second mapping.

[skym, 2016/09/12 19:59:30]

Err, I wasn't very clear, I want this explanation in a comment in the code.

[maxbogue, 2016/09/13 03:44:55]

I've removed this.

+    for (auto& kv : entities_) {
+      if (kv.second->id() == data.id) {
+        entity = kv.second.get();
+      }
+    }
+  }
   return entity ? entity : CreateEntityTracker(data);
 }