[Sync] USS: Clean up ModelTypeWorker a little bit.

Add some helper functions, use cleaner syntax, and more.

BUG=

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

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

5 files changed, 89 insertions, 129 deletions

diff --git a/sync/engine/model_type_worker.cc b/sync/engine/model_type_worker.cc
index 1d55066..c76e213 100644
--- a/sync/engine/model_type_worker.cc
+++ b/sync/engine/model_type_worker.cc
@@ -43,6 +43,8 @@ ModelTypeWorker::ModelTypeWorker(
       cryptographer_(std::move(cryptographer)),
       nudge_handler_(nudge_handler),
       weak_ptr_factory_(this) {
+  DCHECK(model_type_processor_);
+
   // Request an initial sync if it hasn't been completed yet.
   if (!data_type_state_.initial_sync_done()) {
     nudge_handler_->NudgeForInitialDownload(type_);
@@ -106,10 +108,7 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
   UpdateResponseDataList response_datas;
   UpdateResponseDataList pending_updates;
 
-  for (SyncEntityList::const_iterator update_it = applicable_updates.begin();
-       update_it != applicable_updates.end(); ++update_it) {
-    const sync_pb::SyncEntity* update_entity = *update_it;
-
+  for (const sync_pb::SyncEntity* update_entity : applicable_updates) {
     // Skip updates for permanent folders.
     // TODO(stanisc): crbug.com/516866: might need to handle this for
     // hierarchical datatypes.
@@ -134,23 +133,13 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
     UpdateResponseData response_data;
     response_data.response_version = update_entity->version();
 
-    WorkerEntityTracker* entity_tracker = nullptr;
-    EntityMap::const_iterator map_it = entities_.find(client_tag_hash);
-    if (map_it == entities_.end()) {
-      scoped_ptr<WorkerEntityTracker> scoped_entity_tracker =
-          WorkerEntityTracker::FromUpdateResponse(response_data);
-      entity_tracker = scoped_entity_tracker.get();
-      entities_.insert(
-          std::make_pair(client_tag_hash, std::move(scoped_entity_tracker)));
-    } else {
-      entity_tracker = map_it->second.get();
-    }
+    WorkerEntityTracker* entity = GetOrCreateEntityTracker(data);
 
+    // Check if specifics are encrypted and try to decrypt if so.
     const sync_pb::EntitySpecifics& specifics = update_entity->specifics();
-
     if (!specifics.has_encrypted()) {
       // No encryption.
-      entity_tracker->ReceiveUpdate(update_entity->version());
+      entity->ReceiveUpdate(update_entity->version());
       data.specifics = specifics;
       response_data.entity = data.PassToPtr();
       response_datas.push_back(response_data);
@@ -158,7 +147,7 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
                cryptographer_->CanDecrypt(specifics.encrypted())) {
       // Encrypted, but we know the key.
       if (DecryptSpecifics(cryptographer_.get(), specifics, &data.specifics)) {
-        entity_tracker->ReceiveUpdate(update_entity->version());
+        entity->ReceiveUpdate(update_entity->version());
         response_data.entity = data.PassToPtr();
         response_data.encryption_key_name = specifics.encrypted().key_name();
         response_datas.push_back(response_data);
@@ -169,7 +158,7 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
       // Can't decrypt right now.  Ask the entity tracker to handle it.
       data.specifics = specifics;
       response_data.entity = data.PassToPtr();
-      if (entity_tracker->ReceivePendingUpdate(response_data)) {
+      if (entity->ReceivePendingUpdate(response_data)) {
         // Send to the model thread for safe-keeping across restarts if the
         // tracker decides the update is worth keeping.
         pending_updates.push_back(response_data);
@@ -218,9 +207,12 @@ void ModelTypeWorker::EnqueueForCommit(const CommitRequestDataList& list) {
       << "Asked to commit items before type was initialized.  "
       << "ModelType is: " << ModelTypeToString(type_);
 
-  for (CommitRequestDataList::const_iterator it = list.begin();
-       it != list.end(); ++it) {
-    StorePendingCommit(*it);
+  for (const CommitRequestData& commit : list) {
+    const EntityData& data = commit.entity.value();
+    if (!data.is_deleted()) {
+      DCHECK_EQ(type_, syncer::GetModelTypeFromSpecifics(data.specifics));
+    }
+    GetOrCreateEntityTracker(data)->RequestCommit(commit);
   }
 
   if (CanCommitItems())
@@ -263,46 +255,21 @@ scoped_ptr<CommitContribution> ModelTypeWorker::GetContribution(
       this));
 }
 
-void ModelTypeWorker::StorePendingCommit(const CommitRequestData& request) {
-  const EntityData& data = request.entity.value();
-  if (!data.is_deleted()) {
-    DCHECK_EQ(type_, syncer::GetModelTypeFromSpecifics(data.specifics));
-  }
-
-  WorkerEntityTracker* entity;
-  EntityMap::const_iterator map_it = entities_.find(data.client_tag_hash);
-  if (map_it == entities_.end()) {
-    scoped_ptr<WorkerEntityTracker> scoped_entity =
-        WorkerEntityTracker::FromCommitRequest(request);
-    entity = scoped_entity.get();
-    entities_.insert(
-        std::make_pair(data.client_tag_hash, std::move(scoped_entity)));
-  } else {
-    entity = map_it->second.get();
-  }
-  entity->RequestCommit(request);
-}
-
 void ModelTypeWorker::OnCommitResponse(
     const CommitResponseDataList& response_list) {
-  for (CommitResponseDataList::const_iterator response_it =
-           response_list.begin();
-       response_it != response_list.end(); ++response_it) {
-    const std::string client_tag_hash = response_it->client_tag_hash;
-    EntityMap::const_iterator map_it = entities_.find(client_tag_hash);
+  for (const 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 (map_it == entities_.end()) {
+    if (entity == nullptr) {
       NOTREACHED() << "Received commit response for item unknown to us."
                    << " Model type: " << ModelTypeToString(type_)
-                   << " ID: " << response_it->id;
+                   << " ID: " << response.id;
       continue;
     }
 
-    WorkerEntityTracker* entity = map_it->second.get();
-    entity->ReceiveCommitResponse(response_it->id,
-                                  response_it->response_version,
-                                  response_it->sequence_number);
+    entity->ReceiveCommitResponse(response.id, response.response_version,
+                                  response.sequence_number);
   }
 
   // Send the responses back to the model thread.  It needs to know which
@@ -454,4 +421,26 @@ bool ModelTypeWorker::DecryptSpecifics(Cryptographer* cryptographer,
   return true;
 }
 
+WorkerEntityTracker* ModelTypeWorker::GetEntityTracker(
+    const std::string& tag_hash) {
+  auto it = entities_.find(tag_hash);
+  return it != entities_.end() ? it->second.get() : nullptr;
+}
+
+WorkerEntityTracker* ModelTypeWorker::CreateEntityTracker(
+    const EntityData& data) {
+  DCHECK(entities_.find(data.client_tag_hash) == entities_.end());
+  scoped_ptr<WorkerEntityTracker> entity =
+      make_scoped_ptr(new WorkerEntityTracker(data.id, data.client_tag_hash));
+  WorkerEntityTracker* entity_ptr = entity.get();
+  entities_[data.client_tag_hash] = std::move(entity);
+  return entity_ptr;
+}
+
+WorkerEntityTracker* ModelTypeWorker::GetOrCreateEntityTracker(
+    const EntityData& data) {
+  WorkerEntityTracker* entity = GetEntityTracker(data.client_tag_hash);
+  return entity ? entity : CreateEntityTracker(data);
+}
+
 }  // namespace syncer_v2
diff --git a/sync/engine/model_type_worker.h b/sync/engine/model_type_worker.h
index 34fc38f..52e830c 100644
--- a/sync/engine/model_type_worker.h
+++ b/sync/engine/model_type_worker.h
@@ -98,9 +98,6 @@ class SYNC_EXPORT ModelTypeWorker : public syncer::UpdateHandler,
  private:
   using EntityMap = std::map<std::string, scoped_ptr<WorkerEntityTracker>>;
 
-  // Stores a single commit request in this object's internal state.
-  void StorePendingCommit(const CommitRequestData& request);
-
   // Returns true if this type has successfully fetched all available updates
   // from the server at least once.  Our state may or may not be stale, but at
   // least we know that it was valid at some point in the past.
@@ -136,13 +133,22 @@ class SYNC_EXPORT ModelTypeWorker : public syncer::UpdateHandler,
                                const sync_pb::EntitySpecifics& in,
                                sync_pb::EntitySpecifics* out);
 
+  // Returns the entity tracker for the given |tag_hash|, or nullptr.
+  WorkerEntityTracker* GetEntityTracker(const std::string& tag_hash);
+
+  // Creates an entity tracker in the map using the given |data| and returns a
+  // pointer to it. Requires that one doesn't exist for data.client_tag_hash.
+  WorkerEntityTracker* CreateEntityTracker(const EntityData& data);
+
+  // Gets the entity tracker for |data| or creates one if it doesn't exist.
+  WorkerEntityTracker* GetOrCreateEntityTracker(const EntityData& data);
+
   syncer::ModelType type_;
 
   // State that applies to the entire model type.
   sync_pb::DataTypeState data_type_state_;
 
-  // Pointer to the ModelTypeProcessor associated with this worker.
-  // This is NULL when no proxy is connected..
+  // Pointer to the ModelTypeProcessor associated with this worker. Never null.
   scoped_ptr<ModelTypeProcessor> model_type_processor_;
 
   // A private copy of the most recent cryptographer known to sync.
diff --git a/sync/engine/worker_entity_tracker.cc b/sync/engine/worker_entity_tracker.cc
index 33ab384..b7c9a3376 100644
--- a/sync/engine/worker_entity_tracker.cc
+++ b/sync/engine/worker_entity_tracker.cc
@@ -14,29 +14,16 @@
 
 namespace syncer_v2 {
 
-scoped_ptr<WorkerEntityTracker> WorkerEntityTracker::FromUpdateResponse(
-    const UpdateResponseData& data) {
-  return make_scoped_ptr(new WorkerEntityTracker(
-      data.entity->id, data.entity->client_tag_hash, 0, data.response_version));
-}
-
-scoped_ptr<WorkerEntityTracker> WorkerEntityTracker::FromCommitRequest(
-    const CommitRequestData& data) {
-  return make_scoped_ptr(new WorkerEntityTracker(
-      data.entity->id, data.entity->client_tag_hash, 0, 0));
-}
-
-WorkerEntityTracker::WorkerEntityTracker(
-    const std::string& id,
-    const std::string& client_tag_hash,
-    int64_t highest_commit_response_version,
-    int64_t highest_gu_response_version)
+WorkerEntityTracker::WorkerEntityTracker(const std::string& id,
+                                         const std::string& client_tag_hash)
     : id_(id),
       client_tag_hash_(client_tag_hash),
-      highest_commit_response_version_(highest_commit_response_version),
-      highest_gu_response_version_(highest_gu_response_version),
+      highest_commit_response_version_(0),
+      highest_gu_response_version_(0),
       sequence_number_(0),
-      base_version_(kUncommittedVersion) {}
+      base_version_(kUncommittedVersion) {
+  DCHECK(!client_tag_hash_.empty());
+}
 
 WorkerEntityTracker::~WorkerEntityTracker() {}
 
diff --git a/sync/engine/worker_entity_tracker.h b/sync/engine/worker_entity_tracker.h
index 5f413e0..b4af987 100644
--- a/sync/engine/worker_entity_tracker.h
+++ b/sync/engine/worker_entity_tracker.h
@@ -33,15 +33,12 @@ struct UpdateResponseData;
 // update, or both.
 class SYNC_EXPORT WorkerEntityTracker {
  public:
-  ~WorkerEntityTracker();
-
-  // Initialize a new entity based on an update response.
-  static scoped_ptr<WorkerEntityTracker> FromUpdateResponse(
-      const UpdateResponseData& data);
+  // Initializes the entity tracker's main fields. Does not initialize state
+  // related to a pending commit.
+  WorkerEntityTracker(const std::string& id,
+                      const std::string& client_tag_hash);
 
-  // Initialize a new entity based on a commit request.
-  static scoped_ptr<WorkerEntityTracker> FromCommitRequest(
-      const CommitRequestData& data);
+  ~WorkerEntityTracker();
 
   // Returns true if this entity should be commited to the server.
   bool HasPendingCommit() const;
@@ -83,13 +80,6 @@ class SYNC_EXPORT WorkerEntityTracker {
   void ClearPendingUpdate();
 
  private:
-  // Initializes the entity tracker's main fields. Does not initialize state
-  // related to a pending commit.
-  WorkerEntityTracker(const std::string& id,
-                      const std::string& client_tag_hash,
-                      int64_t highest_commit_response_version,
-                      int64_t highest_gu_response_version);
-
   // Checks if the current state indicates a conflict.
   //
   // This can be true only while a call to this object is in progress.
diff --git a/sync/engine/worker_entity_tracker_unittest.cc b/sync/engine/worker_entity_tracker_unittest.cc
index 8a30b6a..871b82f 100644
--- a/sync/engine/worker_entity_tracker_unittest.cc
+++ b/sync/engine/worker_entity_tracker_unittest.cc
@@ -34,7 +34,8 @@ class WorkerEntityTrackerTest : public ::testing::Test {
             syncer::syncable::GenerateSyncableHash(syncer::PREFERENCES,
                                                    kClientTag)),
         kCtime(base::Time::UnixEpoch() + base::TimeDelta::FromDays(10)),
-        kMtime(base::Time::UnixEpoch() + base::TimeDelta::FromDays(20)) {
+        kMtime(base::Time::UnixEpoch() + base::TimeDelta::FromDays(20)),
+        entity_(new WorkerEntityTracker(kServerId, kClientTagHash)) {
     specifics.mutable_preference()->set_name(kClientTag);
     specifics.mutable_preference()->set_value("pref.value");
   }
@@ -75,16 +76,14 @@ class WorkerEntityTrackerTest : public ::testing::Test {
   const base::Time kCtime;
   const base::Time kMtime;
   sync_pb::EntitySpecifics specifics;
+  scoped_ptr<WorkerEntityTracker> entity_;
 };
 
 // Construct a new entity from a server update.  Then receive another update.
 TEST_F(WorkerEntityTrackerTest, FromUpdateResponse) {
-  scoped_ptr<WorkerEntityTracker> entity(
-      WorkerEntityTracker::FromUpdateResponse(MakeUpdateResponseData(10)));
-  EXPECT_FALSE(entity->HasPendingCommit());
-
-  entity->ReceiveUpdate(20);
-  EXPECT_FALSE(entity->HasPendingCommit());
+  EXPECT_FALSE(entity_->HasPendingCommit());
+  entity_->ReceiveUpdate(20);
+  EXPECT_FALSE(entity_->HasPendingCommit());
 }
 
 // Construct a new entity from a commit request.  Then serialize it.
@@ -92,14 +91,12 @@ TEST_F(WorkerEntityTrackerTest, FromCommitRequest) {
   const int64_t kSequenceNumber = 22;
   const int64_t kBaseVersion = 33;
   CommitRequestData data = MakeCommitRequestData(kSequenceNumber, kBaseVersion);
-  scoped_ptr<WorkerEntityTracker> entity(
-      WorkerEntityTracker::FromCommitRequest(data));
-  entity->RequestCommit(data);
+  entity_->RequestCommit(data);
 
-  ASSERT_TRUE(entity->HasPendingCommit());
+  ASSERT_TRUE(entity_->HasPendingCommit());
   sync_pb::SyncEntity pb_entity;
   int64_t sequence_number = 0;
-  entity->PrepareCommitProto(&pb_entity, &sequence_number);
+  entity_->PrepareCommitProto(&pb_entity, &sequence_number);
   EXPECT_EQ(kSequenceNumber, sequence_number);
   EXPECT_EQ(kServerId, pb_entity.id_string());
   EXPECT_EQ(kClientTagHash, pb_entity.client_defined_unique_tag());
@@ -115,50 +112,41 @@ TEST_F(WorkerEntityTrackerTest, FromCommitRequest) {
 
 // Start with a server initiated entity.  Commit over top of it.
 TEST_F(WorkerEntityTrackerTest, RequestCommit) {
-  scoped_ptr<WorkerEntityTracker> entity(
-      WorkerEntityTracker::FromUpdateResponse(MakeUpdateResponseData(10)));
-
-  entity->RequestCommit(MakeCommitRequestData(1, 10));
-
-  EXPECT_TRUE(entity->HasPendingCommit());
+  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) {
-  scoped_ptr<WorkerEntityTracker> entity(
-      WorkerEntityTracker::FromUpdateResponse(MakeUpdateResponseData(10)));
-  EXPECT_FALSE(entity->HasPendingCommit());
-
-  entity->RequestCommit(MakeCommitRequestData(23, 5 /* base_version 5 < 10 */));
-  EXPECT_FALSE(entity->HasPendingCommit());
+  entity_->ReceiveUpdate(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);
-  scoped_ptr<WorkerEntityTracker> entity(
-      WorkerEntityTracker::FromCommitRequest(data));
-  entity->RequestCommit(data);
+  entity_->RequestCommit(data);
 
-  EXPECT_TRUE(entity->HasPendingCommit());
+  EXPECT_TRUE(entity_->HasPendingCommit());
 
-  entity->ReceiveUpdate(400);  // Version 400 > 33.
-  EXPECT_FALSE(entity->HasPendingCommit());
+  entity_->ReceiveUpdate(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);
-  scoped_ptr<WorkerEntityTracker> entity(
-      WorkerEntityTracker::FromCommitRequest(data));
-  entity->RequestCommit(data);
+  entity_->RequestCommit(data);
 
-  EXPECT_TRUE(entity->HasPendingCommit());
+  EXPECT_TRUE(entity_->HasPendingCommit());
 
-  entity->ReceiveUpdate(33);  // Version 33 == 33.
-  EXPECT_TRUE(entity->HasPendingCommit());
+  entity_->ReceiveUpdate(33);  // Version 33 == 33.
+  EXPECT_TRUE(entity_->HasPendingCommit());
 }
 
 }  // namespace syncer_v2