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

components/sync/model_impl/processor_entity_tracker.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/sync/model_impl/processor_entity_tracker.h"
 
 #include "base/base64.h"
 #include "base/sha1.h"
 #include "components/sync/base/time.h"
 #include "components/sync/engine/non_blocking_sync_common.h"
@@ skipping 41 matching lines @@
     sync_pb::EntityMetadata* metadata)
     : storage_key_(storage_key),
       commit_requested_sequence_number_(metadata->acked_sequence_number()) {
   DCHECK(metadata->has_client_tag_hash());
   DCHECK(metadata->has_creation_time());
   metadata_.Swap(metadata);
 }
 
 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());
 }
 
 void ProcessorEntityTracker::CacheCommitData(const EntityDataPtr& data_ptr) {
   DCHECK(RequiresCommitData());
   commit_data_ = data_ptr;
   DCHECK(HasCommitData());
 }
 
 bool ProcessorEntityTracker::HasCommitData() const {
@@ skipping 61 matching lines @@
   DCHECK(IsUnsynced());
   // There was a conflict and the server just won it. Explicitly ack all
   // pending commits so they are never enqueued again.
   metadata_.set_acked_sequence_number(metadata_.sequence_number());
   commit_data_.reset();
   RecordAcceptedUpdate(update);
 }
 
 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() {
   IncrementSequenceNumber();
   metadata_.set_modification_time(TimeToProtoTime(base::Time::Now()));
   metadata_.set_is_deleted(true);
   metadata_.clear_specifics_hash();
   // Clear any cached pending commit data.
   commit_data_.reset();
 }
 
 void ProcessorEntityTracker::InitializeCommitRequestData(
     CommitRequestData* request) {
   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
     // done lazily here to simplify loading a pending deletion on startup.
     EntityData data;
     data.client_tag_hash = metadata_.client_tag_hash();
     data.id = metadata_.server_id();
     data.creation_time = ProtoTimeToTime(metadata_.creation_time());
     data.modification_time = ProtoTimeToTime(metadata_.modification_time());
     request->entity = data.PassToPtr();
@@ skipping 51 matching lines @@
 void ProcessorEntityTracker::UpdateSpecificsHash(
     const sync_pb::EntitySpecifics& specifics) {
   if (specifics.ByteSize() > 0) {
     HashSpecifics(specifics, metadata_.mutable_specifics_hash());
   } else {
     metadata_.clear_specifics_hash();
   }
 }
 
 }  // namespace syncer