[Sync] Don't use WorkerEntityTracker for commit path in worker.

This is a step towards worker not relying on client tag hash to identify entity.
It is needed to implement bookmarks migration as bookmarks code doesn't populate
client tag hash.

In this CL I stop caching CommitRequestData in WorkerEntityTracker, instead I
pass CommitRequestDataList to CommitContributor. When commit response arrives,
records are matched with corresponding request objects in CommitContributor
based on position. All the logic related to populating and adjusting SyncEntity
is moved there too.

BUG=740757
R=skym@chromium.org

Change-Id: I46012c48e506bfdd23f393536dcf2f911d0efad4
Reviewed-on: https://chromium-review.googlesource.com/775773
Commit-Queue: Pavel Yatsuk <pavely@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517287}

components/sync/engine_impl/model_type_worker.cc

@@ -289,43 +289,15 @@ std::unique_ptr<CommitContribution> ModelTypeWorker::GetContribution(
     return std::unique_ptr<CommitContribution>();
   }
 
-  // Add local changes to entity trackers and copy them into on-the-wire
-  // protobuf.
-  google::protobuf::RepeatedPtrField<sync_pb::SyncEntity> commit_entities;
-  for (const CommitRequestData& commit : response) {
-    const EntityData& data = commit.entity.value();
-    DCHECK(data.is_deleted() ||
-           (type_ == GetModelTypeFromSpecifics(data.specifics)))
-        << "Local change has wrong type: "
-        << ModelTypeToString(GetModelTypeFromSpecifics(data.specifics))
-        << ", expected: " << ModelTypeToString(type_);
-    WorkerEntityTracker* entity = CreateEntityTracker(data.client_tag_hash);
-    entity->RequestCommit(commit);
-    sync_pb::SyncEntity* commit_entity = commit_entities.Add();
-    entity->PopulateCommitProto(commit_entity);
-    AdjustCommitProto(commit_entity);
-  }
-
-  DCHECK((size_t)commit_entities.size() <= max_entries);
+  DCHECK(response.size() <= max_entries);
   return std::make_unique<NonBlockingTypeCommitContribution>(
-      model_type_state_.type_context(), commit_entities, this,
-      debug_info_emitter_, CommitOnlyTypes().Has(GetModelType()));
+      GetModelType(), model_type_state_.type_context(), response, this,
+      cryptographer_.get(), debug_info_emitter_,
+      CommitOnlyTypes().Has(GetModelType()));
 }
 
 void ModelTypeWorker::OnCommitResponse(CommitResponseDataList* response_list) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  for (CommitResponseData& response : *response_list) {
-    WorkerEntityTracker* entity = GetEntityTracker(response.client_tag_hash);
-
-    // There's no way we could have committed an entry we know nothing about.
-    if (entity == nullptr) {
-      NOTREACHED() << "Received commit response for item unknown to us."
-                   << " Model type: " << ModelTypeToString(type_)
-                   << " ID: " << response.id;
-    }
-
-    entity->ReceiveCommitResponse(&response);
-  }
 
   // Send the responses back to the model thread. It needs to know which
   // items have been successfully committed so it can save that information in
@@ -376,59 +348,6 @@ bool ModelTypeWorker::BlockForEncryption() const {
   return cryptographer_ && !cryptographer_->is_ready();
 }
 
