1 files changed, 58 insertions, 44 deletions

diff --git a/sync/engine/process_commit_response_command_unittest.cc b/sync/engine/process_commit_response_command_unittest.cc
index 517bc51..195d58a 100644
--- a/sync/engine/process_commit_response_command_unittest.cc
+++ b/sync/engine/process_commit_response_command_unittest.cc
@@ -47,11 +47,7 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
     (*mutable_routing_info())[syncable::PREFERENCES] = GROUP_UI;
     (*mutable_routing_info())[syncable::AUTOFILL] = GROUP_DB;
 
-    commit_set_.reset(new sessions::OrderedCommitSet(routing_info()));
     SyncerCommandTest::SetUp();
-    // Need to explicitly use this-> to avoid obscure template
-    // warning.
-    this->ExpectNoGroupsToChange(command_);
   }
 
  protected:
@@ -113,12 +109,14 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
   // record and a commit response for it in the syncer session.  If item_id
   // is a local ID, the item will be a create operation.  Otherwise, it
   // will be an edit.
-  void CreateUnprocessedCommitResult(const Id& item_id,
-                                     const Id& parent_id,
-                                     const string& name,
-                                     syncable::ModelType model_type) {
-    sessions::StatusController* sync_state =
-        session()->mutable_status_controller();
+  void CreateUnprocessedCommitResult(
+      const Id& item_id,
+      const Id& parent_id,
+      const string& name,
+      syncable::ModelType model_type,
+      sessions::OrderedCommitSet *commit_set,
+      browser_sync::ClientToServerMessage *commit,
+      browser_sync::ClientToServerResponse *response) {
     bool is_folder = true;
     int64 metahandle = 0;
     CreateUnsyncedItem(item_id, parent_id, name, is_folder, model_type,
@@ -126,18 +124,14 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
 
     // ProcessCommitResponseCommand consumes commit_ids from the session
     // state, so we need to update that.  O(n^2) because it's a test.
-    commit_set_->AddCommitItem(metahandle, item_id, model_type);
-    sync_state->set_commit_set(*commit_set_.get());
+    commit_set->AddCommitItem(metahandle, item_id, model_type);
 
     WriteTransaction trans(FROM_HERE, UNITTEST, directory());
     MutableEntry entry(&trans, syncable::GET_BY_ID, item_id);
     ASSERT_TRUE(entry.good());
     entry.Put(syncable::SYNCING, true);
 
-    // ProcessCommitResponseCommand looks at both the commit message as well
-    // as the commit response, so we need to synthesize both here.
-    sync_pb::ClientToServerMessage* commit =
-        sync_state->mutable_commit_message();
+    // Add to the commit message.
     commit->set_message_contents(ClientToServerMessage::COMMIT);
     SyncEntity* entity = static_cast<SyncEntity*>(
         commit->mutable_commit()->add_entries());
@@ -148,8 +142,7 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
     entity->mutable_specifics()->CopyFrom(entry.Get(syncable::SPECIFICS));
     entity->set_id(item_id);
 
-    sync_pb::ClientToServerResponse* response =
-        sync_state->mutable_commit_response();
+    // Add to the response message.
     response->set_error_code(sync_pb::SyncEnums::SUCCESS);
     sync_pb::CommitResponse_EntryResponse* entry_response =
         response->mutable_commit()->add_entryresponse();
@@ -176,20 +169,15 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
     }
   }
 
-  void SetLastErrorCode(CommitResponse::ResponseType error_code) {
-    sessions::StatusController* sync_state =
-        session()->mutable_status_controller();
-    sync_pb::ClientToServerResponse* response =
-        sync_state->mutable_commit_response();
+  void SetLastErrorCode(CommitResponse::ResponseType error_code,
+                        sync_pb::ClientToServerResponse* response) {
     sync_pb::CommitResponse_EntryResponse* entry_response =
         response->mutable_commit()->mutable_entryresponse(
             response->mutable_commit()->entryresponse_size() - 1);
     entry_response->set_response_type(error_code);
   }
 
-  ProcessCommitResponseCommand command_;
   TestIdFactory id_factory_;
-  scoped_ptr<sessions::OrderedCommitSet> commit_set_;
  private:
   int64 next_old_revision_;
   int64 next_new_revision_;
@@ -198,6 +186,10 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
 };
 
 TEST_F(ProcessCommitResponseCommandTest, MultipleCommitIdProjections) {
+  sessions::OrderedCommitSet commit_set(session()->routing_info());
+  browser_sync::ClientToServerMessage request;
+  browser_sync::ClientToServerResponse response;
+
   Id bookmark_folder_id = id_factory_.NewLocalId();
   Id bookmark_id1 = id_factory_.NewLocalId();
   Id bookmark_id2 = id_factory_.NewLocalId();
@@ -205,22 +197,30 @@ TEST_F(ProcessCommitResponseCommandTest, MultipleCommitIdProjections) {
   Id autofill_id1 = id_factory_.NewLocalId();
   Id autofill_id2 = id_factory_.NewLocalId();
   CreateUnprocessedCommitResult(bookmark_folder_id, id_factory_.root(),
-                                "A bookmark folder", syncable::BOOKMARKS);
+                                "A bookmark folder", syncable::BOOKMARKS,
+                                &commit_set, &request, &response);
   CreateUnprocessedCommitResult(bookmark_id1, bookmark_folder_id,
-                                "bookmark 1", syncable::BOOKMARKS);
+                                "bookmark 1", syncable::BOOKMARKS,
+                                &commit_set, &request, &response);
   CreateUnprocessedCommitResult(bookmark_id2, bookmark_folder_id,
-                                "bookmark 2", syncable::BOOKMARKS);
+                                "bookmark 2", syncable::BOOKMARKS,
+                                &commit_set, &request, &response);
   CreateUnprocessedCommitResult(pref_id1, id_factory_.root(),
-                                "Pref 1", syncable::PREFERENCES);
+                                "Pref 1", syncable::PREFERENCES,
+                                &commit_set, &request, &response);
   CreateUnprocessedCommitResult(pref_id2, id_factory_.root(),
-                                "Pref 2", syncable::PREFERENCES);
+                                "Pref 2", syncable::PREFERENCES,
+                                &commit_set, &request, &response);
   CreateUnprocessedCommitResult(autofill_id1, id_factory_.root(),
-                                "Autofill 1", syncable::AUTOFILL);
+                                "Autofill 1", syncable::AUTOFILL,
+                                &commit_set, &request, &response);
   CreateUnprocessedCommitResult(autofill_id2, id_factory_.root(),
-                                "Autofill 2", syncable::AUTOFILL);
+                                "Autofill 2", syncable::AUTOFILL,
+                                &commit_set, &request, &response);
 
