[Sync] Updating processor's Put method to take a scoped_ptr.

BUG=543405

Review URL: https://codereview.chromium.org/1493903002

Cr-Commit-Position: refs/heads/master@{#365310}

6 files changed, 96 insertions, 35 deletions

diff --git a/sync/internal_api/model_type_entity.cc b/sync/internal_api/model_type_entity.cc
index eabdd06..cbfe00c 100644
--- a/sync/internal_api/model_type_entity.cc
+++ b/sync/internal_api/model_type_entity.cc
@@ -92,8 +92,7 @@ void ModelTypeEntity::ApplyUpdateFromServer(
   encryption_key_name_ = response_data.encryption_key_name;
 }
 
-void ModelTypeEntity::MakeLocalChange(const std::string& non_unique_name,
-                                      const sync_pb::EntitySpecifics& specifics,
+void ModelTypeEntity::MakeLocalChange(scoped_ptr<EntityData> entity_data,
                                       base::Time modification_time) {
   DCHECK(metadata_.has_client_tag_hash());
   DCHECK(!metadata_.client_tag_hash().empty());
@@ -102,21 +101,15 @@ void ModelTypeEntity::MakeLocalChange(const std::string& non_unique_name,
   metadata_.set_modification_time(syncer::TimeToProtoTime(modification_time));
   metadata_.set_is_deleted(false);
   IncrementSequenceNumber();
-  UpdateSpecificsHash(specifics);
+  UpdateSpecificsHash(entity_data->specifics);
 
-  // Build and cache commit data.
-  // TODO(stanisc): Consider moving this out to make caching the data
-  // optional.
-  EntityData data;
-  data.client_tag_hash = metadata_.client_tag_hash();
-  data.id = metadata_.server_id();
-  data.non_unique_name = non_unique_name;
-  data.creation_time = syncer::ProtoTimeToTime(metadata_.creation_time());
-  data.modification_time = modification_time;
-  // TODO(stanisc): Consider taking over specifics value without copying.
-  data.specifics.CopyFrom(specifics);
+  entity_data->client_tag_hash = metadata_.client_tag_hash();
+  entity_data->id = metadata_.server_id();
+  entity_data->creation_time =
+      syncer::ProtoTimeToTime(metadata_.creation_time());
+  entity_data->modification_time = modification_time;
 
-  CacheCommitData(&data);
+  CacheCommitData(entity_data.get());
 }
 
 void ModelTypeEntity::UpdateDesiredEncryptionKey(const std::string& name) {
diff --git a/sync/internal_api/model_type_entity_unittest.cc b/sync/internal_api/model_type_entity_unittest.cc
index 0af2b58..08de2fc 100644
--- a/sync/internal_api/model_type_entity_unittest.cc
+++ b/sync/internal_api/model_type_entity_unittest.cc
@@ -57,7 +57,10 @@ class ModelTypeEntityTest : public ::testing::Test {
 
   void MakeLocalChange(ModelTypeEntity* entity,
                        const sync_pb::EntitySpecifics& specifics) {
-    entity->MakeLocalChange("foo", specifics, kMtime);
+    scoped_ptr<EntityData> entity_data = make_scoped_ptr(new EntityData());
+    entity_data->specifics = specifics;
+    entity_data->non_unique_name = "foo";
+    entity->MakeLocalChange(entity_data.Pass(), kMtime);
   }
 
   scoped_ptr<ModelTypeEntity> NewServerItem() {
diff --git a/sync/internal_api/public/model_type_entity.h b/sync/internal_api/public/model_type_entity.h
index 3d0a074..551f1bd 100644
--- a/sync/internal_api/public/model_type_entity.h
+++ b/sync/internal_api/public/model_type_entity.h
@@ -65,8 +65,7 @@ class SYNC_EXPORT ModelTypeEntity {
   void ApplyUpdateFromServer(const UpdateResponseData& response_data);
 
   // Applies a local change to this item.
-  void MakeLocalChange(const std::string& non_unique_name,
-                       const sync_pb::EntitySpecifics& specifics,
+  void MakeLocalChange(scoped_ptr<EntityData> entity_data,
                        base::Time modification_time);
 
   // Schedule a commit if the |name| does not match this item's last known
diff --git a/sync/internal_api/public/shared_model_type_processor.h b/sync/internal_api/public/shared_model_type_processor.h
index 88adbcc..eb47d93 100644
--- a/sync/internal_api/public/shared_model_type_processor.h
+++ b/sync/internal_api/public/shared_model_type_processor.h
@@ -65,8 +65,7 @@ class SYNC_EXPORT SharedModelTypeProcessor : public ModelTypeProcessor,
 
   // ModelTypeChangeProcessor implementation.
   void Put(const std::string& client_key,
-           const std::string& non_unique_name,
-           const sync_pb::EntitySpecifics& specifics,
+           scoped_ptr<EntityData> entity_data,
            MetadataChangeList* metadata_change_list) override;
   void Delete(const std::string& client_key,
               MetadataChangeList* metadata_change_list) override;
diff --git a/sync/internal_api/shared_model_type_processor.cc b/sync/internal_api/shared_model_type_processor.cc
index 97cf8e5..15edd30 100644
--- a/sync/internal_api/shared_model_type_processor.cc
+++ b/sync/internal_api/shared_model_type_processor.cc
@@ -149,16 +149,23 @@ void SharedModelTypeProcessor::OnConnect(scoped_ptr<CommitQueue> worker) {
 }
 
 void SharedModelTypeProcessor::Put(const std::string& client_tag,
-                                   const std::string& non_unique_name,
-                                   const sync_pb::EntitySpecifics& specifics,
+                                   scoped_ptr<EntityData> entity_data,
                                    MetadataChangeList* metadata_change_list) {
-  // TODO(skym): Update for new approach. Different objects, different caching,
-  // different loopups, metadata_change_list, etc.
+  // TODO(skym): Add metadata to persist to MetadataChangeList, crbug/569636.
 
-  DCHECK_EQ(type_, syncer::GetModelTypeFromSpecifics(specifics));
+  DCHECK(entity_data.get());
+  DCHECK(!entity_data->is_deleted());
+  DCHECK(!entity_data->non_unique_name.empty());
+  DCHECK_EQ(type_, syncer::GetModelTypeFromSpecifics(entity_data->specifics));
 
+  // If the service specified an overriding hash, use that, otherwise generate
+  // one from the tag. TODO(skym): This behavior should be delayed, once
+  // crbug/561818 is fixed we will only perform this logic in the create case.
   const std::string client_tag_hash(
-      syncer::syncable::GenerateSyncableHash(type_, client_tag));
+      entity_data->client_tag_hash.empty()
+          ? syncer::syncable::GenerateSyncableHash(type_, client_tag)
+          : entity_data->client_tag_hash);
+
   base::Time now = base::Time::Now();
 
   ModelTypeEntity* entity = nullptr;
@@ -166,28 +173,30 @@ void SharedModelTypeProcessor::Put(const std::string& client_tag,
   // client_tag_hash.
   EntityMap::const_iterator it = entities_.find(client_tag_hash);
   if (it == entities_.end()) {
-    scoped_ptr<ModelTypeEntity> scoped_entity =
-        ModelTypeEntity::CreateNew(client_tag, client_tag_hash, "", now);
+    // The service is creating a new entity.
+    scoped_ptr<ModelTypeEntity> scoped_entity = ModelTypeEntity::CreateNew(
+        client_tag, client_tag_hash, entity_data->id, now);
     entity = scoped_entity.get();
     entities_.insert(
         std::make_pair(client_tag_hash, std::move(scoped_entity)));
   } else {
+    // The service is updating an existing entity.
     entity = it->second.get();
   }
 
-  entity->MakeLocalChange(non_unique_name, specifics, now);
+  entity->MakeLocalChange(entity_data.Pass(), now);
   FlushPendingCommitRequests();
 }
 
 void SharedModelTypeProcessor::Delete(
     const std::string& client_key,
     MetadataChangeList* metadata_change_list) {
-  // TODO(skym): Update for new approach. Different caching, different lookup,
-  // metadata changes.
+  // 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);
   if (it == entities_.end()) {
     // That's unusual, but not necessarily a bad thing.
diff --git a/sync/internal_api/shared_model_type_processor_unittest.cc b/sync/internal_api/shared_model_type_processor_unittest.cc
index 6bbed17..23506a82 100644
--- a/sync/internal_api/shared_model_type_processor_unittest.cc
+++ b/sync/internal_api/shared_model_type_processor_unittest.cc
@@ -116,6 +116,9 @@ class SharedModelTypeProcessorTest : public ::testing::Test,
   // when receiving items.
   void SetServerEncryptionKey(const std::string& key_name);
 
+  MockCommitQueue* mock_queue();
+  SharedModelTypeProcessor* type_processor();
+
  private:
   static std::string GenerateTagHash(const std::string& tag);
   static sync_pb::EntitySpecifics GenerateSpecifics(const std::string& tag,
@@ -197,16 +200,17 @@ void SharedModelTypeProcessorTest::StartDone(
   // Hand off ownership of |mock_queue_ptr_|, while keeping
   // an unsafe pointer to it.  This is why we can only connect once.
   DCHECK(mock_queue_ptr_);
-  context->type_processor->OnConnect(mock_queue_ptr_.Pass());
+  context->type_processor->OnConnect(std::move(mock_queue_ptr_));
   // The context's type processor is a proxy; run the task it posted.
   sync_loop_.RunUntilIdle();
 }
 
 void SharedModelTypeProcessorTest::WriteItem(const std::string& tag,
                                              const std::string& value) {
-  sync_pb::EntitySpecifics specifics = GenerateSpecifics(tag, value);
-  // Use the tag as non-unique name.
-  type_processor_->Put(tag, tag, specifics, nullptr);
+  scoped_ptr<EntityData> entity_data = make_scoped_ptr(new EntityData());
+  entity_data->specifics = GenerateSpecifics(tag, value);
+  entity_data->non_unique_name = tag;
+  type_processor_->Put(tag, std::move(entity_data), nullptr);
 }
 
 void SharedModelTypeProcessorTest::DeleteItem(const std::string& tag) {
@@ -313,6 +317,14 @@ void SharedModelTypeProcessorTest::SetServerEncryptionKey(
   mock_queue_->SetServerEncryptionKey(key_name);
 }
 
+MockCommitQueue* SharedModelTypeProcessorTest::mock_queue() {
+  return mock_queue_;
+}
+
+SharedModelTypeProcessor* SharedModelTypeProcessorTest::type_processor() {
+  return type_processor_.get();
+}
+
 std::string SharedModelTypeProcessorTest::GenerateTagHash(
     const std::string& tag) {
   return syncer::syncable::GenerateSyncableHash(kModelType, tag);
@@ -393,6 +405,52 @@ TEST_F(SharedModelTypeProcessorTest, CreateLocalItem) {
   EXPECT_EQ("value1", tag1_data.specifics.preference().value());
 }
 
+// The purpose of this test case is to test setting |client_tag_hash| and |id|
+// on the EntityData object as we pass it into the Put method of the processor.
+// TODO(skym): Update this test to verify that specifying a |client_tag_hash|
+// on update has no effect once crbug/561818 is completed.
+TEST_F(SharedModelTypeProcessorTest, CreateAndModifyWithOverrides) {
+  InitializeToReadyState();
+  EXPECT_EQ(0U, GetNumCommitRequestLists());
+
+  scoped_ptr<EntityData> entity_data = make_scoped_ptr(new EntityData());
+  entity_data->specifics.mutable_preference()->set_name("tag1");
+  entity_data->specifics.mutable_preference()->set_value("value1");
+
+  entity_data->non_unique_name = "tag1";
+  entity_data->client_tag_hash = "hash";
+  entity_data->id = "cid1";
+  type_processor()->Put("tag1", std::move(entity_data), nullptr);
+
+  // Don't access through tag because we forced a specific hash.
+  EXPECT_EQ(1U, GetNumCommitRequestLists());
+  ASSERT_TRUE(mock_queue()->HasCommitRequestForTagHash("hash"));
+  const EntityData& out_entity1 =
+      mock_queue()->GetLatestCommitRequestForTagHash("hash").entity.value();
+
+  EXPECT_EQ("hash", out_entity1.client_tag_hash);
+  EXPECT_EQ("cid1", out_entity1.id);
+  EXPECT_EQ("value1", out_entity1.specifics.preference().value());
+
+  entity_data.reset(new EntityData());
+  entity_data->specifics.mutable_preference()->set_name("tag2");
+  entity_data->specifics.mutable_preference()->set_value("value2");
+  entity_data->non_unique_name = "tag2";
+  entity_data->client_tag_hash = "hash";
+  entity_data->id = "cid2";
+  type_processor()->Put("tag2", std::move(entity_data), nullptr);
+
+  EXPECT_EQ(2U, GetNumCommitRequestLists());
+  ASSERT_TRUE(mock_queue()->HasCommitRequestForTagHash("hash"));
+  const EntityData& out_entity2 =
+      mock_queue()->GetLatestCommitRequestForTagHash("hash").entity.value();
+
+  // Should still see old cid1 value, override is not respected on update.
+  EXPECT_EQ("hash", out_entity2.client_tag_hash);
+  EXPECT_EQ("cid1", out_entity2.id);
+  EXPECT_EQ("value2", out_entity2.specifics.preference().value());
+}
+
 // Creates a new local item then modifies it.
 // Thoroughly tests data generated by modification of server-unknown item.
 TEST_F(SharedModelTypeProcessorTest, CreateAndModifyLocalItem) {