[Sync::USS] Implement NigoriModelTypeProcessor::OnCommitCompleted()

Bug: 922900
Change-Id: Ib954f8d9e059ab86b19e77fcdf411b887a725014
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1576559
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#653624}

components/sync/nigori/nigori_model_type_processor.cc

@@ -15,7 +15,8 @@ namespace syncer {
 
 namespace {
 
-// TODO(mamir): remove those and adjust the code accordingly.
+// TODO(mamir): remove those and adjust the code accordingly. Similarly in
+// tests.
 const char kNigoriStorageKey[] = "NigoriStorageKey";
 const char kNigoriClientTagHash[] = "NigoriClientTagHash";
 
@@ -89,7 +90,20 @@ void NigoriModelTypeProcessor::OnCommitCompleted(
     const sync_pb::ModelTypeState& type_state,
     const CommitResponseDataList& response_list) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  NOTIMPLEMENTED();
+  DCHECK(entity_);
+
+  model_type_state_ = type_state;
+  if (!response_list.empty()) {
+    entity_->ReceiveCommitResponse(response_list[0], /*commit_only=*/false,
+                                   ModelType::NIGORI);
+  } else {
+    // If the entity hasn't been mentioned in response_list, then it's not
+    // committed and we should reset its commit_requested_sequence_number so
+    // they are committed again on next sync cycle.
+    entity_->ClearTransientSyncState();
+  }
+  // Ask the bridge to persist the new metadata.
+  bridge_->ApplySyncChanges(/*data=*/base::nullopt);
 }
 
 void NigoriModelTypeProcessor::OnUpdateReceived(

components/sync/nigori/nigori_model_type_processor_unittest.cc

@@ -26,6 +26,9 @@ using testing::Eq;
 using testing::Ne;
 using testing::NotNull;
 
+// TODO(mamir): remove those and adjust the code accordingly.
+const char kNigoriClientTagHash[] = "NigoriClientTagHash";
+
 const char kNigoriNonUniqueName[] = "nigori";
 const char kNigoriServerId[] = "nigori_server_id";
 const char kCacheGuid[] = "generated_id";
@@ -67,6 +70,19 @@ std::unique_ptr<syncer::UpdateResponseData> CreateDummyNigoriUpdateResponseData(
   return response_data;
 }
 
+CommitResponseData CreateNigoriCommitResponseData(
+    const CommitRequestData& commit_request_data,
+    int response_version) {
+  CommitResponseData commit_response_data;
+  commit_response_data.id = kNigoriServerId;
+  commit_response_data.client_tag_hash = kNigoriClientTagHash;
+  commit_response_data.sequence_number = commit_request_data.sequence_number;
+  commit_response_data.response_version = response_version;
+  commit_response_data.specifics_hash = commit_request_data.specifics_hash;
+  commit_response_data.unsynced_time = commit_request_data.unsynced_time;
+  return commit_response_data;
+}
+
 class MockNigoriSyncBridge : public NigoriSyncBridge {
  public:
   MockNigoriSyncBridge() = default;
@@ -217,6 +233,123 @@ TEST_F(NigoriModelTypeProcessorTest, ShouldGetLocalChangesWhenPut) {
   EXPECT_EQ(kNigoriNonUniqueName, commit_request[0]->entity->non_unique_name);
 }
 
+TEST_F(NigoriModelTypeProcessorTest,
+       ShouldSquashCommitRequestUponCommitCompleted) {
+  SimulateModelReadyToSync(/*initial_sync_done=*/true);
+
+  auto entity_data = std::make_unique<syncer::EntityData>();
+  entity_data->specifics.mutable_nigori();
+  entity_data->non_unique_name = kNigoriNonUniqueName;
+
+  processor()->Put(std::move(entity_data));
+  CommitRequestDataList commit_request_list;
+  processor()->GetLocalChanges(
+      /*max_entries=*/10,
+      base::BindOnce(&CaptureCommitRequest, &commit_request_list));
+  ASSERT_EQ(1U, commit_request_list.size());
+
+  CommitResponseDataList commit_response_list;
+  commit_response_list.push_back(CreateNigoriCommitResponseData(
+      *commit_request_list[0], /*response_version=*/processor()
+                                       ->GetMetadata()
+                                       .entity_metadata->server_version() +
+                                   1));
+
+  // ApplySyncChanges() should be called to trigger persistence of the metadata.
+  EXPECT_CALL(*mock_nigori_sync_bridge(), ApplySyncChanges(Eq(base::nullopt)));
+  processor()->OnCommitCompleted(CreateDummyModelTypeState(),
+                                 std::move(commit_response_list));
+
+  // There should be no more local changes.
+  commit_response_list.clear();
+  processor()->GetLocalChanges(
+      /*max_entries=*/10,
+      base::BindOnce(&CaptureCommitRequest, &commit_request_list));
+  EXPECT_TRUE(commit_request_list.empty());
+}
+
+TEST_F(NigoriModelTypeProcessorTest,
+       ShouldNotSquashCommitRequestUponEmptyCommitResponse) {
+  SimulateModelReadyToSync(/*initial_sync_done=*/true);
+
+  auto entity_data = std::make_unique<syncer::EntityData>();
+  entity_data->specifics.mutable_nigori();
+  entity_data->non_unique_name = kNigoriNonUniqueName;
+
+  processor()->Put(std::move(entity_data));
+  CommitRequestDataList commit_request_list;
+  processor()->GetLocalChanges(
+      /*max_entries=*/10,
+      base::BindOnce(&CaptureCommitRequest, &commit_request_list));
+  ASSERT_EQ(1U, commit_request_list.size());
+
+  // ApplySyncChanges() should be called to trigger persistence of the metadata.
+  EXPECT_CALL(*mock_nigori_sync_bridge(), ApplySyncChanges(Eq(base::nullopt)));
+  processor()->OnCommitCompleted(CreateDummyModelTypeState(),
+                                 CommitResponseDataList());
+
+  // Data has been moved into the previous request, so the processor will ask
+  // for the commit data once more.
+  ON_CALL(*mock_nigori_sync_bridge(), GetData()).WillByDefault([&]() {
+    auto entity_data = std::make_unique<syncer::EntityData>();
+    entity_data->specifics.mutable_nigori();
+    entity_data->non_unique_name = kNigoriNonUniqueName;
+    return entity_data;
+  });
+
+  // The commit should still be pending.
+  CommitResponseDataList commit_response_list;
+  processor()->GetLocalChanges(
+      /*max_entries=*/10,
+      base::BindOnce(&CaptureCommitRequest, &commit_request_list));
+  EXPECT_EQ(1U, commit_request_list.size());
+}
+
+TEST_F(NigoriModelTypeProcessorTest,
+       ShouldKeepAnotherCommitRequestUponCommitCompleted) {
+  SimulateModelReadyToSync(/*initial_sync_done=*/true);
+
+  auto entity_data = std::make_unique<syncer::EntityData>();
+  sync_pb::NigoriSpecifics* nigori_specifics =
+      entity_data->specifics.mutable_nigori();
+  nigori_specifics->set_encrypt_bookmarks(true);
+  entity_data->non_unique_name = kNigoriNonUniqueName;
+
+  processor()->Put(std::move(entity_data));
+  CommitRequestDataList commit_request_list;
+  processor()->GetLocalChanges(
+      /*max_entries=*/10,
+      base::BindOnce(&CaptureCommitRequest, &commit_request_list));
+  ASSERT_EQ(1U, commit_request_list.size());
+
+  CommitResponseDataList commit_response_list;
+  commit_response_list.push_back(CreateNigoriCommitResponseData(
+      *commit_request_list[0], /*response_version=*/processor()
+                                       ->GetMetadata()
+                                       .entity_metadata->server_version() +
+                                   1));
+
+  // Make another local change before the commit response is received.
+  entity_data = std::make_unique<syncer::EntityData>();
+  nigori_specifics = entity_data->specifics.mutable_nigori();
+  entity_data->non_unique_name = kNigoriNonUniqueName;
+  nigori_specifics->set_encrypt_preferences(true);
+  processor()->Put(std::move(entity_data));
+
+  // ApplySyncChanges() should be called to trigger persistence of the metadata.
+  EXPECT_CALL(*mock_nigori_sync_bridge(), ApplySyncChanges(Eq(base::nullopt)));
+  // Receive the commit response of the first request.
+  processor()->OnCommitCompleted(CreateDummyModelTypeState(),
+                                 std::move(commit_response_list));
+
+  // There should still be a local change.
+  commit_response_list.clear();
+  processor()->GetLocalChanges(
+      /*max_entries=*/10,
+      base::BindOnce(&CaptureCommitRequest, &commit_request_list));
+  EXPECT_EQ(1U, commit_request_list.size());
+}
+
 TEST_F(NigoriModelTypeProcessorTest,
        ShouldNudgeForCommitUponConnectSyncIfReadyToSyncAndLocalChanges) {
   SimulateModelReadyToSync(/*initial_sync_done=*/true);