-void ModelTypeWorker::AdjustCommitProto(sync_pb::SyncEntity* sync_entity) {
-  DCHECK(CanCommitItems());
-
-  // Initial commits need our help to generate a client ID.
-  if (sync_entity->version() == kUncommittedVersion) {
-    DCHECK(sync_entity->id_string().empty()) << sync_entity->id_string();
-    // TODO(crbug.com/516866): This is incorrect for bookmarks for two reasons:
-    // 1) Won't be able to match previously committed bookmarks to the ones
-    //    with server ID.
-    // 2) Recommitting an item in a case of failing to receive commit response
-    //    would result in generating a different client ID, which in turn
-    //    would result in a duplication.
-    // We should generate client ID on the frontend side instead.
-    sync_entity->set_id_string(base::GenerateGUID());
-    sync_entity->set_version(0);
-  } else {
-    DCHECK(!sync_entity->id_string().empty());
-  }
-
-  // Encrypt the specifics and hide the title if necessary.
-  if (cryptographer_) {
-    // If there is a cryptographer and CanCommitItems() is true then the
-    // cryptographer is valid and ready to encrypt.
-    sync_pb::EntitySpecifics encrypted_specifics;
-    bool result = cryptographer_->Encrypt(
-        sync_entity->specifics(), encrypted_specifics.mutable_encrypted());
-    DCHECK(result);
-    sync_entity->mutable_specifics()->CopyFrom(encrypted_specifics);
-    sync_entity->set_name("encrypted");
-  }
-
-  // Always include enough specifics to identify the type. Do this even in
-  // deletion requests, where the specifics are otherwise invalid.
-  AddDefaultFieldValue(type_, sync_entity->mutable_specifics());
-
-  // TODO(crbug.com/516866): Set parent_id_string for hierarchical types here.
-
-  if (CommitOnlyTypes().Has(GetModelType())) {
-    DCHECK(!cryptographer_);
-    // Remove absolutely everything we can get away with. We do not want to
-    // remove |client_defined_unique_tag| yet because the commit contribution
-    // needs the id to track the responses. They will remove it instead.
-    sync_entity->clear_attachment_id();
-    sync_entity->clear_ctime();
-    sync_entity->clear_deleted();
-    sync_entity->clear_folder();
-    sync_entity->clear_id_string();
-    sync_entity->clear_mtime();
-    sync_entity->clear_name();
-    sync_entity->clear_version();
-  }
-}
-
 bool ModelTypeWorker::UpdateEncryptionKeyName() {
   const std::string& new_key_name = cryptographer_->GetDefaultNigoriKeyName();
   const std::string& old_key_name = model_type_state_.encryption_key_name();

components/sync/engine_impl/model_type_worker.h

@@ -132,11 +132,6 @@ class ModelTypeWorker : public UpdateHandler,
   // encryption issues and must wait for keys to be updated.
   bool BlockForEncryption() const;
 
-  // Takes |commit_entity| populated from fields of WorkerEntityTracker and
-  // adjusts some fields before committing to server. Adjustments include
-  // generating client-assigned ID, encrypting data, etc.
-  void AdjustCommitProto(sync_pb::SyncEntity* commit_entity);
-
   // Updates the encryption key name stored in |model_type_state_| if it differs
   // from the default encryption key name in |cryptographer_|. Returns whether
   // an update occurred.

components/sync/engine_impl/non_blocking_type_commit_contribution.cc

@@ -6,7 +6,9 @@
 
 #include <algorithm>
 
+#include "base/guid.h"
 #include "base/values.h"
+#include "components/sync/base/time.h"
 #include "components/sync/engine/non_blocking_sync_common.h"
 #include "components/sync/engine_impl/model_type_worker.h"
 #include "components/sync/protocol/proto_value_conversions.h"
@@ -14,17 +16,21 @@
 namespace syncer {
 
 NonBlockingTypeCommitContribution::NonBlockingTypeCommitContribution(
+    ModelType type,
     const sync_pb::DataTypeContext& context,
-    const google::protobuf::RepeatedPtrField<sync_pb::SyncEntity>& entities,
+    const CommitRequestDataList& commit_requests,
     ModelTypeWorker* worker,
+    Cryptographer* cryptographer,
     DataTypeDebugInfoEmitter* debug_info_emitter,
-    bool clear_client_defined_unique_tags)
-    : worker_(worker),
+    bool only_commit_specifics)
+    : type_(type),
+      worker_(worker),
+      cryptographer_(cryptographer),
       context_(context),
-      entities_(entities),
+      commit_requests_(commit_requests),
       cleaned_up_(false),
       debug_info_emitter_(debug_info_emitter),
-      clear_client_defined_unique_tags_(clear_client_defined_unique_tags) {}
+      only_commit_specifics_(only_commit_specifics) {}
 
 NonBlockingTypeCommitContribution::~NonBlockingTypeCommitContribution() {
   DCHECK(cleaned_up_);
@@ -35,13 +41,20 @@ void NonBlockingTypeCommitContribution::AddToCommitMessage(
   sync_pb::CommitMessage* commit_message = msg->mutable_commit();
   entries_start_index_ = commit_message->entries_size();
 
-  std::copy(entities_.begin(), entities_.end(),
-            RepeatedPtrFieldBackInserter(commit_message->mutable_entries()));
-
-  if (clear_client_defined_unique_tags_) {
-    for (int i = entries_start_index_; i < commit_message->entries_size();
-         ++i) {
-      commit_message->mutable_entries(i)->clear_client_defined_unique_tag();
+  commit_message->mutable_entries()->Reserve(commit_message->entries_size() +
+                                             commit_requests_.size());
+
+  for (const auto& commit_request : commit_requests_) {
+    sync_pb::SyncEntity* sync_entity = commit_message->add_entries();
+    if (only_commit_specifics_) {
+      DCHECK(!commit_request.entity->is_deleted());
+      DCHECK(!cryptographer_);
+      // Only send specifics to server for commit-only types.
+      sync_entity->mutable_specifics()->CopyFrom(
+          commit_request.entity->specifics);
+    } else {
+      PopulateCommitProto(commit_request, sync_entity);
+      AdjustCommitProto(sync_entity);
     }
   }
 
@@ -49,7 +62,7 @@ void NonBlockingTypeCommitContribution::AddToCommitMessage(
     commit_message->add_client_contexts()->CopyFrom(context_);
 
   CommitCounters* counters = debug_info_emitter_->GetMutableCommitCounters();
-  counters->num_commits_attempted += entities_.size();
+  counters->num_commits_attempted += commit_requests_.size();
 }
 
 SyncerError NonBlockingTypeCommitContribution::ProcessCommitResponse(
@@ -64,30 +77,28 @@ SyncerError NonBlockingTypeCommitContribution::ProcessCommitResponse(
 
   CommitResponseDataList response_list;
 
-  for (int i = 0; i < entities_.size(); ++i) {
+  for (size_t i = 0; i < commit_requests_.size(); ++i) {
     const sync_pb::CommitResponse_EntryResponse& entry_response =
         commit_response.entryresponse(entries_start_index_ + i);
 
     switch (entry_response.response_type()) {
       case sync_pb::CommitResponse::INVALID_MESSAGE:
         LOG(ERROR) << "Server reports commit message is invalid.";
-        DLOG(ERROR) << "Message was: "
-                    << SyncEntityToValue(entities_.Get(i), false).get();
         unknown_error = true;
         break;
       case sync_pb::CommitResponse::CONFLICT:
         DVLOG(1) << "Server reports conflict for commit message.";
-        DVLOG(1) << "Message was: "
-                 << SyncEntityToValue(entities_.Get(i), false).get();
         ++conflicting_commits;
         break;
       case sync_pb::CommitResponse::SUCCESS: {
         ++successes;
         CommitResponseData response_data;
+        const CommitRequestData& commit_request = commit_requests_[i];
         response_data.id = entry_response.id_string();
-        response_data.client_tag_hash =
-            entities_.Get(i).client_defined_unique_tag();
         response_data.response_version = entry_response.version();
+        response_data.client_tag_hash = commit_request.entity->client_tag_hash;
+        response_data.sequence_number = commit_request.sequence_number;
+        response_data.specifics_hash = commit_request.specifics_hash;
         response_list.push_back(response_data);
         break;
       }
@@ -134,7 +145,63 @@ void NonBlockingTypeCommitContribution::CleanUp() {
 }
 
 size_t NonBlockingTypeCommitContribution::GetNumEntries() const {
-  return entities_.size();
+  return commit_requests_.size();
+}
+
+// static
+void NonBlockingTypeCommitContribution::PopulateCommitProto(
+    const CommitRequestData& commit_entity,
+    sync_pb::SyncEntity* commit_proto) {
+  const EntityData& entity_data = commit_entity.entity.value();
+
+  commit_proto->set_id_string(entity_data.id);
+  commit_proto->set_client_defined_unique_tag(entity_data.client_tag_hash);
+  commit_proto->set_version(commit_entity.base_version);
+  commit_proto->set_deleted(entity_data.is_deleted());
+  commit_proto->set_folder(false);
+  commit_proto->set_name(entity_data.non_unique_name);
+  // TODO(stanisc): This doesn't support bookmarks yet.
+  DCHECK(entity_data.parent_id.empty());
+  // TODO(crbug.com/516866): Set parent_id_string for hierarchical types here.
+
+  if (!entity_data.is_deleted()) {
+    commit_proto->set_ctime(TimeToProtoTime(entity_data.creation_time));
+    commit_proto->set_mtime(TimeToProtoTime(entity_data.modification_time));
+    commit_proto->mutable_specifics()->CopyFrom(entity_data.specifics);
+  }
+}
+
+void NonBlockingTypeCommitContribution::AdjustCommitProto(
+    sync_pb::SyncEntity* commit_proto) {
+  // Initial commits need our help to generate a client ID.
+  if (commit_proto->version() == kUncommittedVersion) {
+    DCHECK(commit_proto->id_string().empty()) << commit_proto->id_string();
+    // TODO(crbug.com/516866): This is incorrect for bookmarks for two reasons:
+    // 1) Won't be able to match previously committed bookmarks to the ones
+    //    with server ID.
+    // 2) Recommitting an item in a case of failing to receive commit response
+    //    would result in generating a different client ID, which in turn
+    //    would result in a duplication.
+    // We should generate client ID on the frontend side instead.
+    commit_proto->set_id_string(base::GenerateGUID());
+    commit_proto->set_version(0);
+  } else {
+    DCHECK(!commit_proto->id_string().empty());
+  }
+
+  // Encrypt the specifics and hide the title if necessary.
+  if (cryptographer_) {
+    sync_pb::EntitySpecifics encrypted_specifics;
+    bool result = cryptographer_->Encrypt(
+        commit_proto->specifics(), encrypted_specifics.mutable_encrypted());
+    DCHECK(result);
+    commit_proto->mutable_specifics()->CopyFrom(encrypted_specifics);
+    commit_proto->set_name("encrypted");
+  }
+
+  // Always include enough specifics to identify the type. Do this even in
+  // deletion requests, where the specifics are otherwise invalid.
+  AddDefaultFieldValue(type_, commit_proto->mutable_specifics());
 }
 
 }  // namespace syncer

components/sync/engine_impl/non_blocking_type_commit_contribution.h

@@ -11,12 +11,14 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "components/sync/engine/non_blocking_sync_common.h"
 #include "components/sync/engine_impl/commit_contribution.h"
 #include "components/sync/engine_impl/cycle/data_type_debug_info_emitter.h"
 #include "components/sync/protocol/sync.pb.h"
 
 namespace syncer {
 
+class Cryptographer;
 class ModelTypeWorker;
 
 // A non-blocking sync type's contribution to an outgoing commit message.
@@ -26,11 +28,13 @@ class ModelTypeWorker;
 class NonBlockingTypeCommitContribution : public CommitContribution {
  public:
   NonBlockingTypeCommitContribution(
+      ModelType type,
       const sync_pb::DataTypeContext& context,
-      const google::protobuf::RepeatedPtrField<sync_pb::SyncEntity>& entities,
+      const CommitRequestDataList& commit_requests,
       ModelTypeWorker* worker,
+      Cryptographer* cryptographer,
       DataTypeDebugInfoEmitter* debug_info_emitter,
-      bool clear_client_defined_unique_tags);
+      bool only_commit_specifics);
   ~NonBlockingTypeCommitContribution() override;
 
   // Implementation of CommitContribution
@@ -42,14 +46,26 @@ class NonBlockingTypeCommitContribution : public CommitContribution {
   size_t GetNumEntries() const override;
 
  private:
+  // Copies data to be committed from CommitRequestData into SyncEntity proto.
+  static void PopulateCommitProto(const CommitRequestData& commit_entity,
+                                  sync_pb::SyncEntity* commit_proto);
+
+  // Generates id for new entites and encrypts entity if needed.
+  void AdjustCommitProto(sync_pb::SyncEntity* commit_proto);
+
+  const ModelType type_;
+
   // A non-owned pointer back to the object that created this contribution.
   ModelTypeWorker* const worker_;
 
+  // A non-owned pointer to cryptographer to encrypt entities.
+  Cryptographer* const cryptographer_;
+
   // The type-global context information.
   const sync_pb::DataTypeContext context_;
 
-  // The set of entities to be committed, serialized as SyncEntities.
-  const google::protobuf::RepeatedPtrField<sync_pb::SyncEntity> entities_;
+  // The list of entities to be committed.
+  CommitRequestDataList commit_requests_;
 
   // The index in the commit message where this contribution's entities are
   // added.  Used to correlate per-item requests with per-item responses.
@@ -61,12 +77,9 @@ class NonBlockingTypeCommitContribution : public CommitContribution {
 
   DataTypeDebugInfoEmitter* debug_info_emitter_;
 
-  // If we should remove all the tag hashes from the commit data that actually
-  // gets sent over the wire. This is used to save bandwidth when we do not need
-  // these entities to have consistent client ids, such as with commit only
-  // types. These ids are still passed into this contribution object so that
-  // they can be set on response before handing back to the |worker_|.
-  bool clear_client_defined_unique_tags_;
+  // Don't send any metadata to server, only specifics. This is needed for
+  // commit only types to save bandwidth.
+  bool only_commit_specifics_;
 
   DISALLOW_COPY_AND_ASSIGN(NonBlockingTypeCommitContribution);
 };

components/sync/engine_impl/worker_entity_tracker.cc

@@ -21,113 +21,6 @@ WorkerEntityTracker::WorkerEntityTracker(const std::string& client_tag_hash)
 
 WorkerEntityTracker::~WorkerEntityTracker() {}
 
-bool WorkerEntityTracker::HasPendingCommit() const {
-  return !!pending_commit_;
-}
-
-bool WorkerEntityTracker::PendingCommitIsDeletion() const {
-  DCHECK(pending_commit_);
-  return pending_commit_->entity.value().is_deleted();
-}
-
-void WorkerEntityTracker::PopulateCommitProto(
-    sync_pb::SyncEntity* commit_entity) const {
-  DCHECK(HasPendingCommit());
-
-  const EntityData& entity = pending_commit_->entity.value();
-  DCHECK_EQ(client_tag_hash_, entity.client_tag_hash);
-
-  commit_entity->set_id_string(id_);
-  commit_entity->set_client_defined_unique_tag(client_tag_hash_);
-  commit_entity->set_version(base_version_);
-  commit_entity->set_deleted(entity.is_deleted());
-
-  // TODO(stanisc): This doesn't support bookmarks yet.
-  DCHECK(entity.parent_id.empty());
-  commit_entity->set_folder(false);
-
-  commit_entity->set_name(entity.non_unique_name);
-  if (!entity.is_deleted()) {
-    commit_entity->set_ctime(TimeToProtoTime(entity.creation_time));
-    commit_entity->set_mtime(TimeToProtoTime(entity.modification_time));
-    commit_entity->mutable_specifics()->CopyFrom(entity.specifics);
-  }
-}
-
-void WorkerEntityTracker::RequestCommit(const CommitRequestData& data) {
-  DCHECK_GE(data.base_version, base_version_)
-      << "Base version should never decrease";
-  DCHECK_GE(data.sequence_number, sequence_number_)
-      << "Sequence number should never decrease";
-
-  // Store the ID if one isn't tracked yet. If one is tracked, it may be more
-  // up-to-date than one coming from the processor.
-  if (id_.empty()) {
-    id_ = data.entity->id;
-  }
-
-  // Update our book-keeping counters.
-  base_version_ = data.base_version;
-  sequence_number_ = data.sequence_number;
-  pending_commit_specifics_hash_ = data.specifics_hash;
-
-  // Deletions of server-unknown items should not be passed to worker, they
-  // should be blocked at the processor.
-  DCHECK(!data.entity->is_deleted() || IsServerKnown());
-
-  // We intentionally don't update the id_ here. Good ID values come from the
-  // server and always pass through the sync thread first. There's no way the
-  // model thread could have a better ID value than we do.
-
-  // This entity is identified by its client tag. That value can never change.
-  DCHECK_EQ(client_tag_hash_, data.entity->client_tag_hash);
-  // TODO(stanisc): consider simply copying CommitRequestData instead of
-  // allocating one dynamically.
-  pending_commit_ = std::make_unique<CommitRequestData>(data);
-
-  // Do our counter values indicate a conflict? If so, don't commit.
-  //
-  // There's no need to inform the model thread of the conflict. The
-  // conflicting update has already been posted to its task runner; it will
-  // figure it out as soon as it runs that task.
-  //
-  // Note that this check must be after pending_commit_ is set.
-  if (IsInConflict()) {
-    ClearPendingCommit();
-    return;
-  }
-
-  // There's no conflict; increase base_version_ if there was a commit response
-  // the processor didn't know about.
-  base_version_ = std::max(base_version_, highest_commit_response_version_);
-
-  // Otherwise, keep the data associated with this pending commit
-  // so it can be committed at the next possible opportunity.
-}
-
-void WorkerEntityTracker::ReceiveCommitResponse(CommitResponseData* ack) {
-  DCHECK_GT(ack->response_version, highest_commit_response_version_)
-      << "Had expected higher response version."
-      << " id: " << id_;
-
-  // Commit responses, especially after the first commit, can update our ID.
-  DCHECK(!ack->id.empty());
-  id_ = ack->id;
-  highest_commit_response_version_ = ack->response_version;
-
-  // Fill in some cached info for the response data. Since commits happen
-  // synchronously on the sync thread, our item's state is guaranteed to be
-  // the same at the end of the commit as it was at the start.
-  ack->sequence_number = sequence_number_;
-  ack->specifics_hash = pending_commit_specifics_hash_;
-
-  // Because an in-progress commit blocks the sync thread, we can assume that
-  // the item we just committed successfully is exactly the one we have now.
-  // Nothing changed it while the commit was happening. Since we're now in
-  // sync with the server, we can clear the pending commit.
-  ClearPendingCommit();
-}
-
 void WorkerEntityTracker::ReceiveUpdate(const UpdateResponseData& update) {
   if (!UpdateContainsNewVersion(update))
     return;
@@ -139,12 +32,6 @@ void WorkerEntityTracker::ReceiveUpdate(const UpdateResponseData& update) {
   // Got an applicable update newer than any pending updates. It must be safe
   // to discard the old encrypted update, if there was one.
   ClearEncryptedUpdate();
-
-  if (IsInConflict()) {
-    // Incoming update clobbers the pending commit on the sync thread.
-    // The model thread can re-request this commit later if it wants to.
-    ClearPendingCommit();
-  }
 }
 
 bool WorkerEntityTracker::UpdateContainsNewVersion(
@@ -159,7 +46,6 @@ bool WorkerEntityTracker::ReceiveEncryptedUpdate(
 
   highest_gu_response_version_ = data.response_version;
   encrypted_update_ = std::make_unique<UpdateResponseData>(data);
-  ClearPendingCommit();
   return true;
 }
 
@@ -180,42 +66,8 @@ size_t WorkerEntityTracker::EstimateMemoryUsage() const {
   size_t memory_usage = 0;
   memory_usage += EstimateMemoryUsage(client_tag_hash_);
   memory_usage += EstimateMemoryUsage(id_);
-  memory_usage += EstimateMemoryUsage(pending_commit_);
-  memory_usage += EstimateMemoryUsage(pending_commit_specifics_hash_);
   memory_usage += EstimateMemoryUsage(encrypted_update_);
   return memory_usage;
 }
 
-bool WorkerEntityTracker::IsInConflict() const {
-  if (!HasPendingCommit())
-    return false;
-
-  if (HasEncryptedUpdate())
-    return true;
-
-  if (highest_gu_response_version_ <= highest_commit_response_version_) {
-    // The most recent server state was created in a commit made by this
-    // client. We're fully up to date, and therefore not in conflict.
-    return false;
-  } else {
-    // The most recent server state was written by someone else.
-    // Did the model thread have the most up to date version when it issued the
-    // commit request?
-    if (base_version_ >= highest_gu_response_version_) {
-      return false;  // Yes.
-    } else {
-      return true;  // No.
-    }
-  }
-}
-
-bool WorkerEntityTracker::IsServerKnown() const {
-  return base_version_ != kUncommittedVersion;
-}
-
-void WorkerEntityTracker::ClearPendingCommit() {
-  pending_commit_.reset();
-  pending_commit_specifics_hash_.clear();
-}
-
 }  // namespace syncer

components/sync/engine_impl/worker_entity_tracker.h

@@ -37,24 +37,6 @@ class WorkerEntityTracker {
 
   ~WorkerEntityTracker();
 
-  // Returns true if this entity should be commited to the server.
-  bool HasPendingCommit() const;
-
-  // Returns true if pending commit contains deleted entity.
-  bool PendingCommitIsDeletion() const;
-
-  // Populates a sync_pb::SyncEntity for a commit.
-  void PopulateCommitProto(sync_pb::SyncEntity* commit_entity) const;
-
-  // Updates this entity with data from the latest version that the
-  // model asked us to commit. May clobber state related to the
-  // model's previous commit attempt(s).
-  void RequestCommit(const CommitRequestData& data);
-
-  // Tracks the receipt of a commit response and fills in some local-only data
-  // on it to be passed back to the processor.
-  void ReceiveCommitResponse(CommitResponseData* ack);
-
   // Handles receipt of an update from the server.
   void ReceiveUpdate(const UpdateResponseData& update);
 
@@ -82,44 +64,15 @@ class WorkerEntityTracker {
   const std::string& client_tag_hash() const { return client_tag_hash_; }
 
  private:
-  // Checks if the current state indicates a conflict.
-  //
-  // This can be true only while a call to this object is in progress.
-  // Conflicts are always cleared before the method call ends.
-  bool IsInConflict() const;
-
-  // Checks if the server knows about this item.
-  bool IsServerKnown() const;
-
-  // Clears flag and optionally clears state associated with a pending commit.
-  void ClearPendingCommit();
-
   // The hashed client tag for this entry.
   const std::string client_tag_hash_;
 
   // The ID for this entry. May be empty if the entry has never been committed.
   std::string id_;
 
-  // Used to track in-flight commit requests on the model thread. All we need
-  // to do here is return it back to the model thread when the pending commit
-  // is completed and confirmed. Not valid if no commit is pending.
-  int64_t sequence_number_ = 0;
-
-  // The server version on which this item is based.
-  int64_t base_version_ = kUncommittedVersion;
-
-  // The highest version seen in a commit response for this entry.
-  int64_t highest_commit_response_version_ = kUncommittedVersion;
-
   // The highest version seen in a GU response for this entry.
   int64_t highest_gu_response_version_ = kUncommittedVersion;
 
-  // A commit for this entity waiting for a sync cycle to be committed.
-  std::unique_ptr<CommitRequestData> pending_commit_;
-
-  // The specifics hash for the pending commit if there is one, "" otherwise.
-  std::string pending_commit_specifics_hash_;
-
   // An update for this entity which can't be applied right now. The presence
   // of an pending update prevents commits. As of this writing, the only
   // source of pending updates is updates that can't currently be decrypted.

components/sync/engine_impl/worker_entity_tracker_unittest.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.
@@ -7,7 +6,6 @@
 
 #include "components/sync/base/hash_util.h"
 #include "components/sync/base/model_type.h"
-#include "components/sync/base/time.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace syncer {
@@ -26,9 +24,6 @@ class WorkerEntityTrackerTest : public ::testing::Test {
       : kServerId("ServerID"),
         kClientTag("some.sample.tag"),
         kClientTagHash(GenerateSyncableHash(PREFERENCES, kClientTag)),
-        kSpecificsHash("somehash"),
-        kCtime(base::Time::UnixEpoch() + base::TimeDelta::FromDays(10)),
-        kMtime(base::Time::UnixEpoch() + base::TimeDelta::FromDays(20)),
         entity_(new WorkerEntityTracker(kClientTagHash)) {
     specifics.mutable_preference()->set_name(kClientTag);
     specifics.mutable_preference()->set_value("pref.value");
@@ -36,23 +31,6 @@ class WorkerEntityTrackerTest : public ::testing::Test {
 
   ~WorkerEntityTrackerTest() override {}
 
-  CommitRequestData MakeCommitRequestData(int64_t sequence_number,
-                                          int64_t base_version) {
-    EntityData data;
-    data.client_tag_hash = kClientTagHash;
-    data.creation_time = kCtime;
-    data.modification_time = kMtime;
-    data.specifics = specifics;
-    data.non_unique_name = kClientTag;
-
-    CommitRequestData request_data;
-    request_data.entity = data.PassToPtr();
-    request_data.sequence_number = sequence_number;
-    request_data.base_version = base_version;
-    request_data.specifics_hash = kSpecificsHash;
-    return request_data;
-  }
-
   UpdateResponseData MakeUpdateResponseData(int64_t response_version) {
     EntityData data;
     data.id = kServerId;
@@ -67,115 +45,15 @@ class WorkerEntityTrackerTest : public ::testing::Test {
   const std::string kServerId;
   const std::string kClientTag;
   const std::string kClientTagHash;
-  const std::string kSpecificsHash;
-  const base::Time kCtime;
-  const base::Time kMtime;
   sync_pb::EntitySpecifics specifics;
   std::unique_ptr<WorkerEntityTracker> entity_;
 };
 
 // Construct a new entity from a server update. Then receive another update.
 TEST_F(WorkerEntityTrackerTest, FromUpdateResponse) {
-  EXPECT_FALSE(entity_->HasPendingCommit());
   EXPECT_EQ("", entity_->id());
   entity_->ReceiveUpdate(MakeUpdateResponseData(20));
-  EXPECT_FALSE(entity_->HasPendingCommit());
   EXPECT_EQ(kServerId, entity_->id());
 }
 
-// Construct a new entity from a commit request. Then serialize it.
-TEST_F(WorkerEntityTrackerTest, FromCommitRequest) {
-  const int64_t kSequenceNumber = 22;
-  const int64_t kBaseVersion = 33;
-  CommitRequestData data = MakeCommitRequestData(kSequenceNumber, kBaseVersion);
-  entity_->RequestCommit(data);
-  EXPECT_EQ("", entity_->id());
-
-  ASSERT_TRUE(entity_->HasPendingCommit());
-  sync_pb::SyncEntity pb_entity;
-  entity_->PopulateCommitProto(&pb_entity);
-  EXPECT_EQ("", pb_entity.id_string());
-  EXPECT_EQ(kClientTagHash, pb_entity.client_defined_unique_tag());
-  EXPECT_EQ(kBaseVersion, pb_entity.version());
-  EXPECT_EQ(kCtime, ProtoTimeToTime(pb_entity.ctime()));
-  EXPECT_EQ(kMtime, ProtoTimeToTime(pb_entity.mtime()));
-  EXPECT_FALSE(pb_entity.deleted());
-  EXPECT_EQ(specifics.preference().name(),
-            pb_entity.specifics().preference().name());
-  EXPECT_EQ(specifics.preference().value(),
-            pb_entity.specifics().preference().value());
-
-  CommitResponseData ack;
-  ack.response_version = kBaseVersion + 1;
-  ack.id = kServerId;
-  entity_->ReceiveCommitResponse(&ack);
-
-  EXPECT_EQ(kSequenceNumber, ack.sequence_number);
-  EXPECT_EQ(kSpecificsHash, ack.specifics_hash);
-  EXPECT_FALSE(entity_->HasPendingCommit());
-
-  EXPECT_EQ(kServerId, entity_->id());
-  CommitRequestData data2 =
-      MakeCommitRequestData(kSequenceNumber + 1, ack.response_version);
-  entity_->RequestCommit(data2);
-  entity_->PopulateCommitProto(&pb_entity);
-  EXPECT_EQ(kServerId, pb_entity.id_string());
-}
-
-// Start with a server initiated entity. Commit over top of it.
-TEST_F(WorkerEntityTrackerTest, RequestCommit) {
-  entity_->RequestCommit(MakeCommitRequestData(1, 10));
-  EXPECT_TRUE(entity_->HasPendingCommit());
-}
-
-// Start with a server initiated entity. Fail to request a commit because of
-// an out of date base version.
-TEST_F(WorkerEntityTrackerTest, RequestCommitFailure) {
-  entity_->ReceiveUpdate(MakeUpdateResponseData(10));
-  EXPECT_FALSE(entity_->HasPendingCommit());
-  entity_->RequestCommit(
-      MakeCommitRequestData(23, 5 /* base_version 5 < 10 */));
-  EXPECT_FALSE(entity_->HasPendingCommit());
-}
-
-// Start with a pending commit. Clobber it with an incoming update.
-TEST_F(WorkerEntityTrackerTest, UpdateClobbersCommit) {
-  CommitRequestData data = MakeCommitRequestData(22, 33);
-  entity_->RequestCommit(data);
-
-  EXPECT_TRUE(entity_->HasPendingCommit());
-
-  entity_->ReceiveUpdate(MakeUpdateResponseData(400));  // Version 400 > 33.
-  EXPECT_FALSE(entity_->HasPendingCommit());
-}
-
-// Start with a pending commit. Send it a reflected update that
-// will not override the in-progress commit.
-TEST_F(WorkerEntityTrackerTest, ReflectedUpdateDoesntClobberCommit) {
-  CommitRequestData data = MakeCommitRequestData(22, 33);
-  entity_->RequestCommit(data);
-
-  EXPECT_TRUE(entity_->HasPendingCommit());
-
-  entity_->ReceiveUpdate(MakeUpdateResponseData(33));  // Version 33 == 33.
-  EXPECT_TRUE(entity_->HasPendingCommit());
-}
-
-// Request two commits, both with the same old version. The second commit should
-// have the updated server version.
-TEST_F(WorkerEntityTrackerTest, QuickCommits) {
-  const int64_t kLocalBaseVersion = 10;
-  const int64_t kCommitResponseVersion = 11;
-  entity_->RequestCommit(MakeCommitRequestData(1, kLocalBaseVersion));
-  CommitResponseData ack;
-  ack.response_version = kCommitResponseVersion;
-  ack.id = kServerId;
-  entity_->ReceiveCommitResponse(&ack);
-
-  entity_->RequestCommit(MakeCommitRequestData(1, kLocalBaseVersion));
-  sync_pb::SyncEntity pb_entity;
-  entity_->PopulateCommitProto(&pb_entity);
-  EXPECT_EQ(kCommitResponseVersion, pb_entity.version());
-}
-
 }  // namespace syncer

components/sync/model_impl/shared_model_type_processor.cc

@@ -380,7 +380,7 @@ void SharedModelTypeProcessor::OnCommitCompleted(
   // their commit_requested_sequence_number so they are committed again on next
   // sync cycle.
   // TODO(crbug.com/740757): Iterating over all entities is inefficient. It is
-  // better to remember in GetLocalChanges which entities are bieng committed
+  // better to remember in GetLocalChanges which entities are being committed
   // and adjust only them. Alternatively we can make worker return commit status
   // for all entities, not just successful ones and use that to lookup entities
   // to clear.