[Sync] Update EntityData on all codepaths from model type sync bridge

components/sync/model_impl/processor_entity_tracker.cc

Index: components/sync/model_impl/processor_entity_tracker.cc
diff --git a/components/sync/model_impl/processor_entity_tracker.cc b/components/sync/model_impl/processor_entity_tracker.cc
index cae2e8881580fef09822e0f699d47008d4eef762..20cbc74d56937f79ff4a41729ecdad505a43862a 100644
--- a/components/sync/model_impl/processor_entity_tracker.cc
+++ b/components/sync/model_impl/processor_entity_tracker.cc
@@ -59,11 +59,19 @@ ProcessorEntityTracker::ProcessorEntityTracker(
 
 ProcessorEntityTracker::~ProcessorEntityTracker() {}
 
-void ProcessorEntityTracker::CacheCommitData(EntityData* data) {
+void ProcessorEntityTracker::SetCommitData(EntityData* data) {
   DCHECK(data);
-  if (data->client_tag_hash.empty()) {
-    data->client_tag_hash = metadata_.client_tag_hash();
-  }
+  // Update data's fields from metadata.
+  data->client_tag_hash = metadata_.client_tag_hash();
+  if (!metadata_.server_id().empty())
+    data->id = metadata_.server_id();
+  if (data->creation_time.is_null())

[skym, 2017/02/14 00:33:40]

What do you think of DCHECK we have both times and then set them blindly?

[pavely, 2017/02/14 02:09:42]

I thought that if data doesn't specify mtime then we should use the one from
metadata, but if data explicitly specifies mtime then we should commit with that
value. Now that I think about it, if bridge provided mtime for uncommitted
entity then mtime in EntityData would diverge from mtime in metadata.

I'm blindly reseting data's timestamps to metadata values.

+    data->creation_time = ProtoTimeToTime(metadata_.creation_time());
+  if (data->modification_time.is_null())
+    data->modification_time = ProtoTimeToTime(metadata_.modification_time());
+  DCHECK(MatchesSpecificsHash(data->specifics));
+
+  commit_data_.reset();

[skym, 2017/02/14 00:33:40]

why do you need this reset?

[pavely, 2017/02/14 02:09:41]

CacheCommitData has a DCHECK that ensures there were no previous cached value
before setting new one. This DCHECK is useful since CacheCommitData can be
called from other place.

   CacheCommitData(data->PassToPtr());
 }
 
@@ -145,21 +153,24 @@ void ProcessorEntityTracker::RecordForcedUpdate(
 
 void ProcessorEntityTracker::MakeLocalChange(std::unique_ptr<EntityData> data) {
   DCHECK(!metadata_.client_tag_hash().empty());
-  DCHECK_EQ(metadata_.client_tag_hash(), data->client_tag_hash);
 
-  if (data->modification_time.is_null()) {
-    data->modification_time = base::Time::Now();
-  }
+  // Update metadata fields from updated data.
+  base::Time modification_time = !data->modification_time.is_null()
+                                     ? data->modification_time
+                                     : base::Time::Now();
 
+  // IncrementSequenceNumber should be called before UpdateSpecificHash since
+  // it remembers specifics hash before the modifications.
   IncrementSequenceNumber();
   UpdateSpecificsHash(data->specifics);
-  metadata_.set_modification_time(TimeToProtoTime(data->modification_time));
+  if (!data->creation_time.is_null())

[skym, 2017/02/14 00:33:40]

The rest of your change code seems to be defensive/repairing, but not this one?

[pavely, 2017/02/14 02:09:41]

This code says: If data's timestamps were explicitly set by bridge, there must
be a reason for it and we should copy them to metadata. Otherwise keep creation
time and set modification time to Now().

I wasn't sure what the right behavior should be when data timestamps are set, so
I coded the most sensible one I could think of.

+    metadata_.set_creation_time(TimeToProtoTime(data->creation_time));
+  metadata_.set_modification_time(TimeToProtoTime(modification_time));
   metadata_.set_is_deleted(false);
 
-  data->id = metadata_.server_id();
-  data->creation_time = ProtoTimeToTime(metadata_.creation_time());
-  commit_data_.reset();
-  CacheCommitData(data.get());
+  // SetCommitData will update data's fileds from metadata and wrap it into
+  // immutable EntityDataPtr.
+  SetCommitData(data.get());
 }
 
 void ProcessorEntityTracker::Delete() {
@@ -176,6 +187,7 @@ void ProcessorEntityTracker::InitializeCommitRequestData(
   if (!metadata_.is_deleted()) {
     DCHECK(HasCommitData());
     DCHECK_EQ(commit_data_->client_tag_hash, metadata_.client_tag_hash());
+    DCHECK_EQ(commit_data_->id, metadata_.server_id());
     request->entity = commit_data_;
   } else {
     // Make an EntityData with empty specifics to indicate deletion. This is