[Sync] USS: Remove pending updates from SharedModelTypeProcessor.

We are planning to handle these in the worker in a future update.

BUG=529498

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

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

5 files changed, 11 insertions, 176 deletions

diff --git a/sync/internal_api/public/activation_context.h b/sync/internal_api/public/activation_context.h
index 48d9476..c6d2146 100644
--- a/sync/internal_api/public/activation_context.h
+++ b/sync/internal_api/public/activation_context.h
@@ -24,10 +24,6 @@ struct SYNC_EXPORT ActivationContext {
   // Initial DataTypeState at the moment of activation.
   sync_pb::DataTypeState data_type_state;
 
-  // Pending updates from the previous session.
-  // TODO(stanisc): crbug.com/529498: should remove pending updates.
-  UpdateResponseDataList saved_pending_updates;
-
   // The ModelTypeProcessor for the worker. Note that this is owned because
   // it is generally a proxy object to the real processor.
   scoped_ptr<ModelTypeProcessor> type_processor;
diff --git a/sync/internal_api/public/model_type_processor.h b/sync/internal_api/public/model_type_processor.h
index 2f1c724..694d3f5 100644
--- a/sync/internal_api/public/model_type_processor.h
+++ b/sync/internal_api/public/model_type_processor.h
@@ -35,8 +35,7 @@ class SYNC_EXPORT ModelTypeProcessor {
   // handle.
   virtual void OnUpdateReceived(
       const sync_pb::DataTypeState& type_state,
-      const UpdateResponseDataList& updates,
-      const UpdateResponseDataList& pending_updates) = 0;
+      const UpdateResponseDataList& updates) = 0;
 };
 
 }  // namespace syncer_v2
diff --git a/sync/internal_api/public/shared_model_type_processor.h b/sync/internal_api/public/shared_model_type_processor.h
index b9034fe..d365f25 100644
--- a/sync/internal_api/public/shared_model_type_processor.h
+++ b/sync/internal_api/public/shared_model_type_processor.h
@@ -73,14 +73,6 @@ class SYNC_EXPORT SharedModelTypeProcessor : public ModelTypeProcessor,
               MetadataChangeList* metadata_change_list) override;
   void OnMetadataLoaded(scoped_ptr<MetadataBatch> batch) override;
 
-  // Returns the list of pending updates.
-  //
-  // This is used as a helper function, but it's public mainly for testing.
-  // The current test harness setup doesn't allow us to test the data that the
-  // proxy sends to the worker during initialization, so we use this to inspect
-  // its state instead.
-  UpdateResponseDataList GetPendingUpdates();
-
   // Returns the long-lived WeakPtr that is intended to be registered with the
   // ProfileSyncService.
   base::WeakPtr<SharedModelTypeProcessor> AsWeakPtrForUI();
@@ -90,8 +82,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& updates,
-                        const UpdateResponseDataList& pending_updates) override;
+                        const UpdateResponseDataList& updates) override;
 
  private:
   friend class SharedModelTypeProcessorTest;
@@ -148,11 +139,6 @@ class SYNC_EXPORT SharedModelTypeProcessor : public ModelTypeProcessor,
   // The set of sync entities known to this object.
   EntityMap entities_;
 
-  // A set of updates that can not be applied at this time.  These are never
-  // used by the model.  They are kept here only so we can save and restore
-  // them across restarts, and keep them in sync with our progress markers.
-  UpdateMap pending_updates_map_;
-
   // ModelTypeService linked to this processor.
   // The service owns this processor instance so the pointer should never
   // become invalid.
diff --git a/sync/internal_api/shared_model_type_processor.cc b/sync/internal_api/shared_model_type_processor.cc
index 3db885f..0956151 100644
--- a/sync/internal_api/shared_model_type_processor.cc
+++ b/sync/internal_api/shared_model_type_processor.cc
@@ -30,8 +30,7 @@ class ModelTypeProcessorProxy : 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& updates,
-                        const UpdateResponseDataList& pending_updates) override;
+                        const UpdateResponseDataList& updates) override;
 
  private:
   base::WeakPtr<ModelTypeProcessor> processor_;
