Revert r39261 on 307.

1. process_commit_response_command_unittest.cc fails to compile.
  SPECIFICS and SERVER_SPECIFICS are not defined. One example build slave:
http://chrome-master.mtv:8010/builders/chrome%20official%20linux/builds/620/steps/compile/logs/stdio

2. I tried to comment out those lines (and fix chrome_tests.gypi), but the
test fails.

3. I don't want to bring in the hairier r37253 (http://codereview.chromium.org/554066), 
which would probably fix this.

4. Therefore, I'm reverting and leaving it up to Nick to either give up or 
merge and fix the issues on 307.


-- Original CL (r39261) ---------
Merge 38472 - Fix a bug where positions inside of a newlycommitted folder would be
reversed if the children of the folder weren't committed in the same
batch as the folder.

Refactor ApplyUpdatesCommandTest, pulling out functionality that would
be useful for any SyncerCommand test, into syncer_command_test.h.

Add a test case for ProcessCommitResponseCommand using the new
SyncerCommandTest framework.  Add a test for the bug.

BUG=33081
TEST=sync_unit_tests. Also, manual testing, using the reduced repro
instructions described in comment #26 of the bug.


Review URL: http://codereview.chromium.org/652022

git-svn-id: svn://svn.chromium.org/chrome/branches/307/src@39568 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 63 insertions, 377 deletions

diff --git a/chrome/browser/sync/engine/apply_updates_command_unittest.cc b/chrome/browser/sync/engine/apply_updates_command_unittest.cc
index c793351..ed6811f 100755
--- a/chrome/browser/sync/engine/apply_updates_command_unittest.cc
+++ b/chrome/browser/sync/engine/apply_updates_command_unittest.cc
@@ -7,32 +7,66 @@
 #include "chrome/browser/sync/syncable/directory_manager.h"
 #include "chrome/browser/sync/syncable/syncable.h"
 #include "chrome/browser/sync/syncable/syncable_id.h"
-#include "chrome/test/sync/engine/syncer_command_test.h"
+#include "chrome/test/sync/engine/test_directory_setter_upper.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-namespace browser_sync {
-
-using sessions::SyncSession;
 using std::string;
+using syncable::ScopedDirLookup;
+using syncable::WriteTransaction;
+using syncable::ReadTransaction;
+using syncable::MutableEntry;
 using syncable::Entry;
 using syncable::Id;
-using syncable::MutableEntry;
-using syncable::ReadTransaction;
-using syncable::ScopedDirLookup;
 using syncable::UNITTEST;
