sync: Merge {Verify,Process}UpdatesCommand

The VerifyUpdatesCommand was used to ensure the validity of each update
that had been delivered from the server and to prepare for the
ProcessUpdatesCommand.  The ProcessUpdatesCommand would then take all
updates that had been successfully verified and move them into the
SERVER_ fields of the sync directory.

It turns out that the logic can be greatly simplified by performing both
tasks within the same command.  This patch does not take full advantage
of the merge.  This patch is intended simply to merge the two files,
with the goal of performing more significant refactorings later.

BUG=154654


Review URL: https://chromiumcodereview.appspot.com/11091009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@162176 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 251 insertions, 436 deletions

diff --git a/sync/engine/process_updates_command.cc b/sync/engine/process_updates_command.cc
index 0bf03f1..22d465c 100644
--- a/sync/engine/process_updates_command.cc
+++ b/sync/engine/process_updates_command.cc
@@ -32,50 +32,214 @@ using sessions::SyncSession;
 using sessions::StatusController;
 using sessions::UpdateProgress;
 
+using syncable::GET_BY_ID;
+
 ProcessUpdatesCommand::ProcessUpdatesCommand() {}
 ProcessUpdatesCommand::~ProcessUpdatesCommand() {}
 
 std::set<ModelSafeGroup> ProcessUpdatesCommand::GetGroupsToChange(
     const sessions::SyncSession& session) const {
-  return session.GetEnabledGroupsWithVerifiedUpdates();
+  std::set<ModelSafeGroup> groups_with_updates;
+
+  const sync_pb::GetUpdatesResponse& updates =
+      session.status_controller().updates_response().get_updates();
+  for (int i = 0; i < updates.entries().size(); i++) {
+    groups_with_updates.insert(
+        GetGroupForModelType(GetModelType(updates.entries(i)),
+                             session.routing_info()));
+  }
+
+  return groups_with_updates;
+}
+
+namespace {
+
+// This function attempts to determine whether or not this update is genuinely
+// new, or if it is a reflection of one of our own commits.
+//
+// There is a known inaccuracy in its implementation.  If this update ends up
+// being applied to a local item with a different ID, we will count the change
+// as being a non-reflection update.  Fortunately, the server usually updates
+// our IDs correctly in its commit response, so a new ID during GetUpdate should
+// be rare.
+//
+// The only secnarios I can think of where this might happen are:
+// - We commit a  new item to the server, but we don't persist the
+// server-returned new ID to the database before we shut down.  On the GetUpdate
+// following the next restart, we will receive an update from the server that
+// updates its local ID.
+// - When two attempts to create an item with identical UNIQUE_CLIENT_TAG values
+// collide at the server.  I have seen this in testing.  When it happens, the
+// test server will send one of the clients a response to upate its local ID so
+// that both clients will refer to the item using the same ID going forward.  In
+// this case, we're right to assume that the update is not a reflection.
+//
+// For more information, see FindLocalIdToUpdate().
+bool UpdateContainsNewVersion(syncable::BaseTransaction *trans,
+                              const sync_pb::SyncEntity &update) {
+  int64 existing_version = -1; // The server always sends positive versions.
+  syncable::Entry existing_entry(trans, GET_BY_ID,
+                                 SyncableIdFromProto(update.id_string()));
+  if (existing_entry.good())
+    existing_version = existing_entry.Get(syncable::BASE_VERSION);
+
+  if (!existing_entry.good() && update.deleted()) {
+    // There are several possible explanations for this.  The most common cases
+    // will be first time sync and the redelivery of deletions we've already
+    // synced, accepted, and purged from our database.  In either case, the
+    // update is useless to us.  Let's count them all as "not new", even though
+    // that may not always be entirely accurate.
+    return false;
+  }
+
+  if (existing_entry.good() &&
+      !existing_entry.Get(syncable::UNIQUE_CLIENT_TAG).empty() &&
+      existing_entry.Get(syncable::IS_DEL) &&
+      update.deleted()) {
+    // Unique client tags will have their version set to zero when they're
+    // deleted.  The usual version comparison logic won't be able to detect
+    // reflections of these items.  Instead, we assume any received tombstones
+    // are reflections.  That should be correct most of the time.
+    return false;
+  }
+
+  return existing_version < update.version();
 }
 
+}  // namespace
+
 SyncerError ProcessUpdatesCommand::ModelChangingExecuteImpl(
     SyncSession* session) {
   syncable::Directory* dir = session->context()->directory();
 
-  const sessions::UpdateProgress* progress =
-      session->status_controller().update_progress();
-  if (!progress)
-    return SYNCER_OK;  // Nothing to do.
-
   syncable::WriteTransaction trans(FROM_HERE, syncable::SYNCER, dir);
-  vector<sessions::VerifiedUpdate>::const_iterator it;
-  for (it = progress->VerifiedUpdatesBegin();
-       it != progress->VerifiedUpdatesEnd();
-       ++it) {
-    const sync_pb::SyncEntity& update = it->second;
 
-    if (it->first != VERIFY_SUCCESS && it->first != VERIFY_UNDELETE)
+  sessions::StatusController* status = session->mutable_status_controller();
+  const sync_pb::GetUpdatesResponse& updates =
+      status->updates_response().get_updates();
+  int update_count = updates.entries().size();
+
+  ModelTypeSet requested_types = GetRoutingInfoTypes(
+      session->routing_info());
+
+  DVLOG(1) << update_count << " entries to verify";
+  for (int i = 0; i < update_count; i++) {
+    const sync_pb::SyncEntity& update = updates.entries(i);
+
+    // The current function gets executed on several different threads, but
+    // every call iterates over the same list of items that the server returned
+    // to us.  We're not allowed to process items unless we're on the right
+    // thread for that type.  This check will ensure we only touch the items
+    // that live on our current thread.
+    // TODO(tim): Don't allow access to objects in other ModelSafeGroups.
+    // See crbug.com/121521 .
+    ModelSafeGroup g = GetGroupForModelType(GetModelType(update),
+                                            session->routing_info());
+    if (g != status->group_restriction())
       continue;
-    switch (ProcessUpdate(update,
-                          dir->GetCryptographer(&trans),
-                          &trans)) {
-      case SUCCESS_PROCESSED:
-      case SUCCESS_STORED:
-        break;
-      default:
-        NOTREACHED();
-        break;
-    }
+
+    VerifyResult verify_result = VerifyUpdate(&trans, update,
+                                              requested_types,
+                                              session->routing_info());
+    status->mutable_update_progress()->AddVerifyResult(verify_result, update);
+    status->increment_num_updates_downloaded_by(1);
+    if (!UpdateContainsNewVersion(&trans, update))
+      status->increment_num_reflected_updates_downloaded_by(1);
+    if (update.deleted())
+      status->increment_num_tombstone_updates_downloaded_by(1);
+
+    if (verify_result != VERIFY_SUCCESS && verify_result != VERIFY_UNDELETE)
+      continue;
+
+    ServerUpdateProcessingResult process_result =
+       ProcessUpdate(update, dir->GetCryptographer(&trans), &trans);
+
+    DCHECK(process_result == SUCCESS_PROCESSED ||
+           process_result == SUCCESS_STORED);
   }
 
-  StatusController* status = session->mutable_status_controller();
   status->mutable_update_progress()->ClearVerifiedUpdates();
   return SYNCER_OK;
 }
 
 namespace {
+
+// In the event that IDs match, but tags differ AttemptReuniteClient tag
+// will have refused to unify the update.
+// We should not attempt to apply it at all since it violates consistency
+// rules.
+VerifyResult VerifyTagConsistency(const sync_pb::SyncEntity& entry,
+                                  const syncable::MutableEntry& same_id) {
+  if (entry.has_client_defined_unique_tag() &&
+      entry.client_defined_unique_tag() !=
+          same_id.Get(syncable::UNIQUE_CLIENT_TAG)) {
+    return VERIFY_FAIL;
+  }
+  return VERIFY_UNDECIDED;
+}
+
+}  // namespace
+
+VerifyResult ProcessUpdatesCommand::VerifyUpdate(
+    syncable::WriteTransaction* trans, const sync_pb::SyncEntity& entry,
+    ModelTypeSet requested_types,
+    const ModelSafeRoutingInfo& routes) {
+  syncable::Id id = SyncableIdFromProto(entry.id_string());
+  VerifyResult result = VERIFY_FAIL;
+
+  const bool deleted = entry.has_deleted() && entry.deleted();
+  const bool is_directory = IsFolder(entry);
+  const ModelType model_type = GetModelType(entry);
+
+  if (!id.ServerKnows()) {
+    LOG(ERROR) << "Illegal negative id in received updates";
+    return result;
+  }
+  {
+    const std::string name = SyncerProtoUtil::NameFromSyncEntity(entry);
+    if (name.empty() && !deleted) {
+      LOG(ERROR) << "Zero length name in non-deleted update";
+      return result;
+    }
+  }
+
+  syncable::MutableEntry same_id(trans, GET_BY_ID, id);
+  result = VerifyNewEntry(entry, &same_id, deleted);
+
+  ModelType placement_type = !deleted ? GetModelType(entry)
+      : same_id.good() ? same_id.GetModelType() : UNSPECIFIED;
+
+  if (VERIFY_UNDECIDED == result) {
+    result = VerifyTagConsistency(entry, same_id);
+  }
+
+  if (VERIFY_UNDECIDED == result) {
+    if (deleted) {
+      // For deletes the server could send tombostones for items that
+      // the client did not request. If so ignore those items.
+      if (IsRealDataType(placement_type) &&
+          !requested_types.Has(placement_type)) {
+        result = VERIFY_SKIP;
+      } else {
+        result = VERIFY_SUCCESS;
+      }
+    }
+  }
+
+  // If we have an existing entry, we check here for updates that break
+  // consistency rules.
+  if (VERIFY_UNDECIDED == result) {
+    result = VerifyUpdateConsistency(trans, entry, &same_id,
+                                     deleted, is_directory, model_type);
+  }
+
+  if (VERIFY_UNDECIDED == result)
+    result = VERIFY_SUCCESS;  // No news is good news.
+
+  return result;  // This might be VERIFY_SUCCESS as well
+}
+
+namespace {
 // Returns true if the entry is still ok to process.
 bool ReverifyEntry(syncable::WriteTransaction* trans,
                    const sync_pb::SyncEntity& entry,
@@ -115,10 +279,10 @@ ServerUpdateProcessingResult ProcessUpdatesCommand::ProcessUpdate(
 
   // We take a two step approach. First we store the entries data in the
   // server fields of a local entry and then move the data to the local fields
-  syncable::MutableEntry target_entry(trans, syncable::GET_BY_ID, local_id);
+  syncable::MutableEntry target_entry(trans, GET_BY_ID, local_id);
 
   // We need to run the Verify checks again; the world could have changed
-  // since VerifyUpdatesCommand.
+  // since we last verified.
   if (!ReverifyEntry(trans, update, &target_entry)) {
     return SUCCESS_PROCESSED;  // The entry has become irrelevant.
   }
diff --git a/sync/engine/process_updates_command.h b/sync/engine/process_updates_command.h
index 7604d11..9f89a63 100644
--- a/sync/engine/process_updates_command.h
+++ b/sync/engine/process_updates_command.h
@@ -21,14 +21,12 @@ class WriteTransaction;
 
 class Cryptographer;
 
-// A syncer command for processing updates.
+// A syncer command for verifying and processing updates.
 //
-// Preconditions - updates in the SyncerSesssion have been downloaded
-//                 and verified.
+// Preconditions - Updates in the SyncerSesssion have been downloaded.
 //
 // Postconditions - All of the verified SyncEntity data will be copied to
 //                  the server fields of the corresponding syncable entries.
-// TODO(tim): This should not be ModelChanging (bug 36592).
 class ProcessUpdatesCommand : public ModelChangingSyncerCommand {
  public:
   ProcessUpdatesCommand();
@@ -42,6 +40,11 @@ class ProcessUpdatesCommand : public ModelChangingSyncerCommand {
       sessions::SyncSession* session) OVERRIDE;
 
  private:
+  VerifyResult VerifyUpdate(
+      syncable::WriteTransaction* trans,
+      const sync_pb::SyncEntity& entry,
+      ModelTypeSet requested_types,
+      const ModelSafeRoutingInfo& routes);
   ServerUpdateProcessingResult ProcessUpdate(
       const sync_pb::SyncEntity& proto_update,
       const Cryptographer* cryptographer,
diff --git a/sync/engine/process_updates_command_unittest.cc b/sync/engine/process_updates_command_unittest.cc
index 59f2210..fc543fb 100644
--- a/sync/engine/process_updates_command_unittest.cc
+++ b/sync/engine/process_updates_command_unittest.cc
@@ -3,11 +3,11 @@
 // found in the LICENSE file.
 
 #include "base/basictypes.h"
-#include "base/memory/ref_counted.h"
 #include "sync/engine/process_updates_command.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/sessions/session_state.h"
 #include "sync/sessions/sync_session.h"
+#include "sync/syncable/mutable_entry.h"
 #include "sync/syncable/syncable_id.h"
 #include "sync/test/engine/fake_model_worker.h"
 #include "sync/test/engine/syncer_command_test.h"
@@ -15,6 +15,11 @@
 
 namespace syncer {
 
+using syncable::Id;
+using syncable::MutableEntry;
+using syncable::UNITTEST;
+using syncable::WriteTransaction;
+
 namespace {
 
 class ProcessUpdatesCommandTest : public SyncerCommandTest {
@@ -27,24 +32,67 @@ class ProcessUpdatesCommandTest : public SyncerCommandTest {
         make_scoped_refptr(new FakeModelWorker(GROUP_UI)));
     workers()->push_back(
         make_scoped_refptr(new FakeModelWorker(GROUP_DB)));
+    (*mutable_routing_info())[PREFERENCES] = GROUP_UI;
     (*mutable_routing_info())[BOOKMARKS] = GROUP_UI;
     (*mutable_routing_info())[AUTOFILL] = GROUP_DB;
     SyncerCommandTest::SetUp();
   }
 
+  void CreateLocalItem(const std::string& item_id,
+                       const std::string& parent_id,
+                       const ModelType& type) {
+    WriteTransaction trans(FROM_HERE, UNITTEST, directory());
+    MutableEntry entry(&trans, syncable::CREATE_NEW_UPDATE_ITEM,
+        Id::CreateFromServerId(item_id));
+    ASSERT_TRUE(entry.good());
+
+    entry.Put(syncable::BASE_VERSION, 1);
+    entry.Put(syncable::SERVER_VERSION, 1);
+    entry.Put(syncable::NON_UNIQUE_NAME, item_id);
+    entry.Put(syncable::PARENT_ID, Id::CreateFromServerId(parent_id));
+    sync_pb::EntitySpecifics default_specifics;
+    AddDefaultFieldValue(type, &default_specifics);
+    entry.Put(syncable::SERVER_SPECIFICS, default_specifics);
+  }
+
+  void AddUpdate(sync_pb::GetUpdatesResponse* updates,
+      const std::string& id, const std::string& parent,
+      const ModelType& type) {
+    sync_pb::SyncEntity* e = updates->add_entries();
+    e->set_id_string("b1");
+    e->set_parent_id_string(parent);
+    e->set_non_unique_name("b1");
+    e->set_name("b1");
+    AddDefaultFieldValue(type, e->mutable_specifics());
+  }
+
   ProcessUpdatesCommand command_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ProcessUpdatesCommandTest);
 };
 
-TEST_F(ProcessUpdatesCommandTest, GetGroupsToChange) {
+TEST_F(ProcessUpdatesCommandTest, GroupsToChange) {
+  std::string root = syncable::GetNullId().GetServerId();
+
+  CreateLocalItem("b1", root, BOOKMARKS);
+  CreateLocalItem("b2", root, BOOKMARKS);
+  CreateLocalItem("p1", root, PREFERENCES);
+  CreateLocalItem("a1", root, AUTOFILL);
+
   ExpectNoGroupsToChange(command_);
-  // Add a verified update for GROUP_DB.
-  session()->mutable_status_controller()->
-      GetUnrestrictedMutableUpdateProgressForTest(GROUP_DB)->
-      AddVerifyResult(VerifyResult(), sync_pb::SyncEntity());
-  ExpectGroupToChange(command_, GROUP_DB);
+
+  sync_pb::GetUpdatesResponse* updates =
+      session()->mutable_status_controller()->
+      mutable_updates_response()->mutable_get_updates();
+  AddUpdate(updates, "b1", root, BOOKMARKS);
+  AddUpdate(updates, "b2", root, BOOKMARKS);
+  AddUpdate(updates, "p1", root, PREFERENCES);
+  AddUpdate(updates, "a1", root, AUTOFILL);
+
+  ExpectGroupsToChange(command_, GROUP_UI, GROUP_DB);
+
+  command_.ExecuteImpl(session());
 }
 
 }  // namespace
diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc
index 7b09c3f6..ffb2fca 100644
--- a/sync/engine/syncer.cc
+++ b/sync/engine/syncer.cc
@@ -23,7 +23,6 @@
 #include "sync/engine/store_timestamps_command.h"
 #include "sync/engine/syncer_types.h"
 #include "sync/engine/throttled_data_type_tracker.h"
-#include "sync/engine/verify_updates_command.h"
 #include "sync/syncable/mutable_entry.h"
 #include "sync/syncable/syncable-inl.h"
 
@@ -58,7 +57,6 @@ const char* SyncerStepToString(const SyncerStep step)
   switch (step) {
     ENUM_CASE(SYNCER_BEGIN);
     ENUM_CASE(DOWNLOAD_UPDATES);
-    ENUM_CASE(VERIFY_UPDATES);
     ENUM_CASE(PROCESS_UPDATES);
     ENUM_CASE(STORE_TIMESTAMPS);
     ENUM_CASE(APPLY_UPDATES);
@@ -121,12 +119,6 @@ void Syncer::SyncShare(sessions::SyncSession* session,
         DownloadUpdatesCommand download_updates(kCreateMobileBookmarksFolder);
         session->mutable_status_controller()->set_last_download_updates_result(
             download_updates.Execute(session));
-        next_step = VERIFY_UPDATES;
-        break;
-      }
-      case VERIFY_UPDATES: {
-        VerifyUpdatesCommand verify_updates;
-        verify_updates.Execute(session);
         next_step = PROCESS_UPDATES;
         break;
       }
diff --git a/sync/engine/syncer.h b/sync/engine/syncer.h
index 8fadde9..3c4c68e 100644
--- a/sync/engine/syncer.h
+++ b/sync/engine/syncer.h
@@ -27,7 +27,6 @@ class MutableEntry;
 enum SyncerStep {
   SYNCER_BEGIN,
   DOWNLOAD_UPDATES,
-  VERIFY_UPDATES,
   PROCESS_UPDATES,
   STORE_TIMESTAMPS,
   APPLY_UPDATES,
diff --git a/sync/engine/verify_updates_command.cc b/sync/engine/verify_updates_command.cc
deleted file mode 100644
index b8bc043..0000000
--- a/sync/engine/verify_updates_command.cc
+++ /dev/null
@@ -1,210 +0,0 @@
-// Copyright (c) 2012 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 "sync/engine/verify_updates_command.h"
-
-#include <string>
-
-#include "base/location.h"
-#include "sync/engine/syncer.h"
-#include "sync/engine/syncer_proto_util.h"
-#include "sync/engine/syncer_types.h"
-#include "sync/engine/syncer_util.h"
-#include "sync/internal_api/public/engine/model_safe_worker.h"
-#include "sync/protocol/bookmark_specifics.pb.h"
-#include "sync/protocol/sync.pb.h"
-#include "sync/syncable/entry.h"
-#include "sync/syncable/mutable_entry.h"
-#include "sync/syncable/syncable_proto_util.h"
-#include "sync/syncable/write_transaction.h"
-
-namespace syncer {
-
-using syncable::GET_BY_ID;
-using syncable::SYNCER;
-using syncable::WriteTransaction;
-
-namespace {
-
-// This function attempts to determine whether or not this update is genuinely
-// new, or if it is a reflection of one of our own commits.
-//
-// There is a known inaccuracy in its implementation.  If this update ends up
-// being applied to a local item with a different ID, we will count the change
-// as being a non-reflection update.  Fortunately, the server usually updates
-// our IDs correctly in its commit response, so a new ID during GetUpdate should
-// be rare.
-//
-// The only secnarios I can think of where this might happen are:
-// - We commit a  new item to the server, but we don't persist the
-// server-returned new ID to the database before we shut down.  On the GetUpdate
-// following the next restart, we will receive an update from the server that
-// updates its local ID.
-// - When two attempts to create an item with identical UNIQUE_CLIENT_TAG values
-// collide at the server.  I have seen this in testing.  When it happens, the
-// test server will send one of the clients a response to upate its local ID so
-// that both clients will refer to the item using the same ID going forward.  In
-// this case, we're right to assume that the update is not a reflection.
-//
-// For more information, see FindLocalIdToUpdate().
-bool UpdateContainsNewVersion(syncable::BaseTransaction *trans,
-                              const sync_pb::SyncEntity &update) {
-  int64 existing_version = -1; // The server always sends positive versions.
-  syncable::Entry existing_entry(trans, GET_BY_ID,
-                                 SyncableIdFromProto(update.id_string()));
-  if (existing_entry.good())
-    existing_version = existing_entry.Get(syncable::BASE_VERSION);
-
-  if (!existing_entry.good() && update.deleted()) {
-    // There are several possible explanations for this.  The most common cases
-    // will be first time sync and the redelivery of deletions we've already
-    // synced, accepted, and purged from our database.  In either case, the
-    // update is useless to us.  Let's count them all as "not new", even though
-    // that may not always be entirely accurate.
-    return false;
-  }
-
-  if (existing_entry.good() &&
-      !existing_entry.Get(syncable::UNIQUE_CLIENT_TAG).empty() &&
-      existing_entry.Get(syncable::IS_DEL) &&
-      update.deleted()) {
-    // Unique client tags will have their version set to zero when they're
-    // deleted.  The usual version comparison logic won't be able to detect
-    // reflections of these items.  Instead, we assume any received tombstones
-    // are reflections.  That should be correct most of the time.
-    return false;
-  }
-
-  return existing_version < update.version();
-}
-
-// In the event that IDs match, but tags differ AttemptReuniteClient tag
-// will have refused to unify the update.
-// We should not attempt to apply it at all since it violates consistency
-// rules.
-VerifyResult VerifyTagConsistency(const sync_pb::SyncEntity& entry,
-                                  const syncable::MutableEntry& same_id) {
-  if (entry.has_client_defined_unique_tag() &&
-      entry.client_defined_unique_tag() !=
-          same_id.Get(syncable::UNIQUE_CLIENT_TAG)) {
-    return VERIFY_FAIL;
-  }
-  return VERIFY_UNDECIDED;
-}
-}  // namespace
-
-VerifyUpdatesCommand::VerifyUpdatesCommand() {}
-VerifyUpdatesCommand::~VerifyUpdatesCommand() {}
-
-std::set<ModelSafeGroup> VerifyUpdatesCommand::GetGroupsToChange(
-    const sessions::SyncSession& session) const {
-  std::set<ModelSafeGroup> groups_with_updates;
-
-  const sync_pb::GetUpdatesResponse& updates =
-      session.status_controller().updates_response().get_updates();
-  for (int i = 0; i < updates.entries().size(); i++) {
-    groups_with_updates.insert(
-        GetGroupForModelType(GetModelType(updates.entries(i)),
-                             session.routing_info()));
-  }
-
-  return groups_with_updates;
-}
-
-SyncerError VerifyUpdatesCommand::ModelChangingExecuteImpl(
-    sessions::SyncSession* session) {
-  DVLOG(1) << "Beginning Update Verification";
-  syncable::Directory* dir = session->context()->directory();
-  WriteTransaction trans(FROM_HERE, SYNCER, dir);
-  sessions::StatusController* status = session->mutable_status_controller();
-  const sync_pb::GetUpdatesResponse& updates =
-      status->updates_response().get_updates();
-  int update_count = updates.entries().size();
-
-  ModelTypeSet requested_types = GetRoutingInfoTypes(
-      session->routing_info());
-
-  DVLOG(1) << update_count << " entries to verify";
-  for (int i = 0; i < update_count; i++) {
-    const sync_pb::SyncEntity& update = updates.entries(i);
-    ModelSafeGroup g = GetGroupForModelType(GetModelType(update),
-                                            session->routing_info());
-    if (g != status->group_restriction())
-      continue;
-
-    VerifyUpdateResult result = VerifyUpdate(&trans, update,
-                                             requested_types,
-                                             session->routing_info());
-    status->mutable_update_progress()->AddVerifyResult(result.value, update);
-    status->increment_num_updates_downloaded_by(1);
-    if (!UpdateContainsNewVersion(&trans, update))
-      status->increment_num_reflected_updates_downloaded_by(1);
-    if (update.deleted())
-      status->increment_num_tombstone_updates_downloaded_by(1);
-  }
-
-  return SYNCER_OK;
-}
-
-VerifyUpdatesCommand::VerifyUpdateResult VerifyUpdatesCommand::VerifyUpdate(
-    syncable::WriteTransaction* trans, const sync_pb::SyncEntity& entry,
-    ModelTypeSet requested_types,
-    const ModelSafeRoutingInfo& routes) {
-  syncable::Id id = SyncableIdFromProto(entry.id_string());
-  VerifyUpdateResult result = {VERIFY_FAIL, GROUP_PASSIVE};
-
-  const bool deleted = entry.has_deleted() && entry.deleted();
-  const bool is_directory = IsFolder(entry);
-  const ModelType model_type = GetModelType(entry);
-
-  if (!id.ServerKnows()) {
-    LOG(ERROR) << "Illegal negative id in received updates";
-    return result;
-  }
-  {
-    const std::string name = SyncerProtoUtil::NameFromSyncEntity(entry);
-    if (name.empty() && !deleted) {
-      LOG(ERROR) << "Zero length name in non-deleted update";
-      return result;
-    }
-  }
-
-  syncable::MutableEntry same_id(trans, GET_BY_ID, id);
-  result.value = VerifyNewEntry(entry, &same_id, deleted);
-
-  ModelType placement_type = !deleted ? GetModelType(entry)
-      : same_id.good() ? same_id.GetModelType() : UNSPECIFIED;
-  result.placement = GetGroupForModelType(placement_type, routes);
-
-  if (VERIFY_UNDECIDED == result.value) {
-    result.value = VerifyTagConsistency(entry, same_id);
-  }
-
-  if (VERIFY_UNDECIDED == result.value) {
-    if (deleted) {
-      // For deletes the server could send tombostones for items that
-      // the client did not request. If so ignore those items.
-      if (IsRealDataType(placement_type) &&
-          !requested_types.Has(placement_type)) {
-        result.value = VERIFY_SKIP;
-      } else {
-        result.value = VERIFY_SUCCESS;
-      }
-    }
-  }
-
-  // If we have an existing entry, we check here for updates that break
-  // consistency rules.
-  if (VERIFY_UNDECIDED == result.value) {
-    result.value = VerifyUpdateConsistency(trans, entry, &same_id,
-        deleted, is_directory, model_type);
-  }
-
-  if (VERIFY_UNDECIDED == result.value)
-    result.value = VERIFY_SUCCESS;  // No news is good news.
-
-  return result;  // This might be VERIFY_SUCCESS as well
-}
-
-}  // namespace syncer
diff --git a/sync/engine/verify_updates_command.h b/sync/engine/verify_updates_command.h
deleted file mode 100644
index 1788b77..0000000
--- a/sync/engine/verify_updates_command.h
+++ /dev/null
@@ -1,48 +0,0 @@
-// Copyright (c) 2012 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 SYNC_ENGINE_VERIFY_UPDATES_COMMAND_H_
-#define SYNC_ENGINE_VERIFY_UPDATES_COMMAND_H_
-
-#include "base/basictypes.h"
-#include "base/compiler_specific.h"
-#include "sync/engine/model_changing_syncer_command.h"
-#include "sync/engine/syncer_types.h"
-#include "sync/internal_api/public/engine/model_safe_worker.h"
-
-namespace syncer {
-
-namespace syncable {
-class WriteTransaction;
-}
-
-// Verifies the response from a GetUpdates request. All invalid updates will be
-// noted in the SyncSession after this command is executed.
-class VerifyUpdatesCommand : public ModelChangingSyncerCommand {
- public:
-  VerifyUpdatesCommand();
-  virtual ~VerifyUpdatesCommand();
-
- protected:
-  // ModelChangingSyncerCommand implementation.
-  virtual std::set<ModelSafeGroup> GetGroupsToChange(
-      const sessions::SyncSession& session) const OVERRIDE;
-  virtual SyncerError ModelChangingExecuteImpl(
-      sessions::SyncSession* session) OVERRIDE;
-
- private:
-  struct VerifyUpdateResult {
-    VerifyResult value;
-    ModelSafeGroup placement;
-  };
-  VerifyUpdateResult VerifyUpdate(syncable::WriteTransaction* trans,
-                                  const sync_pb::SyncEntity& entry,
-                                  ModelTypeSet requested_types,
-                                  const ModelSafeRoutingInfo& routes);
-  DISALLOW_COPY_AND_ASSIGN(VerifyUpdatesCommand);
-};
-
-}  // namespace syncer
-
-#endif  // SYNC_ENGINE_VERIFY_UPDATES_COMMAND_H_
diff --git a/sync/engine/verify_updates_command_unittest.cc b/sync/engine/verify_updates_command_unittest.cc
deleted file mode 100644
index c0ea363..0000000
--- a/sync/engine/verify_updates_command_unittest.cc
+++ /dev/null
@@ -1,109 +0,0 @@
-// Copyright (c) 2012 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 "base/location.h"
-#include "sync/engine/verify_updates_command.h"
-#include "sync/protocol/bookmark_specifics.pb.h"
-#include "sync/sessions/session_state.h"
-#include "sync/sessions/sync_session.h"
-#include "sync/syncable/mutable_entry.h"
-#include "sync/syncable/syncable_id.h"
-#include "sync/test/engine/fake_model_worker.h"
-#include "sync/test/engine/syncer_command_test.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace syncer {
-
-using sessions::StatusController;
-using std::string;
-using syncable::Id;
-using syncable::MutableEntry;
-using syncable::UNITTEST;
-using syncable::WriteTransaction;
-
-class VerifyUpdatesCommandTest : public SyncerCommandTest {
- public:
-  virtual void SetUp() {
-    workers()->clear();
-    mutable_routing_info()->clear();
-    workers()->push_back(make_scoped_refptr(new FakeModelWorker(GROUP_DB)));
-    workers()->push_back(make_scoped_refptr(new FakeModelWorker(GROUP_UI)));
-    (*mutable_routing_info())[PREFERENCES] = GROUP_UI;
-    (*mutable_routing_info())[BOOKMARKS] = GROUP_UI;
-    (*mutable_routing_info())[AUTOFILL] = GROUP_DB;
-    SyncerCommandTest::SetUp();
-  }
-
-  void CreateLocalItem(const std::string& item_id,
-                       const std::string& parent_id,
-                       const ModelType& type) {
-    WriteTransaction trans(FROM_HERE, UNITTEST, directory());
-    MutableEntry entry(&trans, syncable::CREATE_NEW_UPDATE_ITEM,
-        Id::CreateFromServerId(item_id));
-    ASSERT_TRUE(entry.good());
-
-    entry.Put(syncable::BASE_VERSION, 1);
-    entry.Put(syncable::SERVER_VERSION, 1);
-    entry.Put(syncable::NON_UNIQUE_NAME, item_id);
-    entry.Put(syncable::PARENT_ID, Id::CreateFromServerId(parent_id));
-    sync_pb::EntitySpecifics default_specifics;
-    AddDefaultFieldValue(type, &default_specifics);
-    entry.Put(syncable::SERVER_SPECIFICS, default_specifics);
-  }
-
-  void AddUpdate(sync_pb::GetUpdatesResponse* updates,
-      const std::string& id, const std::string& parent,
-      const ModelType& type) {
-    sync_pb::SyncEntity* e = updates->add_entries();
-    e->set_id_string("b1");
-    e->set_parent_id_string(parent);
-    e->set_non_unique_name("b1");
-    e->set_name("b1");
-    AddDefaultFieldValue(type, e->mutable_specifics());
-  }
-
-  VerifyUpdatesCommand command_;
-
-};
-
-TEST_F(VerifyUpdatesCommandTest, AllVerified) {
-  string root = syncable::GetNullId().GetServerId();
-
-  CreateLocalItem("b1", root, BOOKMARKS);
-  CreateLocalItem("b2", root, BOOKMARKS);
-  CreateLocalItem("p1", root, PREFERENCES);
-  CreateLocalItem("a1", root, AUTOFILL);
-
-  ExpectNoGroupsToChange(command_);
-
-  sync_pb::GetUpdatesResponse* updates =
-      session()->mutable_status_controller()->
-      mutable_updates_response()->mutable_get_updates();
-  AddUpdate(updates, "b1", root, BOOKMARKS);
-  AddUpdate(updates, "b2", root, BOOKMARKS);
-  AddUpdate(updates, "p1", root, PREFERENCES);
-  AddUpdate(updates, "a1", root, AUTOFILL);
-
-  ExpectGroupsToChange(command_, GROUP_UI, GROUP_DB);
-
-  command_.ExecuteImpl(session());
-
-  StatusController* status = session()->mutable_status_controller();
-  {
-    sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);
-    ASSERT_TRUE(status->update_progress());
-    EXPECT_EQ(3, status->update_progress()->VerifiedUpdatesSize());
-  }
-  {
-    sessions::ScopedModelSafeGroupRestriction r(status, GROUP_DB);
-    ASSERT_TRUE(status->update_progress());
-    EXPECT_EQ(1, status->update_progress()->VerifiedUpdatesSize());
-  }
-  {
-    sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE);
-    EXPECT_FALSE(status->update_progress());
-  }
-}
-
-}  // namespace syncer
diff --git a/sync/sessions/sync_session.cc b/sync/sessions/sync_session.cc
index 5586819..5a2b572 100644
--- a/sync/sessions/sync_session.cc
+++ b/sync/sessions/sync_session.cc
@@ -219,24 +219,6 @@ std::set<ModelSafeGroup> SyncSession::GetEnabledGroupsWithConflicts() const {
   return enabled_groups_with_conflicts;
 }
 
-std::set<ModelSafeGroup>
-    SyncSession::GetEnabledGroupsWithVerifiedUpdates() const {
-  const std::set<ModelSafeGroup>& enabled_groups = GetEnabledGroups();
-  std::set<ModelSafeGroup> enabled_groups_with_verified_updates;
-  for (std::set<ModelSafeGroup>::const_iterator it =
-           enabled_groups.begin(); it != enabled_groups.end(); ++it) {
-    const UpdateProgress* update_progress =
-        status_controller_->GetUnrestrictedUpdateProgress(*it);
-    if (update_progress &&
-        (update_progress->VerifiedUpdatesBegin() !=
-         update_progress->VerifiedUpdatesEnd())) {
-      enabled_groups_with_verified_updates.insert(*it);
-    }
-  }
-
-  return enabled_groups_with_verified_updates;
-}
-
 namespace {
 
 // Returns false iff one of the command results had an error.
diff --git a/sync/sessions/sync_session.h b/sync/sessions/sync_session.h
index 461d98e..93d7f41 100644
--- a/sync/sessions/sync_session.h
+++ b/sync/sessions/sync_session.h
@@ -177,9 +177,6 @@ class SyncSession {
   // Returns the set of enabled groups that have conflicts.
   std::set<ModelSafeGroup> GetEnabledGroupsWithConflicts() const;
 
-  // Returns the set of enabled groups that have verified updates.
-  std::set<ModelSafeGroup> GetEnabledGroupsWithVerifiedUpdates() const;
-
   // Mark the session has having finished all the sync steps it needed.
   void SetFinished();
 
diff --git a/sync/sync.gyp b/sync/sync.gyp
index 7347348..0d32ffb 100644
--- a/sync/sync.gyp
+++ b/sync/sync.gyp
@@ -137,8 +137,6 @@
         'engine/traffic_recorder.h',
         'engine/update_applicator.cc',
         'engine/update_applicator.h',
-        'engine/verify_updates_command.cc',
-        'engine/verify_updates_command.h',
         'js/js_arg_list.cc',
         'js/js_arg_list.h',
         'js/js_backend.h',
@@ -616,7 +614,6 @@
           'engine/syncer_unittest.cc',
           'engine/throttled_data_type_tracker_unittest.cc',
           'engine/traffic_recorder_unittest.cc',
-          'engine/verify_updates_command_unittest.cc',
           'js/js_arg_list_unittest.cc',
           'js/js_event_details_unittest.cc',
           'js/sync_js_controller_unittest.cc',