@@ -61,11 +60,10 @@ void ModelTypeProcessorProxy::OnCommitCompleted(
 
 void ModelTypeProcessorProxy::OnUpdateReceived(
     const sync_pb::DataTypeState& type_state,
-    const UpdateResponseDataList& updates,
-    const UpdateResponseDataList& pending_updates) {
+    const UpdateResponseDataList& updates) {
   processor_task_runner_->PostTask(
       FROM_HERE, base::Bind(&ModelTypeProcessor::OnUpdateReceived, processor_,
-                            type_state, updates, pending_updates));
+                            type_state, updates));
 }
 
 }  // namespace
@@ -154,7 +152,6 @@ void SharedModelTypeProcessor::ConnectIfReady() {
   scoped_ptr<ActivationContext> activation_context =
       make_scoped_ptr(new ActivationContext);
   activation_context->data_type_state = data_type_state_;
-  activation_context->saved_pending_updates = GetPendingUpdates();
   activation_context->type_processor = make_scoped_ptr(
       new ModelTypeProcessorProxy(weak_ptr_factory_for_sync_.GetWeakPtr(),
                                   base::ThreadTaskRunnerHandle::Get()));
@@ -357,8 +354,7 @@ void SharedModelTypeProcessor::OnCommitCompleted(
 
 void SharedModelTypeProcessor::OnUpdateReceived(
     const sync_pb::DataTypeState& data_type_state,
-    const UpdateResponseDataList& updates,
-    const UpdateResponseDataList& pending_updates) {
+    const UpdateResponseDataList& updates) {
   if (!data_type_state_.initial_sync_done()) {
     OnInitialUpdateReceived(data_type_state, updates);
     return;
@@ -378,10 +374,6 @@ void SharedModelTypeProcessor::OnUpdateReceived(
     const EntityData& data = update.entity.value();
     const std::string& client_tag_hash = data.client_tag_hash;
 
-    // If we're being asked to apply an update to this entity, this overrides
-    // the previous pending updates.
-    pending_updates_map_.erase(client_tag_hash);
-
     ModelTypeEntity* entity = GetEntityForTagHash(client_tag_hash);
     if (entity == nullptr) {
       if (data.is_deleted()) {
@@ -434,24 +426,6 @@ void SharedModelTypeProcessor::OnUpdateReceived(
     }
   }
 
-  // TODO(stanisc): crbug.com/529498: stop saving pending updates.
-  // Save pending updates in the appropriate data structure.
-  for (const UpdateResponseData& update : pending_updates) {
-    const std::string& client_tag_hash = update.entity->client_tag_hash;
-
-    auto lookup_it = pending_updates_map_.find(client_tag_hash);
-    if (lookup_it == pending_updates_map_.end()) {
-      pending_updates_map_.insert(std::make_pair(
-          client_tag_hash, make_scoped_ptr(new UpdateResponseData(update))));
-    } else if (lookup_it->second->response_version <= update.response_version) {
-      pending_updates_map_.erase(lookup_it);
-      pending_updates_map_.insert(std::make_pair(
-          client_tag_hash, make_scoped_ptr(new UpdateResponseData(update))));
-    } else {
-      // Received update is stale, do not overwrite existing.
-    }
-  }
-
   if (got_new_encryption_requirements) {
     for (auto it = entities_.begin(); it != entities_.end(); ++it) {
       it->second->UpdateDesiredEncryptionKey(
@@ -503,15 +477,6 @@ void SharedModelTypeProcessor::OnInitialUpdateReceived(
   FlushPendingCommitRequests();
 }
 
-UpdateResponseDataList SharedModelTypeProcessor::GetPendingUpdates() {
-  UpdateResponseDataList pending_updates_list;
-  for (auto it = pending_updates_map_.begin(); it != pending_updates_map_.end();
-       ++it) {
-    pending_updates_list.push_back(*it->second);
-  }
-  return pending_updates_list;
-}
-
 std::string SharedModelTypeProcessor::GetHashForTag(const std::string& tag) {
   return syncer::syncable::GenerateSyncableHash(type_, tag);
 }
diff --git a/sync/internal_api/shared_model_type_processor_unittest.cc b/sync/internal_api/shared_model_type_processor_unittest.cc
index 5a8cd66..8e1c730 100644
--- a/sync/internal_api/shared_model_type_processor_unittest.cc
+++ b/sync/internal_api/shared_model_type_processor_unittest.cc
@@ -123,8 +123,7 @@ class SimpleStore {
 // The processor sits between the service (implemented by this test class) and
 // the worker, which is represented as a commit queue (MockCommitQueue). This
 // test suite exercises the initialization flows (whether initial sync is done,
-// loading pending updates, performing the initial merge, etc) as well as normal
-// functionality:
+// performing the initial merge, etc) as well as normal functionality:
 //
 // - Initialization before the initial sync and merge correctly performs a merge
 //   and initializes the metadata in storage. TODO(maxbogue): crbug.com/569675.
@@ -253,8 +252,7 @@ class SharedModelTypeProcessorTest : public ::testing::Test,
   void OnInitialSyncDone(UpdateResponseDataList updates) {
     sync_pb::DataTypeState data_type_state(db_.data_type_state());
     data_type_state.set_initial_sync_done(true);
-    type_processor()->OnUpdateReceived(data_type_state, updates,
-                                       UpdateResponseDataList());
+    type_processor()->OnUpdateReceived(data_type_state, updates);
   }
 
   // Overloaded form with no updates.
@@ -280,8 +278,7 @@ class SharedModelTypeProcessorTest : public ::testing::Test,
 
     UpdateResponseDataList list;
     list.push_back(data);
-    type_processor()->OnUpdateReceived(db_.data_type_state(), list,
-                                       UpdateResponseDataList());
+    type_processor()->OnUpdateReceived(db_.data_type_state(), list);
   }
 
   void TombstoneFromServer(int64_t version_offset, const std::string& tag) {
@@ -293,54 +290,7 @@ class SharedModelTypeProcessorTest : public ::testing::Test,
 
     UpdateResponseDataList list;
     list.push_back(data);
-    type_processor()->OnUpdateReceived(db_.data_type_state(), list,
-                                       UpdateResponseDataList());
-  }
-
-  // Emulate the receipt of pending updates from the server.
-  // Pending updates are usually caused by a temporary decryption failure.
-  void PendingUpdateFromServer(int64_t version_offset,
-                               const std::string& tag,
-                               const std::string& value,
-                               const std::string& key_name) {
-    const std::string tag_hash = GenerateTagHash(tag);
-    UpdateResponseData data = mock_queue_->UpdateFromServer(
-        version_offset, tag_hash,
-        GenerateEncryptedSpecifics(tag, value, key_name));
-
-    UpdateResponseDataList list;
-    list.push_back(data);
-    type_processor()->OnUpdateReceived(db_.data_type_state(),
-                                       UpdateResponseDataList(), list);
-  }
-
-  // Returns true if the proxy has an pending update with specified tag.
-  bool HasPendingUpdate(const std::string& tag) const {
-    const std::string client_tag_hash = GenerateTagHash(tag);
-    const UpdateResponseDataList list = type_processor()->GetPendingUpdates();
-    for (const UpdateResponseData& update : list) {
-      if (update.entity->client_tag_hash == client_tag_hash)
-        return true;
-    }
-    return false;
-  }
-
-  // Returns the pending update with the specified tag.
-  UpdateResponseData GetPendingUpdate(const std::string& tag) const {
-    DCHECK(HasPendingUpdate(tag));
-    const std::string client_tag_hash = GenerateTagHash(tag);
-    const UpdateResponseDataList list = type_processor()->GetPendingUpdates();
-    for (const UpdateResponseData& update : list) {
-      if (update.entity->client_tag_hash == client_tag_hash)
-        return update;
-    }
-    NOTREACHED();
-    return UpdateResponseData();
-  }
-
-  // Returns the number of pending updates.
-  size_t GetNumPendingUpdates() const {
-    return type_processor()->GetPendingUpdates().size();
+    type_processor()->OnUpdateReceived(db_.data_type_state(), list);
   }
 
   // Read emitted commit requests as batches.
@@ -377,7 +327,7 @@ class SharedModelTypeProcessorTest : public ::testing::Test,
     sync_pb::DataTypeState data_type_state(db_.data_type_state());
     data_type_state.set_encryption_key_name(key_name);
     type_processor()->OnUpdateReceived(
-        data_type_state, UpdateResponseDataList(), UpdateResponseDataList());
+        data_type_state, UpdateResponseDataList());
   }
 
   // Sets the key_name that the mock CommitQueue will claim is in use
@@ -1170,67 +1120,6 @@ TEST_F(SharedModelTypeProcessorTest, DISABLED_Disable) {
   EXPECT_TRUE(HasCommitRequestForTag("tag3"));
 }
 
-// Test receipt of pending updates.
-TEST_F(SharedModelTypeProcessorTest, ReceivePendingUpdates) {
-  InitializeToReadyState();
-
-  EXPECT_FALSE(HasPendingUpdate("tag1"));
-  EXPECT_EQ(0U, GetNumPendingUpdates());
-
-  // Receive a pending update.
-  PendingUpdateFromServer(5, "tag1", "value1", "key1");
-  EXPECT_EQ(1U, GetNumPendingUpdates());
-  ASSERT_TRUE(HasPendingUpdate("tag1"));
-  UpdateResponseData data1 = GetPendingUpdate("tag1");
-  EXPECT_EQ(5, data1.response_version);
-
-  // Receive an updated version of a pending update.
-  // It should overwrite the existing item.
-  PendingUpdateFromServer(10, "tag1", "value15", "key1");
-  EXPECT_EQ(1U, GetNumPendingUpdates());
-  ASSERT_TRUE(HasPendingUpdate("tag1"));
-  UpdateResponseData data2 = GetPendingUpdate("tag1");
-  EXPECT_EQ(15, data2.response_version);
-
-  // Receive a stale version of a pending update.
-  // It should have no effect.
-  PendingUpdateFromServer(-3, "tag1", "value12", "key1");
-  EXPECT_EQ(1U, GetNumPendingUpdates());
-  ASSERT_TRUE(HasPendingUpdate("tag1"));
-  UpdateResponseData data3 = GetPendingUpdate("tag1");
-  EXPECT_EQ(15, data3.response_version);
-}
-
-// Test that Disable clears pending update state.
-TEST_F(SharedModelTypeProcessorTest, DisableWithPendingUpdates) {
-  InitializeToReadyState();
-
-  PendingUpdateFromServer(5, "tag1", "value1", "key1");
-  EXPECT_EQ(1U, GetNumPendingUpdates());
-  ASSERT_TRUE(HasPendingUpdate("tag1"));
-
-  Disable();
-  InitializeToReadyState();
-
-  EXPECT_EQ(0U, GetNumPendingUpdates());
-  EXPECT_FALSE(HasPendingUpdate("tag1"));
-}
-
-// Test that disconnecting does not clear pending update state.
-TEST_F(SharedModelTypeProcessorTest, DisconnectWithPendingUpdates) {
-  InitializeToReadyState();
-
-  PendingUpdateFromServer(5, "tag1", "value1", "key1");
-  EXPECT_EQ(1U, GetNumPendingUpdates());
-  ASSERT_TRUE(HasPendingUpdate("tag1"));
-
-  DisconnectSync();
-  OnSyncStarting();
-
-  EXPECT_EQ(1U, GetNumPendingUpdates());
-  EXPECT_TRUE(HasPendingUpdate("tag1"));
-}
-
 // Test re-encrypt everything when desired encryption key changes.
 // TODO(stanisc): crbug/561814: Disabled due to data caching changes in
 // ModelTypeEntity. Revisit the test once fetching of data is implemented.