[Sync] Fixing two bugs in the worker revealed by trying to add an encryption integration test.

components/sync/engine_impl/model_type_worker_unittest.cc

Index: components/sync/engine_impl/model_type_worker_unittest.cc
diff --git a/components/sync/engine_impl/model_type_worker_unittest.cc b/components/sync/engine_impl/model_type_worker_unittest.cc
index 99fb505f1bc908a96e5da80199692a82289d2ea4..9f337fb154ac5c76a8bd00e6ed52f533d37d3e95 100644
--- a/components/sync/engine_impl/model_type_worker_unittest.cc
+++ b/components/sync/engine_impl/model_type_worker_unittest.cc
@@ -279,30 +279,22 @@ class ModelTypeWorkerTest : public ::testing::Test {
     const std::string tag_hash = GenerateTagHash(name);
     CommitRequestData data = mock_type_processor_->CommitRequest(
         tag_hash, GenerateSpecifics(name, value));
-    CommitRequestDataList list;
-    list.push_back(data);
-    worker_->EnqueueForCommit(list);
+    worker_->EnqueueForCommit({data});
   }
 
   void DeleteRequest(const std::string& tag) {
     const std::string tag_hash = GenerateTagHash(tag);
     CommitRequestData data = mock_type_processor_->DeleteRequest(tag_hash);
-    CommitRequestDataList list;
-    list.push_back(data);
-    worker_->EnqueueForCommit(list);
+    worker_->EnqueueForCommit({data});
   }
 
   // Pretend to receive update messages from the server.
 
   void TriggerTypeRootUpdateFromServer() {
     sync_pb::SyncEntity entity = mock_server_.TypeRootUpdate();
-    SyncEntityList entity_list;
-    entity_list.push_back(&entity);
-
     StatusController dummy_status;
-
     worker_->ProcessGetUpdatesResponse(mock_server_.GetProgress(),
-                                       mock_server_.GetContext(), entity_list,
+                                       mock_server_.GetContext(), {&entity},
                                        &dummy_status);
     worker_->PassiveApplyUpdates(&dummy_status);
   }
@@ -318,13 +310,9 @@ class ModelTypeWorkerTest : public ::testing::Test {
                     entity.mutable_specifics());
     }
 
-    SyncEntityList entity_list;
-    entity_list.push_back(&entity);
-
     StatusController dummy_status;
-
     worker_->ProcessGetUpdatesResponse(mock_server_.GetProgress(),
-                                       mock_server_.GetContext(), entity_list,
+                                       mock_server_.GetContext(), {&entity},
                                        &dummy_status);
   }
 
@@ -346,13 +334,9 @@ class ModelTypeWorkerTest : public ::testing::Test {
                     entity.mutable_specifics());
     }
 
-    SyncEntityList entity_list;
-    entity_list.push_back(&entity);
-
     StatusController dummy_status;
-
     worker_->ProcessGetUpdatesResponse(mock_server_.GetProgress(),
-                                       mock_server_.GetContext(), entity_list,
+                                       mock_server_.GetContext(), {&entity},
                                        &dummy_status);
     worker_->ApplyUpdates(&dummy_status);
   }
@@ -769,9 +753,7 @@ TEST_F(ModelTypeWorkerTest, EncryptionBlocksCommits) {
 
   // Once the cryptographer is returned to a normal state, we should be able to
   // commit again.
-  EXPECT_EQ(1, GetNumCommitNudges());
   UpdateLocalCryptographer();
-  EXPECT_EQ(2, GetNumCommitNudges());
   EXPECT_TRUE(WillCommit());
 
   // Verify the committed entity was properly encrypted.
@@ -886,6 +868,30 @@ TEST_F(ModelTypeWorkerTest, EncryptedUpdateOverridesPendingCommit) {
   EXPECT_EQ(0U, updates_list.size());
 }
 
+// Commit twice, both times with the kUncommittedVersion base version. Then
+// verify the second time through that we see the correct version.
+TEST_F(ModelTypeWorkerTest, OldVersionCommit) {
+  NormalInitialize();
+  CommitRequest(kTag1, kValue1);
+  EXPECT_TRUE(WillCommit());
+  DoSuccessfulCommit();
+  int commit_version = processor()->GetCommitResponse(kHash1).response_version;
+  EXPECT_NE(kUncommittedVersion, commit_version);
+
+  CommitRequestData data = processor()->CommitRequest(
+      GenerateTagHash(kTag1), GenerateSpecifics(kTag1, kValue2));

[maxbogue, 2016/09/21 00:57:14]

Use kHash1.

[skym, 2016/09/22 18:58:47]

Done.

+  // Override the version the processor sets.

[maxbogue, 2016/09/21 00:57:14]

Instead of this comment, I would add line break below line 886 and add a comment
above line 881 along the lines of "Create a custom commit request using the old
base_version."

[skym, 2016/09/22 18:58:49]

Done.

+  data.base_version = kUncommittedVersion;
+  worker()->EnqueueForCommit({data});
+  EXPECT_TRUE(WillCommit());
+  DoSuccessfulCommit();
+  sync_pb::ClientToServerMessage message = server()->GetNthCommitMessage(1);
+  const google::protobuf::RepeatedPtrField<sync_pb::SyncEntity>& entries =

[maxbogue, 2016/09/21 00:57:14]

I almost want to say use const auto& here because this type is a mess. But, it
might not be super clear that the result would be a repeated field so feel free
to leave it.

[skym, 2016/09/22 18:58:50]

Yeah, with auto the .Get(0) doesn't really make any sense. I think the
assumption with auto would be that it's just a generic iterator, which is not
how i'm using it.

+      message.commit().entries();
+  ASSERT_EQ(1, entries.size());
+  EXPECT_EQ(entries.Get(0).version(), commit_version);
+}
+
 // Test decryption of pending updates saved across a restart.
 TEST_F(ModelTypeWorkerTest, RestorePendingEntries) {
   // Create a fake pending update.
@@ -905,9 +911,7 @@ TEST_F(ModelTypeWorkerTest, RestorePendingEntries) {
   update.response_version = 100;
 
   // Inject the update during CommitQueue initialization.
-  UpdateResponseDataList saved_pending_updates;
-  saved_pending_updates.push_back(update);
-  InitializeWithPendingUpdates(saved_pending_updates);
+  InitializeWithPendingUpdates({update});
 
   // Update will be undecryptable at first.
   EXPECT_EQ(0U, processor()->GetNumUpdateResponses());
@@ -949,9 +953,7 @@ TEST_F(ModelTypeWorkerTest, RestoreApplicableEntries) {
   update.response_version = 100;
 
   // Inject the update during CommitQueue initialization.
-  UpdateResponseDataList saved_pending_updates;
-  saved_pending_updates.push_back(update);
-  InitializeWithPendingUpdates(saved_pending_updates);
+  InitializeWithPendingUpdates({update});
 
   // Verify the item gets decrypted and sent back to the model thread.
   // TODO(maxbogue): crbug.com/529498: Uncomment when pending updates are
@@ -1012,11 +1014,8 @@ TEST_F(ModelTypeWorkerTest, ReceiveCorruptEncryption) {
   entity.mutable_specifics()->mutable_encrypted()->mutable_blob()->replace(
       0, 4, "xyz!");
 
-  SyncEntityList entity_list;
-  entity_list.push_back(&entity);
-
   // If a corrupt update could trigger a crash, this is where it would happen.
-  DeliverRawUpdates(entity_list);
+  DeliverRawUpdates({&entity});
 
   EXPECT_FALSE(processor()->HasUpdateResponse(kHash1));