-  ExpectGroupsToChange(command_, GROUP_UI, GROUP_DB);
-  command_.ExecuteImpl(session());
+  ProcessCommitResponseCommand command(commit_set, request, response);
+  ExpectGroupsToChange(command, GROUP_UI, GROUP_DB);
+  command.ExecuteImpl(session());
 
   ReadTransaction trans(FROM_HERE, directory());
   Id new_fid;
@@ -270,10 +270,15 @@ TEST_F(ProcessCommitResponseCommandTest, MultipleCommitIdProjections) {
 // how this scenario used to fail, reversing the order for the second half
 // of the children.
 TEST_F(ProcessCommitResponseCommandTest, NewFolderCommitKeepsChildOrder) {
+  sessions::OrderedCommitSet commit_set(session()->routing_info());
+  browser_sync::ClientToServerMessage request;
+  browser_sync::ClientToServerResponse response;
+
   // Create the parent folder, a new item whose ID will change on commit.
   Id folder_id = id_factory_.NewLocalId();
   CreateUnprocessedCommitResult(folder_id, id_factory_.root(), "A",
-                                syncable::BOOKMARKS);
+                                syncable::BOOKMARKS,
+                                &commit_set, &request, &response);
 
   // Verify that the item is reachable.
   {
@@ -292,7 +297,8 @@ TEST_F(ProcessCommitResponseCommandTest, NewFolderCommitKeepsChildOrder) {
     // Alternate between new and old child items, just for kicks.
     Id id = (i % 4 < 2) ? id_factory_.NewLocalId() : id_factory_.NewServerId();
     CreateUnprocessedCommitResult(
-        id, folder_id, base::StringPrintf("Item %d", i), syncable::BOOKMARKS);
+        id, folder_id, base::StringPrintf("Item %d", i), syncable::BOOKMARKS,
+        &commit_set, &request, &response);
   }
   // The second 25 children will be unsynced items but NOT part of the commit
   // batch.  When the ID of the parent folder changes during the commit,
@@ -309,8 +315,9 @@ TEST_F(ProcessCommitResponseCommandTest, NewFolderCommitKeepsChildOrder) {
   // 25 items.  This should apply the values indicated by
   // each CommitResponse_EntryResponse to the syncable Entries.  All new
   // items in the commit batch should have their IDs changed to server IDs.
-  ExpectGroupToChange(command_, GROUP_UI);
-  command_.ExecuteImpl(session());
+  ProcessCommitResponseCommand command(commit_set, request, response);
+  ExpectGroupToChange(command, GROUP_UI);
+  command.ExecuteImpl(session());
 
   ReadTransaction trans(FROM_HERE, directory());
   // Lookup the parent folder by finding a child of the root.  We can't use
@@ -391,20 +398,26 @@ INSTANTIATE_TEST_CASE_P(ProcessCommitResponse,
 // happens to the extensions activity records.  Commits could fail or succeed,
 // depending on the test parameter.
 TEST_P(MixedResult, ExtensionActivity) {
+  sessions::OrderedCommitSet commit_set(session()->routing_info());
+  browser_sync::ClientToServerMessage request;
+  browser_sync::ClientToServerResponse response;
+
   EXPECT_NE(routing_info().find(syncable::BOOKMARKS)->second,
             routing_info().find(syncable::AUTOFILL)->second)
       << "To not be lame, this test requires more than one active group.";
 
   // Bookmark item setup.
   CreateUnprocessedCommitResult(id_factory_.NewServerId(),
-      id_factory_.root(), "Some bookmark", syncable::BOOKMARKS);
+      id_factory_.root(), "Some bookmark", syncable::BOOKMARKS,
+      &commit_set, &request, &response);
   if (ShouldFailBookmarkCommit())
-    SetLastErrorCode(CommitResponse::TRANSIENT_ERROR);
+    SetLastErrorCode(CommitResponse::TRANSIENT_ERROR, &response);
   // Autofill item setup.
   CreateUnprocessedCommitResult(id_factory_.NewServerId(),
-      id_factory_.root(), "Some autofill", syncable::AUTOFILL);
+      id_factory_.root(), "Some autofill", syncable::AUTOFILL,
+      &commit_set, &request, &response);
   if (ShouldFailAutofillCommit())
-    SetLastErrorCode(CommitResponse::TRANSIENT_ERROR);
+    SetLastErrorCode(CommitResponse::TRANSIENT_ERROR, &response);
 
   // Put some extensions activity in the session.
   {
@@ -415,8 +428,9 @@ TEST_P(MixedResult, ExtensionActivity) {
     (*records)["xyz"].extension_id = "xyz";
     (*records)["xyz"].bookmark_write_count = 4U;
   }
-  ExpectGroupsToChange(command_, GROUP_UI, GROUP_DB);
-  command_.ExecuteImpl(session());
+  ProcessCommitResponseCommand command(commit_set, request, response);
+  command.ExecuteImpl(session());
+  ExpectGroupsToChange(command, GROUP_UI, GROUP_DB);
 
   ExtensionsActivityMonitor::Records final_monitor_records;
   context()->extensions_monitor()->GetAndClearRecords(&final_monitor_records);