sync: Remove test dependencies on SyncerCommand

Refactor the ApplyControlDataUpdates and Download unit tests to no
longer depend on SyncerCommand and related concepts, including
ModelSafeRoutingInfo, SyncSession, and SyncSessionContext.

The refactoring required rewriting some non-test code to expose new or
different signatures.  These changes should have no effect on non-test
behavior.

This CL also removes a useless field from the StatusController.
Removing that field made the refactoring of download.cc a bit easier.

These changes prepare for a refactoring that will remove
ModelChangingSyncerCommand.  This is one more step on the path to hiding
threading details from the sync engine.

BUG=278484

Review URL: https://codereview.chromium.org/53763002

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

7 files changed, 341 insertions, 164 deletions

diff --git a/sync/engine/apply_control_data_updates.cc b/sync/engine/apply_control_data_updates.cc
index 137560d..e97741e 100644
--- a/sync/engine/apply_control_data_updates.cc
+++ b/sync/engine/apply_control_data_updates.cc
@@ -24,8 +24,7 @@ using syncable::SERVER_SPECIFICS;
 using syncable::SPECIFICS;
 using syncable::SYNCER;
 
-void ApplyControlDataUpdates(sessions::SyncSession* session) {
-  syncable::Directory* dir = session->context()->directory();
+void ApplyControlDataUpdates(syncable::Directory* dir) {
   syncable::WriteTransaction trans(FROM_HERE, SYNCER, dir);
 
   std::vector<int64> handles;
diff --git a/sync/engine/apply_control_data_updates.h b/sync/engine/apply_control_data_updates.h
index 4646cb0..b825665e 100644
--- a/sync/engine/apply_control_data_updates.h
+++ b/sync/engine/apply_control_data_updates.h
@@ -11,18 +11,13 @@ namespace syncer {
 
 class Cryptographer;
 
-namespace sessions {
-class SyncSession;
-}
-
 namespace syncable {
 class Directory;
 class MutableEntry;
 class WriteTransaction;
 }
 
-SYNC_EXPORT_PRIVATE void ApplyControlDataUpdates(
-    sessions::SyncSession* session);
+SYNC_EXPORT_PRIVATE void ApplyControlDataUpdates(syncable::Directory* dir);
 void ApplyNigoriUpdate(syncable::WriteTransaction* trans,
                        syncable::MutableEntry* const entry,
                        Cryptographer* cryptographer);
diff --git a/sync/engine/apply_control_data_updates_unittest.cc b/sync/engine/apply_control_data_updates_unittest.cc
index f3d173c..caacfbc 100644
--- a/sync/engine/apply_control_data_updates_unittest.cc
+++ b/sync/engine/apply_control_data_updates_unittest.cc
@@ -5,19 +5,21 @@
 #include "base/format_macros.h"
 #include "base/location.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
 #include "base/strings/stringprintf.h"
 #include "sync/engine/apply_control_data_updates.h"
 #include "sync/engine/syncer.h"
 #include "sync/engine/syncer_util.h"
 #include "sync/internal_api/public/test/test_entry_factory.h"
 #include "sync/protocol/nigori_specifics.pb.h"
+#include "sync/syncable/directory.h"
 #include "sync/syncable/mutable_entry.h"
 #include "sync/syncable/nigori_util.h"
 #include "sync/syncable/syncable_read_transaction.h"
 #include "sync/syncable/syncable_util.h"
 #include "sync/syncable/syncable_write_transaction.h"
 #include "sync/test/engine/fake_model_worker.h"
-#include "sync/test/engine/syncer_command_test.h"
+#include "sync/test/engine/test_directory_setter_upper.h"
 #include "sync/test/engine/test_id_factory.h"
 #include "sync/test/fake_sync_encryption_handler.h"
 #include "sync/util/cryptographer.h"
@@ -29,32 +31,31 @@ using syncable::MutableEntry;
 using syncable::UNITTEST;
 using syncable::Id;
 
-class ApplyControlDataUpdatesTest : public SyncerCommandTest {
+class ApplyControlDataUpdatesTest : public ::testing::Test {
  public:
  protected:
   ApplyControlDataUpdatesTest() {}
   virtual ~ApplyControlDataUpdatesTest() {}
 
   virtual void SetUp() {
-    workers()->clear();
-    mutable_routing_info()->clear();
-    workers()->push_back(make_scoped_refptr(new FakeModelWorker(GROUP_UI)));
-    workers()->push_back(
-        make_scoped_refptr(new FakeModelWorker(GROUP_PASSWORD)));
-    (*mutable_routing_info())[NIGORI] = GROUP_PASSIVE;
-    (*mutable_routing_info())[EXPERIMENTS] = GROUP_PASSIVE;
-    SyncerCommandTest::SetUp();
+    dir_maker_.SetUp();
     entry_factory_.reset(new TestEntryFactory(directory()));
+  }
 
-    session()->mutable_status_controller()->set_updates_request_types(
-        ControlTypes());
+  virtual void TearDown() {
+    dir_maker_.TearDown();
+  }
 
-    syncable::ReadTransaction trans(FROM_HERE, directory());
+  syncable::Directory* directory() {
+    return dir_maker_.directory();
   }
 
   TestIdFactory id_factory_;
   scoped_ptr<TestEntryFactory> entry_factory_;
  private:
+  base::MessageLoop loop_;  // Needed for directory init.
+  TestDirectorySetterUpper dir_maker_;
+
   DISALLOW_COPY_AND_ASSIGN(ApplyControlDataUpdatesTest);
 };
 
@@ -87,7 +88,7 @@ TEST_F(ApplyControlDataUpdatesTest, NigoriUpdate) {
       ModelTypeToRootTag(NIGORI), specifics, true);
   EXPECT_FALSE(cryptographer->has_pending_keys());
 
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
 
   EXPECT_FALSE(cryptographer->is_ready());
   EXPECT_TRUE(cryptographer->has_pending_keys());
@@ -166,7 +167,7 @@ TEST_F(ApplyControlDataUpdatesTest, EncryptUnsyncedChanges) {
     EXPECT_EQ(2*batch_s+1, handles.size());
   }
 
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
 
   EXPECT_FALSE(cryptographer->has_pending_keys());
   EXPECT_TRUE(cryptographer->is_ready());
@@ -194,7 +195,7 @@ TEST_F(ApplyControlDataUpdatesTest, EncryptUnsyncedChanges) {
     entry.PutIsUnappliedUpdate(true);
   }
 
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
 
   EXPECT_FALSE(cryptographer->has_pending_keys());
   EXPECT_TRUE(cryptographer->is_ready());
@@ -281,7 +282,7 @@ TEST_F(ApplyControlDataUpdatesTest, CannotEncryptUnsyncedChanges) {
     EXPECT_EQ(2*batch_s+1, handles.size());
   }
 
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
 
   EXPECT_FALSE(cryptographer->is_ready());
   EXPECT_TRUE(cryptographer->has_pending_keys());
@@ -359,7 +360,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -437,7 +438,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -510,7 +511,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -589,7 +590,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -671,7 +672,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -751,7 +752,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -831,7 +832,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -865,7 +866,7 @@ TEST_F(ApplyControlDataUpdatesTest, ControlApply) {
       set_enabled(true);
   int64 experiment_handle = entry_factory_->CreateUnappliedNewItem(
       experiment_id, specifics, false);
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
 
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(experiment_handle));
   EXPECT_TRUE(
@@ -884,7 +885,7 @@ TEST_F(ApplyControlDataUpdatesTest, ControlApplyParentBeforeChild) {
       experiment_id, specifics, parent_id);
   int64 parent_handle = entry_factory_->CreateUnappliedNewItem(
       parent_id, specifics, true);
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
 
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(parent_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(experiment_handle));
@@ -908,7 +909,7 @@ TEST_F(ApplyControlDataUpdatesTest, ControlConflict) {
                                             server_specifics);
   entry_factory_->SetLocalSpecificsForItem(experiment_handle,
                                            local_specifics);
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
 
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(experiment_handle));
   EXPECT_TRUE(
diff --git a/sync/engine/download.cc b/sync/engine/download.cc
index 9b74417..53d36f5 100644
--- a/sync/engine/download.cc
+++ b/sync/engine/download.cc
@@ -16,8 +16,6 @@
 #include "sync/syncable/nigori_handler.h"
 #include "sync/syncable/syncable_read_transaction.h"
 
-using sync_pb::DebugInfo;
-
 namespace syncer {
 
 using sessions::StatusController;
@@ -25,6 +23,8 @@ using sessions::SyncSession;
 using sessions::SyncSessionContext;
 using std::string;
 
+namespace download {
+
 namespace {
 
 typedef std::map<ModelType, size_t> TypeToIndexMap;
@@ -80,27 +80,10 @@ bool ShouldRequestEncryptionKey(
   return need_encryption_key;
 }
 
-void AppendClientDebugInfoIfNeeded(
-    SyncSession* session,
-    DebugInfo* debug_info) {
-  // We want to send the debug info only once per sync cycle. Check if it has
-  // already been sent.
-  if (!session->status_controller().debug_info_sent()) {
-    DVLOG(1) << "Sending client debug info ...";
-    // Could be null in some unit tests.
-    if (session->context()->debug_info_getter()) {
-      session->context()->debug_info_getter()->GetAndClearDebugInfo(
-          debug_info);
-    }
-    session->mutable_status_controller()->set_debug_info_sent();
-  }
-}
-
-void InitDownloadUpdatesRequest(
+void InitDownloadUpdatesContext(
     SyncSession* session,
     bool create_mobile_bookmarks_folder,
-    sync_pb::ClientToServerMessage* message,
-    ModelTypeSet proto_request_types) {
+    sync_pb::ClientToServerMessage* message) {
   message->set_share(session->context()->account_name());
   message->set_message_contents(sync_pb::ClientToServerMessage::GET_UPDATES);
 
@@ -111,8 +94,10 @@ void InitDownloadUpdatesRequest(
   // (e.g. Bookmark URLs but not their containing folders).
   get_updates->set_fetch_folders(true);
 
-  DebugInfo* debug_info = message->mutable_debug_info();
-  AppendClientDebugInfoIfNeeded(session, debug_info);
+  sync_pb::DebugInfo* debug_info = message->mutable_debug_info();
+  AppendClientDebugInfoIfNeeded(session->context()->debug_info_getter(),
+                                session->mutable_status_controller(),
+                                debug_info);
 
   get_updates->set_create_mobile_bookmarks_folder(
       create_mobile_bookmarks_folder);
@@ -122,12 +107,12 @@ void InitDownloadUpdatesRequest(
   // Set legacy GetUpdatesMessage.GetUpdatesCallerInfo information.
   get_updates->mutable_caller_info()->set_notifications_enabled(
       session->context()->notifications_enabled());
+}
 
-  StatusController* status = session->mutable_status_controller();
-  status->set_updates_request_types(proto_request_types);
-
-  UpdateHandlerMap* handler_map = session->context()->update_handler_map();
-
+void InitDownloadUpdatesProgress(
+    ModelTypeSet proto_request_types,
+    UpdateHandlerMap* handler_map,
+    sync_pb::GetUpdatesMessage* get_updates) {
   for (ModelTypeSet::Iterator it = proto_request_types.First();
        it.Good(); it.Inc()) {
     UpdateHandlerMap::iterator handler_it = handler_map->find(it.Get());
@@ -219,19 +204,35 @@ void BuildNormalDownloadUpdates(
     ModelTypeSet request_types,
     const sessions::NudgeTracker& nudge_tracker,
     sync_pb::ClientToServerMessage* client_to_server_message) {
-  InitDownloadUpdatesRequest(
-      session,
-      create_mobile_bookmarks_folder,
-      client_to_server_message,
-      Intersection(request_types, ProtocolTypes()));
-  sync_pb::GetUpdatesMessage* get_updates =
-      client_to_server_message->mutable_get_updates();
-
   // Request updates for all requested types.
   DVLOG(1) << "Getting updates for types "
            << ModelTypeSetToString(request_types);
   DCHECK(!request_types.Empty());
 
+  InitDownloadUpdatesContext(
+      session,
+      create_mobile_bookmarks_folder,
+      client_to_server_message);
+
+  BuildNormalDownloadUpdatesImpl(
+      Intersection(request_types, ProtocolTypes()),
+      session->context()->update_handler_map(),
+      nudge_tracker,
+      client_to_server_message->mutable_get_updates());
+}
+
+void BuildNormalDownloadUpdatesImpl(
+    ModelTypeSet proto_request_types,
+    UpdateHandlerMap* update_handler_map,
+    const sessions::NudgeTracker& nudge_tracker,
+    sync_pb::GetUpdatesMessage* get_updates) {
+  DCHECK(!proto_request_types.Empty());
+
+  InitDownloadUpdatesProgress(
+      proto_request_types,
+      update_handler_map,
+      get_updates);
+
   // Set legacy GetUpdatesMessage.GetUpdatesCallerInfo information.
   get_updates->mutable_caller_info()->set_source(
       nudge_tracker.updates_source());
@@ -262,18 +263,32 @@ void BuildDownloadUpdatesForConfigure(
     sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source,
     ModelTypeSet request_types,
     sync_pb::ClientToServerMessage* client_to_server_message) {
-  InitDownloadUpdatesRequest(
-      session,
-      create_mobile_bookmarks_folder,
-      client_to_server_message,
-      Intersection(request_types, ProtocolTypes()));
-  sync_pb::GetUpdatesMessage* get_updates =
-      client_to_server_message->mutable_get_updates();
-
   // Request updates for all enabled types.
   DVLOG(1) << "Initial download for types "
            << ModelTypeSetToString(request_types);
-  DCHECK(!request_types.Empty());
+
+  InitDownloadUpdatesContext(
+      session,
+      create_mobile_bookmarks_folder,
+      client_to_server_message);
+  BuildDownloadUpdatesForConfigureImpl(
+      Intersection(request_types, ProtocolTypes()),
+      session->context()->update_handler_map(),
+      source,
+      client_to_server_message->mutable_get_updates());
+}
+
+void BuildDownloadUpdatesForConfigureImpl(
+    ModelTypeSet proto_request_types,
+    UpdateHandlerMap* update_handler_map,
+    sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source,
+    sync_pb::GetUpdatesMessage* get_updates) {
+  DCHECK(!proto_request_types.Empty());
+
+  InitDownloadUpdatesProgress(
+      proto_request_types,
+      update_handler_map,
+      get_updates);
 
   // Set legacy GetUpdatesMessage.GetUpdatesCallerInfo information.
   get_updates->mutable_caller_info()->set_source(source);
@@ -289,17 +304,29 @@ void BuildDownloadUpdatesForPoll(
     bool create_mobile_bookmarks_folder,
     ModelTypeSet request_types,
     sync_pb::ClientToServerMessage* client_to_server_message) {
-  InitDownloadUpdatesRequest(
+  DVLOG(1) << "Polling for types "
+           << ModelTypeSetToString(request_types);
+
+  InitDownloadUpdatesContext(
       session,
       create_mobile_bookmarks_folder,
-      client_to_server_message,
-      Intersection(request_types, ProtocolTypes()));
-  sync_pb::GetUpdatesMessage* get_updates =
-      client_to_server_message->mutable_get_updates();
+      client_to_server_message);
+  BuildDownloadUpdatesForPollImpl(
+      Intersection(request_types, ProtocolTypes()),
+      session->context()->update_handler_map(),
+      client_to_server_message->mutable_get_updates());
+}
 
-  DVLOG(1) << "Polling for types "
-           << ModelTypeSetToString(request_types);
-  DCHECK(!request_types.Empty());
+void BuildDownloadUpdatesForPollImpl(
+    ModelTypeSet proto_request_types,
+    UpdateHandlerMap* update_handler_map,
+    sync_pb::GetUpdatesMessage* get_updates) {
+  DCHECK(!proto_request_types.Empty());
+
+  InitDownloadUpdatesProgress(
+      proto_request_types,
+      update_handler_map,
+      get_updates);
 
   // Set legacy GetUpdatesMessage.GetUpdatesCallerInfo information.
   get_updates->mutable_caller_info()->set_source(
@@ -365,4 +392,22 @@ SyncerError ExecuteDownloadUpdates(
   }
 }
 
+void AppendClientDebugInfoIfNeeded(
+    sessions::DebugInfoGetter* debug_info_getter,
+    StatusController* status,
+    sync_pb::DebugInfo* debug_info) {
+  // We want to send the debug info only once per sync cycle. Check if it has
+  // already been sent.
+  if (!status->debug_info_sent()) {
+    DVLOG(1) << "Sending client debug info ...";
+    // Could be null in some unit tests.
+    if (debug_info_getter) {
+      debug_info_getter->GetAndClearDebugInfo(debug_info);
+    }
+    status->set_debug_info_sent();
+  }
+}
+
+}  // namespace download
+
 }  // namespace syncer
diff --git a/sync/engine/download.h b/sync/engine/download.h
index 7c5c582..ec085c9 100644
--- a/sync/engine/download.h
+++ b/sync/engine/download.h
@@ -6,6 +6,7 @@
 #define SYNC_ENGINE_DOWNLOAD_H_
 
 #include "sync/base/sync_export.h"
+#include "sync/engine/sync_directory_update_handler.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/util/syncer_error.h"
 #include "sync/protocol/sync.pb.h"
@@ -17,11 +18,13 @@ class DebugInfo;
 namespace syncer {
 
 namespace sessions {
+class DebugInfoGetter;
 class NudgeTracker;
+class StatusController;
 class SyncSession;
 }  // namespace sessions
 
-class Syncer;
+namespace download {
 
 // This function executes a single GetUpdate request and stores the response in
 // the session's StatusController.  It constructs the type of request used to
@@ -33,6 +36,13 @@ SYNC_EXPORT_PRIVATE void BuildNormalDownloadUpdates(
     const sessions::NudgeTracker& nudge_tracker,
     sync_pb::ClientToServerMessage* client_to_server_message);
 
+// Helper function.  Defined here for testing.
+void SYNC_EXPORT_PRIVATE BuildNormalDownloadUpdatesImpl(
+    ModelTypeSet proto_request_types,
+    UpdateHandlerMap* update_handler_map,
+    const sessions::NudgeTracker& nudge_tracker,
+    sync_pb::GetUpdatesMessage* get_updates);
+
 // This function executes a single GetUpdate request and stores the response in
 // the session's StatusController.  It constructs the type of request used to
 // initialize a type for the first time.
@@ -43,6 +53,13 @@ SYNC_EXPORT_PRIVATE void BuildDownloadUpdatesForConfigure(
     ModelTypeSet request_types,
     sync_pb::ClientToServerMessage* client_to_server_message);
 
+// Helper function.  Defined here for testing.
+void SYNC_EXPORT_PRIVATE BuildDownloadUpdatesForConfigureImpl(
+    ModelTypeSet proto_request_types,
+    UpdateHandlerMap* update_handler_map,
+    sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source,
+    sync_pb::GetUpdatesMessage* get_updates);
+
 // This function executes a single GetUpdate request and stores the response in
 // the session's status controller.  It constructs the type of request used for
 // periodic polling.
@@ -52,6 +69,12 @@ SYNC_EXPORT_PRIVATE void BuildDownloadUpdatesForPoll(
     ModelTypeSet request_types,
     sync_pb::ClientToServerMessage* client_to_server_message);
 
+// Helper function.  Defined here for testing.
+void SYNC_EXPORT_PRIVATE BuildDownloadUpdatesForPollImpl(
+    ModelTypeSet proto_request_types,
+    UpdateHandlerMap* update_handler_map,
+    sync_pb::GetUpdatesMessage* get_updates);
+
 // Sends the specified message to the server and stores the response in a member
 // of the |session|'s StatusController.
 SYNC_EXPORT_PRIVATE SyncerError
@@ -59,6 +82,15 @@ SYNC_EXPORT_PRIVATE SyncerError
                            sessions::SyncSession* session,
                            sync_pb::ClientToServerMessage* msg);
 
+// Helper function to append client debug info to the message, but only do so
+// once per sync cycle.  Defined here for testing.
+void SYNC_EXPORT_PRIVATE AppendClientDebugInfoIfNeeded(
+    sessions::DebugInfoGetter* debug_info_getter,
+    sessions::StatusController* status,
+    sync_pb::DebugInfo* debug_info);
+
+}  // namespace download
+
 }  // namespace syncer
 
 #endif  // SYNC_ENGINE_DOWNLOAD_H_
diff --git a/sync/engine/download_unittest.cc b/sync/engine/download_unittest.cc
index fb4431a..f38ab2b 100644
--- a/sync/engine/download_unittest.cc
+++ b/sync/engine/download_unittest.cc
@@ -3,49 +3,84 @@
 // found in the LICENSE file.
 
 #include "sync/engine/download.h"
+
+#include "base/message_loop/message_loop.h"
+#include "base/stl_util.h"
+#include "sync/engine/sync_directory_update_handler.h"
 #include "sync/internal_api/public/base/model_type_test_util.h"
 #include "sync/protocol/sync.pb.h"
+#include "sync/sessions/debug_info_getter.h"
 #include "sync/sessions/nudge_tracker.h"
-#include "sync/test/engine/fake_model_worker.h"
-#include "sync/test/engine/syncer_command_test.h"
+#include "sync/sessions/status_controller.h"
+#include "sync/syncable/directory.h"
+#include "sync/test/engine/test_directory_setter_upper.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 namespace syncer {
 
 // A test fixture for tests exercising download updates functions.
-class DownloadUpdatesTest : public SyncerCommandTest {
+class DownloadUpdatesTest : public ::testing::Test {
  protected:
-  DownloadUpdatesTest() {
+  DownloadUpdatesTest()
+      : update_handler_map_deleter_(&update_handler_map_) {
   }
 
   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())[AUTOFILL] = GROUP_DB;
-    (*mutable_routing_info())[BOOKMARKS] = GROUP_UI;
-    (*mutable_routing_info())[PREFERENCES] = GROUP_UI;
-    SyncerCommandTest::SetUp();
+    dir_maker_.SetUp();
+
+    AddUpdateHandler(AUTOFILL);
+    AddUpdateHandler(BOOKMARKS);
+    AddUpdateHandler(PREFERENCES);
+  }
+
+  virtual void TearDown() {
+    dir_maker_.TearDown();
+  }
+
+  ModelTypeSet proto_request_types() {
+    ModelTypeSet types;
+    for (UpdateHandlerMap::iterator it = update_handler_map_.begin();
+         it != update_handler_map_.end(); ++it) {
+      types.Put(it->first);
+    }
+    return types;
+  }
+
+  syncable::Directory* directory() {
+    return dir_maker_.directory();
+  }
+
+  UpdateHandlerMap* update_handler_map() {
+    return &update_handler_map_;
   }
 
  private:
+  void AddUpdateHandler(ModelType type) {
+    DCHECK(directory());
+    update_handler_map_.insert(
+        std::make_pair(type,
+                       new SyncDirectoryUpdateHandler(directory(), type)));
+  }
+
+  base::MessageLoop loop_;  // Needed for directory init.
+  TestDirectorySetterUpper dir_maker_;
+
+  UpdateHandlerMap update_handler_map_;
+  STLValueDeleter<UpdateHandlerMap> update_handler_map_deleter_;
+
   DISALLOW_COPY_AND_ASSIGN(DownloadUpdatesTest);
 };
 
-TEST_F(DownloadUpdatesTest, ExecuteNoStates) {
+// Basic test to make sure nudges are expressed properly in the request.
+TEST_F(DownloadUpdatesTest, BookmarkNudge) {
   sessions::NudgeTracker nudge_tracker;
   nudge_tracker.RecordLocalChange(ModelTypeSet(BOOKMARKS));
 
-  scoped_ptr<sessions::SyncSession> session(
-      sessions::SyncSession::Build(context(), delegate()));
   sync_pb::ClientToServerMessage msg;
-  BuildNormalDownloadUpdates(session.get(),
-                             false,
-                             GetRoutingInfoTypes(routing_info()),
-                             nudge_tracker,
-                             &msg);
+  download::BuildNormalDownloadUpdatesImpl(proto_request_types(),
+                                           update_handler_map(),
+                                           nudge_tracker,
+                                           msg.mutable_get_updates());
 
   const sync_pb::GetUpdatesMessage& gu_msg = msg.get_updates();
   EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::LOCAL,
@@ -54,7 +89,6 @@ TEST_F(DownloadUpdatesTest, ExecuteNoStates) {
   for (int i = 0; i < gu_msg.from_progress_marker_size(); ++i) {
     syncer::ModelType type = GetModelTypeFromSpecificsFieldNumber(
         gu_msg.from_progress_marker(i).data_type_id());
-    EXPECT_TRUE(GetRoutingInfoTypes(routing_info()).Has(type));
 
     const sync_pb::DataTypeProgressMarker& progress_marker =
         gu_msg.from_progress_marker(i);
@@ -76,7 +110,8 @@ TEST_F(DownloadUpdatesTest, ExecuteNoStates) {
   }
 }
 
-TEST_F(DownloadUpdatesTest, ExecuteWithStates) {
+// Basic test to ensure invalidation payloads are expressed in the request.
+TEST_F(DownloadUpdatesTest, NotifyMany) {
   sessions::NudgeTracker nudge_tracker;
   nudge_tracker.RecordRemoteInvalidation(
       BuildInvalidationMap(AUTOFILL, 1, "autofill_payload"));
@@ -89,14 +124,11 @@ TEST_F(DownloadUpdatesTest, ExecuteWithStates) {
   notified_types.Put(BOOKMARKS);
   notified_types.Put(PREFERENCES);
 
-  scoped_ptr<sessions::SyncSession> session(
-      sessions::SyncSession::Build(context(), delegate()));
   sync_pb::ClientToServerMessage msg;
-  BuildNormalDownloadUpdates(session.get(),
-                             false,
-                             GetRoutingInfoTypes(routing_info()),
-                             nudge_tracker,
-                             &msg);
+  download::BuildNormalDownloadUpdatesImpl(proto_request_types(),
+                                           update_handler_map(),
+                                           nudge_tracker,
+                                           msg.mutable_get_updates());
 
   const sync_pb::GetUpdatesMessage& gu_msg = msg.get_updates();
   EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::NOTIFICATION,
@@ -105,7 +137,6 @@ TEST_F(DownloadUpdatesTest, ExecuteWithStates) {
   for (int i = 0; i < gu_msg.from_progress_marker_size(); ++i) {
     syncer::ModelType type = GetModelTypeFromSpecificsFieldNumber(
         gu_msg.from_progress_marker(i).data_type_id());
-    EXPECT_TRUE(GetRoutingInfoTypes(routing_info()).Has(type));
 
     const sync_pb::DataTypeProgressMarker& progress_marker =
         gu_msg.from_progress_marker(i);
@@ -125,45 +156,119 @@ TEST_F(DownloadUpdatesTest, ExecuteWithStates) {
   }
 }
 
-// Test that debug info is sent uploaded only once per sync session.
-TEST_F(DownloadUpdatesTest, VerifyAppendDebugInfo) {
-  // Start by expecting that no events are uploaded.
-  sessions::NudgeTracker nudge_tracker;
-  nudge_tracker.RecordLocalChange(ModelTypeSet(BOOKMARKS));
+TEST_F(DownloadUpdatesTest, ConfigureTest) {
+  sync_pb::ClientToServerMessage msg;
+  download::BuildDownloadUpdatesForConfigureImpl(
+      proto_request_types(),
+      update_handler_map(),
+      sync_pb::GetUpdatesCallerInfo::RECONFIGURATION,
+      msg.mutable_get_updates());
+
+  const sync_pb::GetUpdatesMessage& gu_msg = msg.get_updates();
+
+  EXPECT_EQ(sync_pb::SyncEnums::RECONFIGURATION, gu_msg.get_updates_origin());
+  EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::RECONFIGURATION,
+            gu_msg.caller_info().source());
+
+  ModelTypeSet progress_types;
+  for (int i = 0; i < gu_msg.from_progress_marker_size(); ++i) {
+    syncer::ModelType type = GetModelTypeFromSpecificsFieldNumber(
+        gu_msg.from_progress_marker(i).data_type_id());
+    progress_types.Put(type);
+  }
+  EXPECT_TRUE(proto_request_types().Equals(progress_types));
+}
+
+TEST_F(DownloadUpdatesTest, PollTest) {
+  sync_pb::ClientToServerMessage msg;
+  download::BuildDownloadUpdatesForPollImpl(
+      proto_request_types(),
+      update_handler_map(),
+      msg.mutable_get_updates());
+
+  const sync_pb::GetUpdatesMessage& gu_msg = msg.get_updates();
+
+  EXPECT_EQ(sync_pb::SyncEnums::PERIODIC, gu_msg.get_updates_origin());
+  EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::PERIODIC,
+            gu_msg.caller_info().source());
+
+  ModelTypeSet progress_types;
+  for (int i = 0; i < gu_msg.from_progress_marker_size(); ++i) {
+    syncer::ModelType type = GetModelTypeFromSpecificsFieldNumber(
+        gu_msg.from_progress_marker(i).data_type_id());
+    progress_types.Put(type);
+  }
+  EXPECT_TRUE(proto_request_types().Equals(progress_types));
+}
+
+class MockDebugInfoGetter : public sessions::DebugInfoGetter {
+ public:
+  MockDebugInfoGetter() {}
+  virtual ~MockDebugInfoGetter() {}
+
+  virtual void GetAndClearDebugInfo(sync_pb::DebugInfo* debug_info) OVERRIDE {
+    debug_info->CopyFrom(debug_info_);
+    debug_info_.Clear();
+  }
+
+  void AddDebugEvent() {
+    debug_info_.add_events();
+  }
+
+ private:
+  sync_pb::DebugInfo debug_info_;
+};
+
+class DownloadUpdatesDebugInfoTest : public ::testing::Test {
+ public:
+  DownloadUpdatesDebugInfoTest() {}
+  virtual ~DownloadUpdatesDebugInfoTest() {}
+
+  sessions::StatusController* status() {
+    return &status_;
+  }
+
+  sessions::DebugInfoGetter* debug_info_getter() {
+    return &debug_info_getter_;
+  }
+
+  void AddDebugEvent() {
+    debug_info_getter_.AddDebugEvent();
+  }
+
+ private:
+  sessions::StatusController status_;
+  MockDebugInfoGetter debug_info_getter_;
+};
+
+
+// Verify AppendDebugInfo when there are no events to upload.
+TEST_F(DownloadUpdatesDebugInfoTest, VerifyAppendDebugInfo_Empty) {
+  sync_pb::DebugInfo debug_info;
+  download::AppendClientDebugInfoIfNeeded(debug_info_getter(),
+                                          status(),
+                                          &debug_info);
+  EXPECT_EQ(0, debug_info.events_size());
+}
+
+// We should upload debug info only once per sync cycle.
+TEST_F(DownloadUpdatesDebugInfoTest, TryDoubleAppend) {
+  sync_pb::DebugInfo debug_info1;
+
+  AddDebugEvent();
+  download::AppendClientDebugInfoIfNeeded(debug_info_getter(),
+                                          status(),
+                                          &debug_info1);
+  EXPECT_EQ(1, debug_info1.events_size());
+
 
-  sync_pb::ClientToServerMessage msg1;
-  scoped_ptr<sessions::SyncSession> session1(
-      sessions::SyncSession::Build(context(), delegate()));
-  BuildNormalDownloadUpdates(session1.get(),
-                             false,
-                             GetRoutingInfoTypes(routing_info()),
-                             nudge_tracker,
-                             &msg1);
-  EXPECT_EQ(0, msg1.debug_info().events_size());
-
-  // Create a new session, record an event, and try again.
-  scoped_ptr<sessions::SyncSession> session2(
-      sessions::SyncSession::Build(context(), delegate()));
-  DataTypeConfigurationStats stats;
-  stats.model_type = BOOKMARKS;
-  debug_info_event_listener()->OnDataTypeConfigureComplete(
-      std::vector<DataTypeConfigurationStats>(1, stats));
-  sync_pb::ClientToServerMessage msg2;
-  BuildNormalDownloadUpdates(session2.get(),
-                             false,
-                             GetRoutingInfoTypes(routing_info()),
-                             nudge_tracker,
-                             &msg2);
-  EXPECT_EQ(1, msg2.debug_info().events_size());
-
-  // Events should never be sent up more than once per session.
-  sync_pb::ClientToServerMessage msg3;
-  BuildNormalDownloadUpdates(session2.get(),
-                             false,
-                             GetRoutingInfoTypes(routing_info()),
-                             nudge_tracker,
-                             &msg3);
-  EXPECT_EQ(0, msg3.debug_info().events_size());
+  // Repeated invocations should not send up more events.
+  AddDebugEvent();
+  sync_pb::DebugInfo debug_info2;
+  download::AppendClientDebugInfoIfNeeded(debug_info_getter(),
+                                          status(),
+                                          &debug_info2);
+  EXPECT_EQ(0, debug_info2.events_size());
 }
 
 }  // namespace syncer
diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc
index 50cc167..0c451ee 100644
--- a/sync/engine/syncer.cc
+++ b/sync/engine/syncer.cc
@@ -63,7 +63,7 @@ bool Syncer::NormalSyncShare(ModelTypeSet request_types,
     if (!DownloadAndApplyUpdates(
             request_types,
             session,
-            base::Bind(&BuildNormalDownloadUpdates,
+            base::Bind(&download::BuildNormalDownloadUpdates,
                        session,
                        kCreateMobileBookmarksFolder,
                        request_types,
@@ -88,7 +88,7 @@ bool Syncer::ConfigureSyncShare(
   DownloadAndApplyUpdates(
       request_types,
       session,
-      base::Bind(&BuildDownloadUpdatesForConfigure,
+      base::Bind(&download::BuildDownloadUpdatesForConfigure,
                  session,
                  kCreateMobileBookmarksFolder,
                  source,
@@ -103,7 +103,7 @@ bool Syncer::PollSyncShare(ModelTypeSet request_types,
   DownloadAndApplyUpdates(
       request_types,
       session,
-      base::Bind(&BuildDownloadUpdatesForPoll,
+      base::Bind(&download::BuildDownloadUpdatesForPoll,
                  session,
                  kCreateMobileBookmarksFolder,
                  request_types));
@@ -113,7 +113,7 @@ bool Syncer::PollSyncShare(ModelTypeSet request_types,
 void Syncer::ApplyUpdates(SyncSession* session) {
   TRACE_EVENT0("sync", "ApplyUpdates");
 
-  ApplyControlDataUpdates(session);
+  ApplyControlDataUpdates(session->context()->directory());
 
   ApplyUpdatesAndResolveConflictsCommand apply_updates;
   apply_updates.Execute(session);
@@ -133,7 +133,7 @@ bool Syncer::DownloadAndApplyUpdates(
     sync_pb::ClientToServerMessage msg;
     build_fn.Run(&msg);
     SyncerError download_result =
-        ExecuteDownloadUpdates(request_types, session, &msg);
+        download::ExecuteDownloadUpdates(request_types, session, &msg);
     session->mutable_status_controller()->set_last_download_updates_result(
         download_result);
     if (download_result != SYNCER_OK) {