[Sync] USS: Load data for pending commits on startup.

This change also includes fairly massive changes to how the SMTP test
file works. Specifically, there is now a SimpleStore for data and
metadata, and the tests have been modified to assert against the stores.
This was done to allow for more intuitive, flexible testing.

Other minor changes:

- Cleaned up some unnecessary functions in SMTP and ModelTypeEntity.
- Added helper functions for accessing ModelTypeEntity objects.
- Removed FakeMetadataChangeList. Simple* is sufficient for testing.
- Switch to using map[k] = v over map.insert(make_pair(k, v)) as the
  former looks cleaner and does not silently fail if a value for k
  already exists in the map.

BUG=569642

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

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

components/sync_driver/non_blocking_data_type_controller_unittest.cc

@@ -261,7 +261,9 @@ class NonBlockingDataTypeControllerTest : public testing::Test,
   }
 
   void LoadModels() {
-    OnMetadataLoaded();
+    if (!type_processor_->IsAllowingChanges()) {
+      OnMetadataLoaded();
+    }
     controller_->LoadModels(
         base::Bind(&NonBlockingDataTypeControllerTest::LoadModelsDone,
                    base::Unretained(this)));

sync/BUILD.gn

@@ -554,7 +554,6 @@ static_library("test_support_sync_core") {
 static_library("test_support_sync_internal_api") {
   testonly = true
   sources = [
-    "internal_api/public/test/fake_metadata_change_list.h",
     "internal_api/public/test/fake_model_type_service.h",
     "internal_api/public/test/fake_sync_manager.h",
     "internal_api/public/test/model_type_store_test_util.h",
@@ -563,7 +562,6 @@ static_library("test_support_sync_internal_api") {
     "internal_api/public/test/test_entry_factory.h",
     "internal_api/public/test/test_internal_components_factory.h",
     "internal_api/public/test/test_user_share.h",
-    "internal_api/test/fake_metadata_change_list.cc",
     "internal_api/test/fake_model_type_service.cc",
     "internal_api/test/fake_sync_manager.cc",
     "internal_api/test/model_type_store_test_util.cc",

sync/internal_api/model_type_entity.cc

@@ -51,7 +51,12 @@ ModelTypeEntity::ModelTypeEntity(const std::string& client_tag,
 ModelTypeEntity::~ModelTypeEntity() {}
 
 void ModelTypeEntity::CacheCommitData(EntityData* data) {
+  DCHECK(RequiresCommitRequest());
+  if (data->client_tag_hash.empty()) {
+    data->client_tag_hash = metadata_.client_tag_hash();
+  }
   commit_data_ = data->PassToPtr();
+  DCHECK(HasCommitData());
 }
 
 bool ModelTypeEntity::HasCommitData() const {
@@ -66,6 +71,10 @@ bool ModelTypeEntity::RequiresCommitRequest() const {
   return metadata_.sequence_number() > commit_requested_sequence_number_;
 }
 
+bool ModelTypeEntity::RequiresCommitData() const {
+  return RequiresCommitRequest() && !HasCommitData() && !metadata_.is_deleted();
+}
+
 bool ModelTypeEntity::UpdateIsReflection(int64_t update_version) const {
   return metadata_.server_version() >= update_version;
 }
@@ -137,26 +146,27 @@ void ModelTypeEntity::Delete() {
   IncrementSequenceNumber();
   metadata_.set_is_deleted(true);
   metadata_.clear_specifics_hash();
-
-  EntityData data;
-  data.client_tag_hash = metadata_.client_tag_hash();
-  data.id = metadata_.server_id();
-  data.creation_time = syncer::ProtoTimeToTime(metadata_.creation_time());
-
-  CacheCommitData(&data);
+  // Clear any cached pending commit data.
+  if (HasCommitData()) commit_data_.reset();
 }
 
-void ModelTypeEntity::InitializeCommitRequestData(
-    CommitRequestData* request) const {
-  DCHECK(HasCommitData());
-  DCHECK_EQ(commit_data_->client_tag_hash, metadata_.client_tag_hash());
+void ModelTypeEntity::InitializeCommitRequestData(CommitRequestData* request) {
+  if (!metadata_.is_deleted()) {
+    DCHECK(HasCommitData());
+    DCHECK_EQ(commit_data_->client_tag_hash, metadata_.client_tag_hash());
+    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 = syncer::ProtoTimeToTime(metadata_.creation_time());
+    request->entity = data.PassToPtr();
+  }
 
   request->sequence_number = metadata_.sequence_number();
   request->base_version = metadata_.server_version();
-  request->entity = commit_data_;
-}
-
-void ModelTypeEntity::SetCommitRequestInProgress() {
   commit_requested_sequence_number_ = metadata_.sequence_number();
 }
 
@@ -178,19 +188,6 @@ void ModelTypeEntity::ClearTransientSyncState() {
   commit_requested_sequence_number_ = metadata_.acked_sequence_number();
 }
 
-void ModelTypeEntity::ClearSyncState() {
-  // TODO(stanisc): crbug/561830: Need to review this entire method. It looks
-  // like the tests expect this to reset some metadata state but not the data.
-  // We should be able to reimplement this once we have the code fore
-  // fetching the data from the service.
-  metadata_.set_server_version(kUncommittedVersion);
-  // TODO(stanisc): Why is this 1 and not 0? This leaves the item unsynced.
-  metadata_.set_sequence_number(1);
-  metadata_.set_acked_sequence_number(0);
-  metadata_.clear_server_id();
-  commit_requested_sequence_number_ = 0;
-}
-
 void ModelTypeEntity::IncrementSequenceNumber() {
   DCHECK(metadata_.has_sequence_number());
   metadata_.set_sequence_number(metadata_.sequence_number() + 1);

sync/internal_api/model_type_entity_unittest.cc

@@ -228,7 +228,8 @@ TEST_F(ModelTypeEntityTest, LocalDeletion) {
   entity->Delete();
   EXPECT_FALSE(HasSpecificsHash(entity));
 
-  EXPECT_TRUE(entity->HasCommitData());
+  EXPECT_FALSE(entity->HasCommitData());
+  EXPECT_FALSE(entity->RequiresCommitData());
   EXPECT_TRUE(entity->IsUnsynced());
 
   EXPECT_TRUE(entity->UpdateIsReflection(10));

sync/internal_api/public/model_type_entity.h

@@ -60,6 +60,10 @@ class SYNC_EXPORT ModelTypeEntity {
   // this item.
   bool RequiresCommitRequest() const;
 
+  // Whether commit data is needed to be cached before a commit request can be
+  // created. Note that deletions do not require cached data.
+  bool RequiresCommitData() const;
+
   // Returns true if the specified update version does not contain new data.
   bool UpdateIsReflection(int64_t update_version) const;
 
@@ -85,11 +89,8 @@ class SYNC_EXPORT ModelTypeEntity {
   void Delete();
 
   // Initializes a message representing this item's uncommitted state
-  // to be forwarded to the sync server for committing.
-  void InitializeCommitRequestData(CommitRequestData* request) const;
-
-  // Notes that the current version of this item has been queued for commit.
-  void SetCommitRequestInProgress();
+  // and assumes that it is forwarded to the sync engine for commiting.
+  void InitializeCommitRequestData(CommitRequestData* request);
 
   // Receives a successful commit response.
   //
@@ -107,9 +108,6 @@ class SYNC_EXPORT ModelTypeEntity {
   // Clears any in-memory sync state associated with outstanding commits.
   void ClearTransientSyncState();
 
-  // Clears all sync state.  Invoked when a user signs out.
-  void ClearSyncState();
-
   // Takes the passed commit data and caches it in the instance.
   // The data is swapped from the input struct without copying.
   void CacheCommitData(EntityData* data);

sync/internal_api/public/model_type_processor.h

@@ -35,7 +35,7 @@ class SYNC_EXPORT ModelTypeProcessor {
   // handle.
   virtual void OnUpdateReceived(
       const sync_pb::DataTypeState& type_state,
-      const UpdateResponseDataList& response_list,
+      const UpdateResponseDataList& updates,
       const UpdateResponseDataList& pending_updates) = 0;
 };
 

sync/internal_api/public/shared_model_type_processor.h

@@ -11,6 +11,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/non_thread_safe.h"
+#include "sync/api/data_batch.h"
 #include "sync/api/metadata_batch.h"
 #include "sync/api/metadata_change_list.h"
 #include "sync/api/model_type_change_processor.h"
@@ -89,7 +90,7 @@ class SYNC_EXPORT SharedModelTypeProcessor : public ModelTypeProcessor,
   void OnCommitCompleted(const sync_pb::DataTypeState& type_state,
                          const CommitResponseDataList& response_list) override;
   void OnUpdateReceived(const sync_pb::DataTypeState& type_state,
-                        const UpdateResponseDataList& response_list,
+                        const UpdateResponseDataList& updates,
                         const UpdateResponseDataList& pending_updates) override;
 
  private:
@@ -98,21 +99,29 @@ class SYNC_EXPORT SharedModelTypeProcessor : public ModelTypeProcessor,
   using EntityMap = std::map<std::string, scoped_ptr<ModelTypeEntity>>;
   using UpdateMap = std::map<std::string, scoped_ptr<UpdateResponseData>>;
 
-  // Complete the start process.
-  void ReadyToConnect();
+  // Callback for ModelTypeService::GetData(). Used when we need to load data
+  // for pending commits during the initialization process.
+  void OnDataLoaded(syncer::SyncError error, scoped_ptr<DataBatch> data_batch);
+
+  // Check conditions, and if met inform sync that we are ready to connect.
+  void ConnectIfReady();
 
   // Handle the first update received from the server after being enabled.
   void OnInitialUpdateReceived(const sync_pb::DataTypeState& type_state,
-                               const UpdateResponseDataList& response_list,
+                               const UpdateResponseDataList& updates,
                                const UpdateResponseDataList& pending_updates);
 
   // Sends all commit requests that are due to be sent to the sync thread.
   void FlushPendingCommitRequests();
 
-  // Clears any state related to outstanding communications with the
-  // CommitQueue.  Used when we want to disconnect from
-  // the current worker.
-  void ClearTransientSyncState();
+  // Computes the client tag hash for the given client |tag|.
+  std::string GetHashForTag(const std::string& tag);
+
+  // Gets the entity for the given tag, which must exist.
+  ModelTypeEntity* GetEntityForTag(const std::string& tag);
+
+  // Gets the entity for the given tag hash, which must exist.
+  ModelTypeEntity* GetEntityForTagHash(const std::string& tag_hash);
 
   syncer::ModelType type_;
   sync_pb::DataTypeState data_type_state_;
@@ -123,6 +132,9 @@ class SYNC_EXPORT SharedModelTypeProcessor : public ModelTypeProcessor,
   // Indicates whether the metadata has finished loading.
   bool is_metadata_loaded_;
 
+  // Indicates whether data for pending commits has finished loading.
+  bool is_pending_commit_data_loaded_;
+
   // Reference to the CommitQueue.
   //
   // The interface hides the posting of tasks across threads as well as the
@@ -148,7 +160,7 @@ class SYNC_EXPORT SharedModelTypeProcessor : public ModelTypeProcessor,
   // thread, we want to make sure that no tasks generated as part of the
   // now-obsolete connection to affect us.  But we also want the WeakPtr we
   // sent to the UI thread to remain valid.
-  base::WeakPtrFactory<SharedModelTypeProcessor> weak_ptr_factory_for_ui_;
+  base::WeakPtrFactory<SharedModelTypeProcessor> weak_ptr_factory_;
   base::WeakPtrFactory<SharedModelTypeProcessor> weak_ptr_factory_for_sync_;
 };