[USS] Add function for update storage key for processor

components/sync/model_impl/shared_model_type_processor_unittest.cc

Index: components/sync/model_impl/shared_model_type_processor_unittest.cc
diff --git a/components/sync/model_impl/shared_model_type_processor_unittest.cc b/components/sync/model_impl/shared_model_type_processor_unittest.cc
index 819a0bb303aabd6d039f8cea1c0b55eeb9632711..0f388bbbf18be1cd72c65fb16e8ffecb1e563f31 100644
--- a/components/sync/model_impl/shared_model_type_processor_unittest.cc
+++ b/components/sync/model_impl/shared_model_type_processor_unittest.cc
@@ -14,6 +14,7 @@
 #include "base/callback.h"
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
+#include "base/rand_util.h"
 #include "base/run_loop.h"
 #include "components/sync/base/time.h"
 #include "components/sync/engine/activation_context.h"
@@ -48,6 +49,10 @@ const std::string kHash5(FakeModelTypeSyncBridge::TagHashFromKey(kKey5));
 // worker/processor will not have been initialized and thus empty.
 const EntitySpecifics kEmptySpecifics;
 
+const int invalidStorageKeySize = 64;
+
+const std::string InvalidStorageKeyPrefix = "InvalidStorageKey";
+
 EntitySpecifics GenerateSpecifics(const std::string& key,
                                   const std::string& value) {
   return FakeModelTypeSyncBridge::GenerateSpecifics(key, value);
@@ -58,6 +63,15 @@ std::unique_ptr<EntityData> GenerateEntityData(const std::string& key,
   return FakeModelTypeSyncBridge::GenerateEntityData(key, value);
 }
 
+std::string GenerateInvalidStorageKey() {
+  return InvalidStorageKeyPrefix +
+         base::RandBytesAsString(invalidStorageKeySize);
+}
+
+bool IsInvalidStorageKey(const std::string& storage_key) {
+  return 0 == storage_key.find(InvalidStorageKeyPrefix);
+}
+
 class TestModelTypeSyncBridge : public FakeModelTypeSyncBridge {
  public:
   TestModelTypeSyncBridge()
@@ -101,13 +115,83 @@ class TestModelTypeSyncBridge : public FakeModelTypeSyncBridge {
   // FakeModelTypeSyncBridge overrides.
 
   base::Optional<ModelError> MergeSyncData(

[pavely, 2017/05/09 00:49:25]

The code where this function is copied from had simpler structure:
- loop through local storage to pass local only entities to processor
- loop through remote data to update local storage.

In this implementation "// Store any new remote entities." section is more
complicated.

What if you restructure it into three sections:
- loop through remote data to update storage key for entities with random one
- loop through local storage to pass local only entities to processor
- loop through remote data to update local storage.
(Note, last two bullets are exactly the same as before)

This way implementation is cleaner and allows you to refactor function to avoid
code duplication. It also allows you not to expose error_next_ and
ApplyMetadataChangeList from base class.

[Gang Wu, 2017/05/09 22:33:15]

Done.

-      std::unique_ptr<MetadataChangeList> mcl,
+      std::unique_ptr<MetadataChangeList> metadata_changes,
       EntityDataMap entity_data_map) override {
     merge_call_count_++;
-    return FakeModelTypeSyncBridge::MergeSyncData(std::move(mcl),
-                                                  entity_data_map);
+
+    if (error_next_) {
+      error_next_ = false;
+      return ModelError(FROM_HERE, "boom");
+    }
+
+    std::map<std::string, std::string> updated_key;
+
+    // Store any new remote entities.
+    for (const auto& kv : entity_data_map) {
+      EXPECT_FALSE(kv.second->is_deleted());
+      std::string storage_key = kv.first;
+      if (IsInvalidStorageKey(storage_key)) {
+        change_processor()->UpdateStorageKey(
+            storage_key, kv.second.value().specifics.preference().name(),
+            metadata_changes.get());
+        updated_key[storage_key] =
+            kv.second.value().specifics.preference().name();
+        storage_key = kv.second.value().specifics.preference().name();
+      }
+      db_->PutData(storage_key, kv.second.value());
+    }
+
+    for (const auto& kv : updated_key) {
+      entity_data_map[kv.first] = entity_data_map[kv.second];
+      entity_data_map.erase(kv.second);
   }
 

[pavely, 2017/05/09 00:49:25]

nit: Indentation is off in the next few lines.

[Gang Wu, 2017/05/09 22:33:15]

Done.

+  // Commit any local entities that aren't being overwritten by the server.
+  for (const auto& kv : db_->all_data()) {
+    if (entity_data_map.find(kv.first) == entity_data_map.end()) {
+      change_processor()->Put(
+          kv.first, FakeModelTypeSyncBridge::CopyEntityData(*kv.second),
+          metadata_changes.get());
+    }
+  }
+  ApplyMetadataChangeList(std::move(metadata_changes));
+  return {};
+  }
+
+  base::Optional<ModelError> ApplySyncChanges(

[pavely, 2017/05/09 00:49:24]

Same as above, is there a way to refactor this function so that you don't have
to copy implementation of FakeModelTypeSyncBridge::ApplySyncChanges and
introduce code duplication?

[Gang Wu, 2017/05/09 22:33:15]

Done.

+      std::unique_ptr<MetadataChangeList> metadata_changes,
+      EntityChangeList entity_changes) override {
+    if (error_next_) {
+      error_next_ = false;
+      return ModelError(FROM_HERE, "boom");
+    }
+
+    for (const EntityChange& change : entity_changes) {
+      std::string storage_key = change.storage_key();
+      if (IsInvalidStorageKey(storage_key)) {
+        change_processor()->UpdateStorageKey(
+            storage_key, change.data().specifics.preference().name(),
+            metadata_changes.get());
+      }
+      storage_key = change.data().specifics.preference().name();
+      switch (change.type()) {
+        case EntityChange::ACTION_ADD:
+          EXPECT_FALSE(db_->HasData(storage_key));
+          db_->PutData(storage_key, change.data());
+          break;
+        case EntityChange::ACTION_UPDATE:
+          EXPECT_TRUE(db_->HasData(change.storage_key()));
+          db_->PutData(change.storage_key(), change.data());
+          break;
+        case EntityChange::ACTION_DELETE:
+          EXPECT_TRUE(db_->HasData(change.storage_key()));
+          db_->RemoveData(change.storage_key());
+          break;
+      }
+    }
+    ApplyMetadataChangeList(std::move(metadata_changes));
+    return {};
+  }
   void GetData(StorageKeyList keys, DataCallback callback) override {
     if (synchronous_data_callback_) {
       synchronous_data_callback_ = false;
@@ -119,6 +203,13 @@ class TestModelTypeSyncBridge : public FakeModelTypeSyncBridge {
     }
   }
 
+  std::string GetStorageKey(const EntityData& entity_data) override {
+    if (db().HasData(entity_data.specifics.preference().name())) {

[pavely, 2017/05/09 00:49:25]

Alternatively GetStorageKey could generate either real or random storage key
based on entity data. You could for example partition preference names into
groups by first letter of preference name:
- [a-k]: return preference name
- [l-z]: generate random key

[Gang Wu, 2017/05/09 22:33:15]

Done.

+      return entity_data.specifics.preference().name();
+    }
+    return GenerateInvalidStorageKey();
+  }
+
  private:
   void CaptureDataCallback(DataCallback callback,
                            std::unique_ptr<DataBatch> data) {