-using syncable::WriteTransaction;
+
+namespace browser_sync {
+using sessions::SyncSessionContext;
+using sessions::SyncSession;
 
 // A test fixture for tests exercising ApplyUpdatesCommand.
-class ApplyUpdatesCommandTest : public SyncerCommandTest {
+class ApplyUpdatesCommandTest : public testing::Test,
+                                public SyncSession::Delegate,
+                                public ModelSafeWorkerRegistrar {
  public:
+  // SyncSession::Delegate implementation.
+  virtual void OnSilencedUntil(const base::TimeTicks& silenced_until) {
+   FAIL() << "Should not get silenced.";
+  }
+  virtual bool IsSyncingCurrentlySilenced() {
+   ADD_FAILURE() << "No requests for silenced state should be made.";
+   return false;
+  }
+  virtual void OnReceivedLongPollIntervalUpdate(
+     const base::TimeDelta& new_interval) {
+   FAIL() << "Should not get poll interval update.";
+  }
+  virtual void OnReceivedShortPollIntervalUpdate(
+     const base::TimeDelta& new_interval) {
+   FAIL() << "Should not get poll interval update.";
+  }
+
+  // ModelSafeWorkerRegistrar implementation.
+  virtual void GetWorkers(std::vector<ModelSafeWorker*>* out) {}
+  virtual void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) {}
+
  protected:
   ApplyUpdatesCommandTest() : next_revision_(1) {}
   virtual ~ApplyUpdatesCommandTest() {}
+  virtual void SetUp() {
+    syncdb_.SetUp();
+    context_.reset(new SyncSessionContext(NULL, syncdb_.manager(), this));
+    context_->set_account_name(syncdb_.name());
+  }
+  virtual void TearDown() {
+    syncdb_.TearDown();
+  }
+
+ protected:
 
   // Create a new unapplied update.
   void CreateUnappliedNewItemWithParent(const string& item_id,
                                         const string& parent_id) {
-    ScopedDirLookup dir(syncdb().manager(), syncdb().name());
+    ScopedDirLookup dir(syncdb_.manager(), syncdb_.name());
     ASSERT_TRUE(dir.good());
     WriteTransaction trans(dir, UNITTEST, __FILE__, __LINE__);
     MutableEntry entry(&trans, syncable::CREATE_NEW_UPDATE_ITEM,
@@ -46,8 +80,9 @@ class ApplyUpdatesCommandTest : public SyncerCommandTest {
     entry.Put(syncable::SERVER_IS_DIR, true);
   }
 
+  TestDirectorySetterUpper syncdb_;
   ApplyUpdatesCommand apply_updates_command_;
-
+  scoped_ptr<SyncSessionContext> context_;
  private:
   int64 next_revision_;
   DISALLOW_COPY_AND_ASSIGN(ApplyUpdatesCommandTest);
@@ -58,9 +93,10 @@ TEST_F(ApplyUpdatesCommandTest, Simple) {
   CreateUnappliedNewItemWithParent("parent", root_server_id);
   CreateUnappliedNewItemWithParent("child", "parent");
 
-  apply_updates_command_.ModelChangingExecuteImpl(session());
+  SyncSession session(context_.get(), this);
+  apply_updates_command_.ModelChangingExecuteImpl(&session);
 
-  sessions::StatusController* status = session()->status_controller();
+  sessions::StatusController* status = session.status_controller();
   EXPECT_EQ(2, status->update_progress().AppliedUpdatesSize())
       << "All updates should have been attempted";
   EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize())
@@ -79,9 +115,10 @@ TEST_F(ApplyUpdatesCommandTest, UpdateWithChildrenBeforeParents) {
   CreateUnappliedNewItemWithParent("a_child_created_second", "parent");
   CreateUnappliedNewItemWithParent("x_child_created_second", "parent");
 
-  apply_updates_command_.ModelChangingExecuteImpl(session());
+  SyncSession session(context_.get(), this);
+  apply_updates_command_.ModelChangingExecuteImpl(&session);
 
-  sessions::StatusController* status = session()->status_controller();
+  sessions::StatusController* status = session.status_controller();
   EXPECT_EQ(5, status->update_progress().AppliedUpdatesSize())
       << "All updates should have been attempted";
   EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize())
@@ -95,9 +132,10 @@ TEST_F(ApplyUpdatesCommandTest, NestedItemsWithUnknownParent) {
   CreateUnappliedNewItemWithParent("some_item", "unknown_parent");
   CreateUnappliedNewItemWithParent("some_other_item", "some_item");
 
-  apply_updates_command_.ModelChangingExecuteImpl(session());
+  SyncSession session(context_.get(), this);
+  apply_updates_command_.ModelChangingExecuteImpl(&session);
 
-  sessions::StatusController* status = session()->status_controller();
+  sessions::StatusController* status = session.status_controller();
   EXPECT_EQ(2, status->update_progress().AppliedUpdatesSize())
       << "All updates should have been attempted";
   EXPECT_EQ(2, status->conflict_progress()->ConflictingItemsSize())
@@ -116,9 +154,10 @@ TEST_F(ApplyUpdatesCommandTest, ItemsBothKnownAndUnknown) {
   CreateUnappliedNewItemWithParent("third_known_item", "fourth_known_item");
   CreateUnappliedNewItemWithParent("fourth_known_item", root_server_id);
 
-  apply_updates_command_.ModelChangingExecuteImpl(session());
+  SyncSession session(context_.get(), this);
+  apply_updates_command_.ModelChangingExecuteImpl(&session);
 
-  sessions::StatusController* status = session()->status_controller();
+  sessions::StatusController* status = session.status_controller();
   EXPECT_EQ(6, status->update_progress().AppliedUpdatesSize())
       << "All updates should have been attempted";
   EXPECT_EQ(2, status->conflict_progress()->ConflictingItemsSize())
diff --git a/chrome/browser/sync/engine/process_commit_response_command_unittest.cc b/chrome/browser/sync/engine/process_commit_response_command_unittest.cc
deleted file mode 100755
index c8b5d14..0000000
--- a/chrome/browser/sync/engine/process_commit_response_command_unittest.cc
+++ /dev/null
@@ -1,252 +0,0 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include <vector>
-
-#include "base/string_util.h"
-#include "chrome/browser/sync/engine/process_commit_response_command.h"
-#include "chrome/browser/sync/protocol/bookmark_specifics.pb.h"
-#include "chrome/browser/sync/protocol/sync.pb.h"
-#include "chrome/browser/sync/sessions/sync_session.h"
-#include "chrome/browser/sync/syncable/directory_manager.h"
-#include "chrome/browser/sync/syncable/syncable.h"
-#include "chrome/browser/sync/syncable/syncable_id.h"
-#include "chrome/test/sync/engine/syncer_command_test.h"
-#include "chrome/test/sync/engine/test_id_factory.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace browser_sync {
-
-using sessions::SyncSession;
-using std::string;
-using syncable::BASE_VERSION;
-using syncable::Entry;
-using syncable::IS_DIR;
-using syncable::IS_UNSYNCED;
-using syncable::Id;
-using syncable::MutableEntry;
-using syncable::NON_UNIQUE_NAME;
-using syncable::ReadTransaction;
-using syncable::ScopedDirLookup;
-using syncable::UNITTEST;
-using syncable::WriteTransaction;
-
-// A test fixture for tests exercising ProcessCommitResponseCommand.
-class ProcessCommitResponseCommandTest : public SyncerCommandTest {
- public:
- protected:
-  ProcessCommitResponseCommandTest()
-      : next_old_revision_(1),
-        next_new_revision_(4000),
-        next_server_position_(10000) {
-  }
-
-  // Create an unsynced item in the database.  If item_id is a local ID, it
-  // will be treated as a create-new.  Otherwise, if it's a server ID, we'll
-  // fake the server data so that it looks like it exists on the server.
-  void CreateUnsyncedItem(const Id& item_id,
-                          const Id& parent_id,
-                          const string& name,
-                          bool is_folder) {
-    ScopedDirLookup dir(syncdb().manager(), syncdb().name());
-    ASSERT_TRUE(dir.good());
-    WriteTransaction trans(dir, UNITTEST, __FILE__, __LINE__);
-    Id predecessor_id = dir->GetLastChildId(&trans, parent_id);
-    MutableEntry entry(&trans, syncable::CREATE, parent_id, name);
-    ASSERT_TRUE(entry.good());
-    entry.Put(syncable::ID, item_id);
-    entry.Put(syncable::BASE_VERSION,
-        item_id.ServerKnows() ? next_old_revision_++ : 0);
-    entry.Put(syncable::IS_UNSYNCED, true);
-    entry.Put(syncable::IS_DIR, is_folder);
-    entry.Put(syncable::IS_DEL, false);
-    entry.Put(syncable::PARENT_ID, parent_id);
-    entry.PutPredecessor(predecessor_id);
-    sync_pb::EntitySpecifics default_bookmark_specifics;
-    default_bookmark_specifics.MutableExtension(sync_pb::bookmark);
-    entry.Put(syncable::SPECIFICS, default_bookmark_specifics);
-    if (item_id.ServerKnows()) {
-      entry.Put(syncable::SERVER_SPECIFICS, default_bookmark_specifics);
-      entry.Put(syncable::SERVER_IS_DIR, is_folder);
-      entry.Put(syncable::SERVER_PARENT_ID, parent_id);
-      entry.Put(syncable::SERVER_IS_DEL, false);
-    }
-  }
-
-  // Create a new unsynced item in the database, and synthesize a commit
-  // 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) {
-    sessions::StatusController* sync_state = session()->status_controller();
-    bool is_folder = true;
-    CreateUnsyncedItem(item_id, parent_id, name, is_folder);
-
-    // ProcessCommitResponseCommand consumes commit_ids from the session
-    // state, so we need to update that.  O(n^2) because it's a test.
-    std::vector<Id> commit_ids = sync_state->commit_ids();
-    commit_ids.push_back(item_id);
-    sync_state->set_commit_ids(commit_ids);
-
-    ScopedDirLookup dir(syncdb().manager(), syncdb().name());
-    ASSERT_TRUE(dir.good());
-    WriteTransaction trans(dir, UNITTEST, __FILE__, __LINE__);
-    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();
-    commit->set_message_contents(ClientToServerMessage::COMMIT);
-    SyncEntity* entity = static_cast<SyncEntity*>(
-        commit->mutable_commit()->add_entries());
-    entity->set_non_unique_name(name);
-    entity->set_folder(is_folder);
-    entity->set_parent_id(parent_id);
-    entity->set_version(entry.Get(syncable::BASE_VERSION));
-    entity->mutable_specifics()->CopyFrom(entry.Get(syncable::SPECIFICS));
-    entity->set_id(item_id);
-
-    sync_pb::ClientToServerResponse* response =
-        sync_state->mutable_commit_response();
-    response->set_error_code(sync_pb::ClientToServerResponse::SUCCESS);
-    sync_pb::CommitResponse_EntryResponse* entry_response =
-        response->mutable_commit()->add_entryresponse();
-    entry_response->set_response_type(CommitResponse::SUCCESS);
-    entry_response->set_name("Garbage.");
-    entry_response->set_non_unique_name(entity->name());
-    if (item_id.ServerKnows())
-      entry_response->set_id_string(entity->id_string());
-    else
-      entry_response->set_id_string(id_factory_.NewServerId().GetServerId());
-    entry_response->set_version(next_new_revision_++);
-    entry_response->set_position_in_parent(next_server_position_++);
-
-    // If the ID of our parent item committed earlier in the batch was
-    // rewritten, rewrite it in the entry response.  This matches
-    // the server behavior.
-    entry_response->set_parent_id_string(entity->parent_id_string());
-    for (int i = 0; i < commit->commit().entries_size(); ++i) {
-      if (commit->commit().entries(i).id_string() ==
-          entity->parent_id_string()) {
-        entry_response->set_parent_id_string(
-            response->commit().entryresponse(i).id_string());
-      }
-    }
-  }
-
-  ProcessCommitResponseCommand command_;
-  TestIdFactory id_factory_;
-
- private:
-  int64 next_old_revision_;
-  int64 next_new_revision_;
-  int64 next_server_position_;
-  DISALLOW_COPY_AND_ASSIGN(ProcessCommitResponseCommandTest);
-};
-
-// In this test, we test processing a commit response for a commit batch that
-// includes a newly created folder and some (but not all) of its children.
-// In particular, the folder has 50 children, which alternate between being
-// new items and preexisting items.  This mixture of new and old is meant to
-// be a torture test of the code in ProcessCommitResponseCommand that changes
-// an item's ID from a local ID to a server-generated ID on the first commit.
-// We commit only the first 25 children in the sibling order, leaving the
-// second 25 children as unsynced items.  http://crbug.com/33081 describes
-// how this scenario used to fail, reversing the order for the second half
-// of the children.
-TEST_F(ProcessCommitResponseCommandTest, NewFolderCommitKeepsChildOrder) {
-  // 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");
-
-  // Verify that the item is reachable.
-  {
-    ScopedDirLookup dir(syncdb().manager(), syncdb().name());
-    ASSERT_TRUE(dir.good());
-    ReadTransaction trans(dir, __FILE__, __LINE__);
-    ASSERT_EQ(folder_id, dir->GetFirstChildId(&trans, id_factory_.root()));
-  }
-
-  // The first 25 children of the parent folder will be part of the commit
-  // batch.
-  int batch_size = 25;
-  int i = 0;
-  for (; i < batch_size; ++i) {
-    // 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, StringPrintf("Item %d", i));
-  }
-  // 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,
-  // these items PARENT_ID should be updated, and their ordering should be
-  // preserved.
-  for (; i < 2*batch_size; ++i) {
-    // Alternate between new and old child items, just for kicks.
-    Id id = (i % 4 < 2) ? id_factory_.NewLocalId() : id_factory_.NewServerId();
-    CreateUnsyncedItem(id, folder_id, StringPrintf("Item %d", i), false);
-  }
-
-  // Process the commit response for the parent folder and the first
-  // 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.
-  command_.ModelChangingExecuteImpl(session());
-
-  ScopedDirLookup dir(syncdb().manager(), syncdb().name());
-  ASSERT_TRUE(dir.good());
-  ReadTransaction trans(dir, __FILE__, __LINE__);
-  // Lookup the parent folder by finding a child of the root.  We can't use
-  // folder_id here, because it changed during the commit.
-  Id new_fid = dir->GetFirstChildId(&trans, id_factory_.root());
-  ASSERT_FALSE(new_fid.IsRoot());
-  EXPECT_TRUE(new_fid.ServerKnows());
-  EXPECT_FALSE(folder_id.ServerKnows());
-  EXPECT_TRUE(new_fid != folder_id);
-  Entry parent(&trans, syncable::GET_BY_ID, new_fid);
-  ASSERT_TRUE(parent.good());
-  ASSERT_EQ("A", parent.Get(NON_UNIQUE_NAME))
-      << "Name of parent folder should not change.";
-  ASSERT_LT(0, parent.Get(BASE_VERSION))
-      << "Parent should have a valid (positive) server base revision";
-
-  Id cid = dir->GetFirstChildId(&trans, new_fid);
-  int child_count = 0;
-  // Now loop over all the children of the parent folder, verifying
-  // that they are in their original order by checking to see that their
-  // names are still sequential.
-  while (!cid.IsRoot()) {
-    SCOPED_TRACE(::testing::Message("Examining item #") << child_count);
-    Entry c(&trans, syncable::GET_BY_ID, cid);
-    DCHECK(c.good());
-    ASSERT_EQ(StringPrintf("Item %d", child_count), c.Get(NON_UNIQUE_NAME));
-    ASSERT_EQ(new_fid, c.Get(syncable::PARENT_ID));
-    if (child_count < batch_size) {
-      ASSERT_FALSE(c.Get(IS_UNSYNCED)) << "Item should be committed";
-      ASSERT_TRUE(cid.ServerKnows());
-      ASSERT_LT(0, c.Get(BASE_VERSION));
-    } else {
-      ASSERT_TRUE(c.Get(IS_UNSYNCED)) << "Item should be uncommitted";
-      // We alternated between creates and edits; double check that these items
-      // have been preserved.
-      if (child_count % 4 < 2) {
-        ASSERT_FALSE(cid.ServerKnows());
-        ASSERT_GE(0, c.Get(BASE_VERSION));
-      } else {
-        ASSERT_TRUE(cid.ServerKnows());
-        ASSERT_LT(0, c.Get(BASE_VERSION));
-      }
-    }
-    cid = c.Get(syncable::NEXT_ID);
-    child_count++;
-  }
-  ASSERT_EQ(batch_size*2, child_count)
-      << "Too few or too many children in parent folder after commit.";
-}
-
-}  // namespace browser_sync
diff --git a/chrome/browser/sync/engine/process_updates_command.h b/chrome/browser/sync/engine/process_updates_command.h
index 6eb4333..27f3814 100644
--- a/chrome/browser/sync/engine/process_updates_command.h
+++ b/chrome/browser/sync/engine/process_updates_command.h
@@ -32,11 +32,10 @@ class ProcessUpdatesCommand : public ModelChangingSyncerCommand {
 
   // ModelChangingSyncerCommand implementation.
   virtual void ModelChangingExecuteImpl(sessions::SyncSession* session);
-
- private:
   ServerUpdateProcessingResult ProcessUpdate(
       const syncable::ScopedDirLookup& dir,
       const sync_pb::SyncEntity& pb_entry);
+ private:
   DISALLOW_COPY_AND_ASSIGN(ProcessUpdatesCommand);
 };
 
diff --git a/chrome/browser/sync/engine/syncer_util.cc b/chrome/browser/sync/engine/syncer_util.cc
index 1923991..da451a5 100755
--- a/chrome/browser/sync/engine/syncer_util.cc
+++ b/chrome/browser/sync/engine/syncer_util.cc
@@ -99,12 +99,7 @@ void SyncerUtil::ChangeEntryIDAndUpdateChildren(
     while (i != children->end()) {
       MutableEntry child_entry(trans, GET_BY_HANDLE, *i++);
       CHECK(child_entry.good());
-      // Use the unchecked setter here to avoid touching the child's NEXT_ID
-      // and PREV_ID fields (which Put(PARENT_ID) would normally do to
-      // maintain linked-list invariants).  In this case, NEXT_ID and PREV_ID
-      // among the children will be valid after the loop, since we update all
-      // the children at once.
-      child_entry.PutParentIdPropertyOnly(new_id);
+      CHECK(child_entry.Put(PARENT_ID, new_id));
     }
   }
   // Update Id references on the previous and next nodes in the sibling
diff --git a/chrome/browser/sync/syncable/syncable.cc b/chrome/browser/sync/syncable/syncable.cc
index da7fec2..b18d10b 100755
--- a/chrome/browser/sync/syncable/syncable.cc
+++ b/chrome/browser/sync/syncable/syncable.cc
@@ -1093,8 +1093,8 @@ bool MutableEntry::Put(IdField field, const Id& value) {
       if (!dir()->ReindexId(kernel_, value))
         return false;
     } else if (PARENT_ID == field) {
-      PutParentIdPropertyOnly(value);  // Makes sibling order inconsistent.
-      PutPredecessor(Id());  // Fixes up the sibling order inconsistency.
+      dir()->ReindexParentId(kernel_, value);
+      PutPredecessor(Id());
     } else {
       kernel_->put(field, value);
     }
@@ -1219,7 +1219,6 @@ bool MutableEntry::PutPredecessor(const Id& predecessor_id) {
   return true;
 }
 
-
 ///////////////////////////////////////////////////////////////////////////
 // High-level functions
 
diff --git a/chrome/browser/sync/syncable/syncable.h b/chrome/browser/sync/syncable/syncable.h
index 286f6c8..05b3052 100755
--- a/chrome/browser/sync/syncable/syncable.h
+++ b/chrome/browser/sync/syncable/syncable.h
@@ -441,18 +441,6 @@ class MutableEntry : public Entry {
   // TODO(chron): Remove some of these unecessary return values.
   bool Put(Int64Field field, const int64& value);
   bool Put(IdField field, const Id& value);
-
-  // Do a simple property-only update if the PARENT_ID field.  Use with caution.
-  //
-  // The normal Put(IS_PARENT) call will move the item to the front of the
-  // sibling order to maintain the linked list invariants when the parent
-  // changes.  That's usually what you want to do, but it's inappropriate
-  // when the caller is trying to change the parent ID of a the whole set
-  // of children (e.g. because the ID changed during a commit).  For those
-  // cases, there's this function.  It will corrupt the sibling ordering
-  // if you're not careful.
-  void PutParentIdPropertyOnly(const Id& parent_id);
-
   bool Put(StringField field, const std::string& value);
   bool Put(BaseVersion field, int64 value);
 
@@ -478,6 +466,7 @@ class MutableEntry : public Entry {
   }
 
  protected:
+
   template <typename FieldType, typename ValueType>
   inline bool PutField(FieldType field, const ValueType& value) {
     DCHECK(kernel_);
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 651f8c2..779f4dc 100755
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1430,7 +1430,6 @@
         'browser/sync/engine/apply_updates_command_unittest.cc',
         'browser/sync/engine/auth_watcher_unittest.cc',
         'browser/sync/engine/net/gaia_authenticator_unittest.cc',
-        'browser/sync/engine/process_commit_response_command_unittest.cc',
         'browser/sync/engine/syncer_proto_util_unittest.cc',
         'browser/sync/engine/syncer_thread_unittest.cc',
         'browser/sync/engine/syncer_unittest.cc',
@@ -1439,7 +1438,6 @@
         'browser/sync/notifier/listener/talk_mediator_unittest.cc',
         'browser/sync/sessions/status_controller_unittest.cc',
         'browser/sync/sessions/sync_session_unittest.cc',
-        'browser/sync/syncable/directory_backing_store_unittest.cc',
         'browser/sync/syncable/syncable_id_unittest.cc',
         'browser/sync/syncable/syncable_unittest.cc',
         'browser/sync/util/crypto_helpers_unittest.cc',
@@ -1452,7 +1450,6 @@
         'test/sync/engine/mock_gaia_authenticator_unittest.cc',
         'test/sync/engine/mock_server_connection.cc',
         'test/sync/engine/mock_server_connection.h',
-        'test/sync/engine/syncer_command_test.h',
         'test/sync/engine/test_directory_setter_upper.cc',
         'test/sync/engine/test_directory_setter_upper.h',
         'test/sync/engine/test_id_factory.h',
diff --git a/chrome/test/sync/engine/syncer_command_test.h b/chrome/test/sync/engine/syncer_command_test.h
deleted file mode 100755
index e5ef5dc..0000000
--- a/chrome/test/sync/engine/syncer_command_test.h
+++ /dev/null
@@ -1,80 +0,0 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CHROME_TEST_SYNC_ENGINE_SYNCER_COMMAND_TEST_H_
-#define CHROME_TEST_SYNC_ENGINE_SYNCER_COMMAND_TEST_H_
-
-#include "chrome/test/sync/engine/test_directory_setter_upper.h"
-#include "chrome/browser/sync/engine/model_safe_worker.h"
-#include "chrome/browser/sync/sessions/sync_session.h"
-#include "chrome/browser/sync/sessions/sync_session_context.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace browser_sync {
-
-// A test fixture that simplifies writing unit tests for individual
-// SyncerCommands, providing convenient access to a test directory
-// and a syncer session.
-class SyncerCommandTest : public testing::Test,
-                          public sessions::SyncSession::Delegate,
-                          public ModelSafeWorkerRegistrar {
- public:
-  // SyncSession::Delegate implementation.
-  virtual void OnSilencedUntil(const base::TimeTicks& silenced_until) {
-    FAIL() << "Should not get silenced.";
-  }
-  virtual bool IsSyncingCurrentlySilenced() {
-    ADD_FAILURE() << "No requests for silenced state should be made.";
-    return false;
-  }
-  virtual void OnReceivedLongPollIntervalUpdate(
-      const base::TimeDelta& new_interval) {
-    FAIL() << "Should not get poll interval update.";
-  }
-  virtual void OnReceivedShortPollIntervalUpdate(
-      const base::TimeDelta& new_interval) {
-    FAIL() << "Should not get poll interval update.";
-  }
-
-  // ModelSafeWorkerRegistrar implementation.
-  virtual void GetWorkers(std::vector<ModelSafeWorker*>* out) {}
-  virtual void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) {}
-
- protected:
-  SyncerCommandTest() {}
-  virtual ~SyncerCommandTest() {}
-  virtual void SetUp() {
-    syncdb_.SetUp();
-    context_.reset(new sessions::SyncSessionContext(NULL, syncdb_.manager(),
-                                                    registrar()));
-    context_->set_account_name(syncdb_.name());
-  }
-  virtual void TearDown() {
-    syncdb_.TearDown();
-  }
-
-  const TestDirectorySetterUpper& syncdb() const { return syncdb_; }
-  sessions::SyncSessionContext* context() const { return context_.get(); }
-  sessions::SyncSession::Delegate* delegate() { return this; }
-  ModelSafeWorkerRegistrar* registrar() { return this; }
-  // Lazily create a session.
-  sessions::SyncSession* session() {
-    if (!session_.get())
-      session_.reset(new sessions::SyncSession(context(), delegate()));
-    return session_.get();
-  }
-  void ClearSession() {
-    session_.reset();
-  }
-
- private:
-  TestDirectorySetterUpper syncdb_;
-  scoped_ptr<sessions::SyncSessionContext> context_;
-  scoped_ptr<sessions::SyncSession> session_;
-  DISALLOW_COPY_AND_ASSIGN(SyncerCommandTest);
-};
-
-}  // namespace browser_sync
-
-#endif  // CHROME_TEST_SYNC_ENGINE_SYNCER_COMMAND_TEST_H_