sync: Move download logic into GetUpdatesProcessor

Moves more of the logic to build and process GetUpdates messages into
the GetUpdatesProcessor.  When refactoring the sync engine, some
functions ended up being left orphaned in download.h.  This CL gets them
back into a class.

This change should make the download updates logic a bit clearer, since
the most of the update request building, sending, and processing logic
will now live entirely in GetUpdatesProcessor and its helper class,
GetUpdatesDelegate.

BUG=278484

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

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

8 files changed, 265 insertions, 300 deletions

diff --git a/sync/engine/download.cc b/sync/engine/download.cc
deleted file mode 100644
index def78dd..0000000
--- a/sync/engine/download.cc
+++ /dev/null
@@ -1,175 +0,0 @@
-// Copyright 2013 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/download.h"
-
-#include <string>
-
-#include "sync/engine/syncer_proto_util.h"
-#include "sync/sessions/sync_session.h"
-#include "sync/sessions/sync_session_context.h"
-#include "sync/syncable/directory.h"
-#include "sync/syncable/nigori_handler.h"
-#include "sync/syncable/syncable_read_transaction.h"
-
-namespace syncer {
-
-using sessions::StatusController;
-using sessions::SyncSession;
-using sessions::SyncSessionContext;
-using std::string;
-
-namespace {
-
-SyncerError HandleGetEncryptionKeyResponse(
-    const sync_pb::ClientToServerResponse& update_response,
-    syncable::Directory* dir) {
-  bool success = false;
-  if (update_response.get_updates().encryption_keys_size() == 0) {
-    LOG(ERROR) << "Failed to receive encryption key from server.";
-    return SERVER_RESPONSE_VALIDATION_FAILED;
-  }
-  syncable::ReadTransaction trans(FROM_HERE, dir);
-  syncable::NigoriHandler* nigori_handler = dir->GetNigoriHandler();
-  success = nigori_handler->SetKeystoreKeys(
-      update_response.get_updates().encryption_keys(),
-      &trans);
-
-  DVLOG(1) << "GetUpdates returned "
-           << update_response.get_updates().encryption_keys_size()
-           << "encryption keys. Nigori keystore key "
-           << (success ? "" : "not ") << "updated.";
-  return (success ? SYNCER_OK : SERVER_RESPONSE_VALIDATION_FAILED);
-}
-
-bool ShouldRequestEncryptionKey(
-    SyncSessionContext* context) {
-  bool need_encryption_key = false;
-  if (context->keystore_encryption_enabled()) {
-    syncable::Directory* dir = context->directory();
-    syncable::ReadTransaction trans(FROM_HERE, dir);
-    syncable::NigoriHandler* nigori_handler = dir->GetNigoriHandler();
-    need_encryption_key = nigori_handler->NeedKeystoreKey(&trans);
-  }
-  return need_encryption_key;
-}
-
-}  // namespace
-
-namespace download {
-
-void InitDownloadUpdatesContext(
-    SyncSession* session,
-    bool create_mobile_bookmarks_folder,
-    sync_pb::ClientToServerMessage* message) {
-  message->set_share(session->context()->account_name());
-  message->set_message_contents(sync_pb::ClientToServerMessage::GET_UPDATES);
-
-  sync_pb::GetUpdatesMessage* get_updates = message->mutable_get_updates();
-
-  // We want folders for our associated types, always.  If we were to set
-  // this to false, the server would send just the non-container items
-  // (e.g. Bookmark URLs but not their containing folders).
-  get_updates->set_fetch_folders(true);
-
-  get_updates->set_create_mobile_bookmarks_folder(
-      create_mobile_bookmarks_folder);
-  bool need_encryption_key = ShouldRequestEncryptionKey(session->context());
-  get_updates->set_need_encryption_key(need_encryption_key);
-
-  // Set legacy GetUpdatesMessage.GetUpdatesCallerInfo information.
-  get_updates->mutable_caller_info()->set_notifications_enabled(
-      session->context()->notifications_enabled());
-}
-
-SyncerError ExecuteDownloadUpdates(
-    ModelTypeSet request_types,
-    SyncSession* session,
-    GetUpdatesProcessor* get_updates_processor,
-    sync_pb::ClientToServerMessage* msg) {
-  sync_pb::ClientToServerResponse update_response;
-  StatusController* status = session->mutable_status_controller();
-  bool need_encryption_key = ShouldRequestEncryptionKey(session->context());
-
-  if (session->context()->debug_info_getter()) {
-    sync_pb::DebugInfo* debug_info = msg->mutable_debug_info();
-    CopyClientDebugInfo(session->context()->debug_info_getter(), debug_info);
-  }
-
-  SyncerError result = SyncerProtoUtil::PostClientToServerMessage(
-      msg,
-      &update_response,
-      session);
-
-  DVLOG(2) << SyncerProtoUtil::ClientToServerResponseDebugString(
-      update_response);
-
-  if (result != SYNCER_OK) {
-    LOG(ERROR) << "PostClientToServerMessage() failed during GetUpdates";
-    return result;
-  }
-
-  DVLOG(1) << "GetUpdates "
-           << " returned " << update_response.get_updates().entries_size()
-           << " updates and indicated "
-           << update_response.get_updates().changes_remaining()
-           << " updates left on server.";
-
-  if (session->context()->debug_info_getter()) {
-    // Clear debug info now that we have successfully sent it to the server.
-    DVLOG(1) << "Clearing client debug info.";
-    session->context()->debug_info_getter()->ClearDebugInfo();
-  }
-
-  if (need_encryption_key ||
-      update_response.get_updates().encryption_keys_size() > 0) {
-    syncable::Directory* dir = session->context()->directory();
-    status->set_last_get_key_result(
-        HandleGetEncryptionKeyResponse(update_response, dir));
-  }
-
-  return ProcessResponse(update_response.get_updates(),
-                         request_types,
-                         get_updates_processor,
-                         status);
-}
-
-SyncerError ProcessResponse(
-    const sync_pb::GetUpdatesResponse& gu_response,
-    ModelTypeSet request_types,
-    GetUpdatesProcessor* get_updates_processor,
-    StatusController* status) {
-  status->increment_num_updates_downloaded_by(gu_response.entries_size());
-
-  // The changes remaining field is used to prevent the client from looping.  If
-  // that field is being set incorrectly, we're in big trouble.
-  if (!gu_response.has_changes_remaining()) {
-    return SERVER_RESPONSE_VALIDATION_FAILED;
-  }
-  status->set_num_server_changes_remaining(gu_response.changes_remaining());
-
-
-  if (!get_updates_processor->ProcessGetUpdatesResponse(request_types,
-                                                        gu_response,
-                                                        status)) {
-    return SERVER_RESPONSE_VALIDATION_FAILED;
-  }
-
-  if (gu_response.changes_remaining() == 0) {
-    return SYNCER_OK;
-  } else {
-    return SERVER_MORE_TO_DOWNLOAD;
-  }
-}
-
-void CopyClientDebugInfo(
-    sessions::DebugInfoGetter* debug_info_getter,
-    sync_pb::DebugInfo* debug_info) {
-  DVLOG(1) << "Copying client debug info to send.";
-  debug_info_getter->GetDebugInfo(debug_info);
-}
-
-}  // namespace download
-
-}  // namespace syncer
diff --git a/sync/engine/download.h b/sync/engine/download.h
deleted file mode 100644
index 0eeca97..0000000
--- a/sync/engine/download.h
+++ /dev/null
@@ -1,60 +0,0 @@
-// Copyright 2013 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_DOWNLOAD_H_
-#define SYNC_ENGINE_DOWNLOAD_H_
-
-#include "sync/base/sync_export.h"
-#include "sync/engine/get_updates_processor.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"
-
-namespace sync_pb {
-class DebugInfo;
-}  // namespace sync_pb
-
-namespace syncer {
-
-namespace sessions {
-class DebugInfoGetter;
-class StatusController;
-class SyncSession;
-}  // namespace sessions
-
-namespace download {
-
-// Generic initialization of a GetUpdates message.
-SYNC_EXPORT_PRIVATE void InitDownloadUpdatesContext(
-    sessions::SyncSession* session,
-    bool create_mobile_bookmarks_folder,
-    sync_pb::ClientToServerMessage* message);
-
-// Sends the specified message to the server and stores the response in a member
-// of the |session|'s StatusController.
-SYNC_EXPORT_PRIVATE SyncerError
-    ExecuteDownloadUpdates(ModelTypeSet request_types,
-                           sessions::SyncSession* session,
-                           GetUpdatesProcessor* get_updates_processor,
-                           sync_pb::ClientToServerMessage* msg);
-
-// Helper function for processing responses from the server.
-// Defined here for testing.
-SYNC_EXPORT_PRIVATE SyncerError ProcessResponse(
-    const sync_pb::GetUpdatesResponse& gu_response,
-    ModelTypeSet proto_request_types,
-    GetUpdatesProcessor* get_updates_processor,
-    sessions::StatusController* status);
-
-// Helper function to copy client debug info from debug_info_getter to
-// debug_info.  Defined here for testing.
-SYNC_EXPORT_PRIVATE void CopyClientDebugInfo(
-    sessions::DebugInfoGetter* debug_info_getter,
-    sync_pb::DebugInfo* debug_info);
-
-}  // namespace download
-
-}  // namespace syncer
-
-#endif  // SYNC_ENGINE_DOWNLOAD_H_
diff --git a/sync/engine/get_updates_processor.cc b/sync/engine/get_updates_processor.cc
index 449f718..2ffc4b3 100644
--- a/sync/engine/get_updates_processor.cc
+++ b/sync/engine/get_updates_processor.cc
@@ -6,9 +6,16 @@
 
 #include <map>
 
+#include "base/debug/trace_event.h"
 #include "sync/engine/get_updates_delegate.h"
+#include "sync/engine/syncer_proto_util.h"
 #include "sync/engine/update_handler.h"
 #include "sync/protocol/sync.pb.h"
+#include "sync/sessions/status_controller.h"
+#include "sync/sessions/sync_session.h"
+#include "sync/syncable/directory.h"
+#include "sync/syncable/nigori_handler.h"
+#include "sync/syncable/syncable_read_transaction.h"
 
 typedef std::vector<const sync_pb::SyncEntity*> SyncEntityList;
 typedef std::map<syncer::ModelType, SyncEntityList> TypeSyncEntityMap;
@@ -19,6 +26,35 @@ typedef std::map<ModelType, size_t> TypeToIndexMap;
 
 namespace {
 
+bool ShouldRequestEncryptionKey(sessions::SyncSessionContext* context) {
+  syncable::Directory* dir = context->directory();
+  syncable::ReadTransaction trans(FROM_HERE, dir);
+  syncable::NigoriHandler* nigori_handler = dir->GetNigoriHandler();
+  return nigori_handler->NeedKeystoreKey(&trans);
+}
+
+
+SyncerError HandleGetEncryptionKeyResponse(
+    const sync_pb::ClientToServerResponse& update_response,
+    syncable::Directory* dir) {
+  bool success = false;
+  if (update_response.get_updates().encryption_keys_size() == 0) {
+    LOG(ERROR) << "Failed to receive encryption key from server.";
+    return SERVER_RESPONSE_VALIDATION_FAILED;
+  }
+  syncable::ReadTransaction trans(FROM_HERE, dir);
+  syncable::NigoriHandler* nigori_handler = dir->GetNigoriHandler();
+  success = nigori_handler->SetKeystoreKeys(
+      update_response.get_updates().encryption_keys(),
+      &trans);
+
+  DVLOG(1) << "GetUpdates returned "
+           << update_response.get_updates().encryption_keys_size()
+           << "encryption keys. Nigori keystore key "
+           << (success ? "" : "not ") << "updated.";
+  return (success ? SYNCER_OK : SERVER_RESPONSE_VALIDATION_FAILED);
+}
+
 // Given a GetUpdates response, iterates over all the returned items and
 // divides them according to their type.  Outputs a map from model types to
 // received SyncEntities.  The output map will have entries (possibly empty)
@@ -74,6 +110,34 @@ void PartitionProgressMarkersByType(
   }
 }
 
+// Initializes the parts of the GetUpdatesMessage that depend on shared state,
+// like the ShouldRequestEncryptionKey() status.  This is kept separate from the
+// other of the message-building functions to make the rest of the code easier
+// to test.
+void InitDownloadUpdatesContext(
+    sessions::SyncSession* session,
+    bool create_mobile_bookmarks_folder,
+    sync_pb::ClientToServerMessage* message) {
+  message->set_share(session->context()->account_name());
+  message->set_message_contents(sync_pb::ClientToServerMessage::GET_UPDATES);
+
+  sync_pb::GetUpdatesMessage* get_updates = message->mutable_get_updates();
+
+  // We want folders for our associated types, always.  If we were to set
+  // this to false, the server would send just the non-container items
+  // (e.g. Bookmark URLs but not their containing folders).
+  get_updates->set_fetch_folders(true);
+
+  get_updates->set_create_mobile_bookmarks_folder(
+      create_mobile_bookmarks_folder);
+  bool need_encryption_key = ShouldRequestEncryptionKey(session->context());
+  get_updates->set_need_encryption_key(need_encryption_key);
+
+  // Set legacy GetUpdatesMessage.GetUpdatesCallerInfo information.
+  get_updates->mutable_caller_info()->set_notifications_enabled(
+      session->context()->notifications_enabled());
+}
+
 }  // namespace
 
 GetUpdatesProcessor::GetUpdatesProcessor(UpdateHandlerMap* update_handler_map,
@@ -82,9 +146,29 @@ GetUpdatesProcessor::GetUpdatesProcessor(UpdateHandlerMap* update_handler_map,
 
 GetUpdatesProcessor::~GetUpdatesProcessor() {}
 
+SyncerError GetUpdatesProcessor::DownloadUpdates(
+    ModelTypeSet request_types,
+    sessions::SyncSession* session,
+    bool create_mobile_bookmarks_folder) {
+  TRACE_EVENT0("sync", "DownloadUpdates");
+
+  sync_pb::ClientToServerMessage message;
+  InitDownloadUpdatesContext(session,
+                             create_mobile_bookmarks_folder,
+                             &message);
+  PrepareGetUpdates(request_types, &message);
+
+  SyncerError result = ExecuteDownloadUpdates(request_types, session, &message);
+  session->mutable_status_controller()->set_last_download_updates_result(
+      result);
+  return result;
+}
+
 void GetUpdatesProcessor::PrepareGetUpdates(
     ModelTypeSet gu_types,
-    sync_pb::GetUpdatesMessage* get_updates) {
+    sync_pb::ClientToServerMessage* message) {
+  sync_pb::GetUpdatesMessage* get_updates = message->mutable_get_updates();
+
   for (ModelTypeSet::Iterator it = gu_types.First(); it.Good(); it.Inc()) {
     UpdateHandlerMap::iterator handler_it = update_handler_map_->find(it.Get());
     DCHECK(handler_it != update_handler_map_->end());
@@ -95,6 +179,80 @@ void GetUpdatesProcessor::PrepareGetUpdates(
   delegate_.HelpPopulateGuMessage(get_updates);
 }
 
+SyncerError GetUpdatesProcessor::ExecuteDownloadUpdates(
+    ModelTypeSet request_types,
+    sessions::SyncSession* session,
+    sync_pb::ClientToServerMessage* msg) {
+  sync_pb::ClientToServerResponse update_response;
+  sessions::StatusController* status = session->mutable_status_controller();
+  bool need_encryption_key = ShouldRequestEncryptionKey(session->context());
+
+  if (session->context()->debug_info_getter()) {
+    sync_pb::DebugInfo* debug_info = msg->mutable_debug_info();
+    CopyClientDebugInfo(session->context()->debug_info_getter(), debug_info);
+  }
+
+  SyncerError result = SyncerProtoUtil::PostClientToServerMessage(
+      msg,
+      &update_response,
+      session);
+
+  DVLOG(2) << SyncerProtoUtil::ClientToServerResponseDebugString(
+      update_response);
+
+  if (result != SYNCER_OK) {
+    LOG(ERROR) << "PostClientToServerMessage() failed during GetUpdates";
+    return result;
+  }
+
+  DVLOG(1) << "GetUpdates "
+           << " returned " << update_response.get_updates().entries_size()
+           << " updates and indicated "
+           << update_response.get_updates().changes_remaining()
+           << " updates left on server.";
+
+  if (session->context()->debug_info_getter()) {
+    // Clear debug info now that we have successfully sent it to the server.
+    DVLOG(1) << "Clearing client debug info.";
+    session->context()->debug_info_getter()->ClearDebugInfo();
+  }
+
+  if (need_encryption_key ||
+      update_response.get_updates().encryption_keys_size() > 0) {
+    syncable::Directory* dir = session->context()->directory();
+    status->set_last_get_key_result(
+        HandleGetEncryptionKeyResponse(update_response, dir));
+  }
+
+  return ProcessResponse(update_response.get_updates(),
+                         request_types,
+                         status);
+}
+
+SyncerError GetUpdatesProcessor::ProcessResponse(
+    const sync_pb::GetUpdatesResponse& gu_response,
+    ModelTypeSet request_types,
+    sessions::StatusController* status) {
+  status->increment_num_updates_downloaded_by(gu_response.entries_size());
+
+  // The changes remaining field is used to prevent the client from looping.  If
+  // that field is being set incorrectly, we're in big trouble.
+  if (!gu_response.has_changes_remaining()) {
+    return SERVER_RESPONSE_VALIDATION_FAILED;
+  }
+  status->set_num_server_changes_remaining(gu_response.changes_remaining());
+
+  if (!ProcessGetUpdatesResponse(request_types, gu_response, status)) {
+    return SERVER_RESPONSE_VALIDATION_FAILED;
+  }
+
+  if (gu_response.changes_remaining() == 0) {
+    return SYNCER_OK;
+  } else {
+    return SERVER_MORE_TO_DOWNLOAD;
+  }
+}
+
 bool GetUpdatesProcessor::ProcessGetUpdatesResponse(
     ModelTypeSet gu_types,
     const sync_pb::GetUpdatesResponse& gu_response,
@@ -148,4 +306,11 @@ void GetUpdatesProcessor::ApplyUpdates(
   delegate_.ApplyUpdates(status_controller, update_handler_map_);
 }
 
+void GetUpdatesProcessor::CopyClientDebugInfo(
+    sessions::DebugInfoGetter* debug_info_getter,
+    sync_pb::DebugInfo* debug_info) {
+  DVLOG(1) << "Copying client debug info to send.";
+  debug_info_getter->GetDebugInfo(debug_info);
+}
+
 }  // namespace syncer
diff --git a/sync/engine/get_updates_processor.h b/sync/engine/get_updates_processor.h
index 13a9c83..787f267 100644
--- a/sync/engine/get_updates_processor.h
+++ b/sync/engine/get_updates_processor.h
@@ -9,9 +9,11 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/gtest_prod_util.h"
 #include "sync/base/sync_export.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/engine/model_safe_worker.h"
+#include "sync/protocol/sync.pb.h"
 #include "sync/sessions/model_type_registry.h"
 
 namespace sync_pb {
@@ -23,6 +25,9 @@ namespace syncer {
 
 namespace sessions {
 class StatusController;
+class SyncSession;
+class SyncSessionContext;
+class DebugInfoGetter;
 }  // namespace sessions
 
 namespace syncable {
@@ -43,9 +48,36 @@ class SYNC_EXPORT_PRIVATE GetUpdatesProcessor {
                                const GetUpdatesDelegate& delegate);
   ~GetUpdatesProcessor();
 
+  // Downloads and processes a batch of updates for the specified types.
+  //
+  // Returns SYNCER_OK if the download succeeds, SERVER_MORE_TO_DOWNLOAD if the
+  // download succeeded but there are still some updates left to fetch on the
+  // server, or an appropriate error value in case of failure.
+  SyncerError DownloadUpdates(
+      ModelTypeSet request_types,
+      sessions::SyncSession* session,
+      bool create_mobile_bookmarks_folder);
+
+  // Applies any downloaded and processed updates.
+  void ApplyUpdates(sessions::StatusController* status_controller);
+
+ private:
   // Populates a GetUpdates request message with per-type information.
-  void PrepareGetUpdates(ModelTypeSet gu_types,
-                         sync_pb::GetUpdatesMessage* get_updates);
+  void PrepareGetUpdates(
+      ModelTypeSet gu_types,
+      sync_pb::ClientToServerMessage* message);
+
+  // Sends the specified message to the server and stores the response in a
+  // member of the |session|'s StatusController.
+  SyncerError ExecuteDownloadUpdates(ModelTypeSet request_types,
+                                     sessions::SyncSession* session,
+                                     sync_pb::ClientToServerMessage* msg);
+
+  // Helper function for processing responses from the server.  Defined here for
+  // testing.
+  SyncerError ProcessResponse(const sync_pb::GetUpdatesResponse& gu_response,
+                              ModelTypeSet proto_request_types,
+                              sessions::StatusController* status);
 
   // Processes a GetUpdates responses for each type.
   bool ProcessGetUpdatesResponse(
@@ -53,10 +85,23 @@ class SYNC_EXPORT_PRIVATE GetUpdatesProcessor {
       const sync_pb::GetUpdatesResponse& gu_response,
       sessions::StatusController* status_controller);
 
-  // Hands off control to the delegate so it can apply updates.
-  void ApplyUpdates(sessions::StatusController* status_controller);
+  static void CopyClientDebugInfo(
+      sessions::DebugInfoGetter* debug_info_getter,
+      sync_pb::DebugInfo* debug_info);
+
+  FRIEND_TEST_ALL_PREFIXES(GetUpdatesProcessorTest, BookmarkNudge);
+  FRIEND_TEST_ALL_PREFIXES(GetUpdatesProcessorTest, NotifyMany);
+  FRIEND_TEST_ALL_PREFIXES(GetUpdatesProcessorTest, ConfigureTest);
+  FRIEND_TEST_ALL_PREFIXES(GetUpdatesProcessorTest, PollTest);
+  FRIEND_TEST_ALL_PREFIXES(GetUpdatesProcessorTest, RetryTest);
+  FRIEND_TEST_ALL_PREFIXES(GetUpdatesProcessorTest, NudgeWithRetryTest);
+  FRIEND_TEST_ALL_PREFIXES(GetUpdatesProcessorTest, InvalidResponse);
+  FRIEND_TEST_ALL_PREFIXES(GetUpdatesProcessorTest, MoreToDownloadResponse);
+  FRIEND_TEST_ALL_PREFIXES(GetUpdatesProcessorTest, NormalResponseTest);
+  FRIEND_TEST_ALL_PREFIXES(DownloadUpdatesDebugInfoTest,
+                           VerifyCopyClientDebugInfo_Empty);
+  FRIEND_TEST_ALL_PREFIXES(DownloadUpdatesDebugInfoTest, VerifyCopyOverwrites);
 
- private:
   // A map of 'update handlers', one for each enabled type.
   // This must be kept in sync with the routing info.  Our temporary solution to
   // that problem is to initialize this map in set_routing_info().
diff --git a/sync/engine/download_unittest.cc b/sync/engine/get_updates_processor_unittest.cc
index e114022..f8fd274 100644
--- a/sync/engine/download_unittest.cc
+++ b/sync/engine/get_updates_processor_unittest.cc
@@ -1,8 +1,8 @@
-// Copyright 2013 The Chromium Authors. All rights reserved.
+// Copyright 2014 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/download.h"
+#include "sync/engine/get_updates_processor.h"
 
 #include "base/message_loop/message_loop.h"
 #include "base/stl_util.h"
@@ -23,9 +23,9 @@ namespace syncer {
 using sessions::MockDebugInfoGetter;
 
 // A test fixture for tests exercising download updates functions.
-class DownloadUpdatesTest : public ::testing::Test {
+class GetUpdatesProcessorTest : public ::testing::Test {
  protected:
-  DownloadUpdatesTest() :
+  GetUpdatesProcessorTest() :
     kTestStartTime(base::TimeTicks::Now()),
     update_handler_deleter_(&update_handler_map_) {}
 
@@ -73,20 +73,21 @@ class DownloadUpdatesTest : public ::testing::Test {
   STLValueDeleter<UpdateHandlerMap> update_handler_deleter_;
   scoped_ptr<GetUpdatesProcessor> get_updates_processor_;
 
-  DISALLOW_COPY_AND_ASSIGN(DownloadUpdatesTest);
+  DISALLOW_COPY_AND_ASSIGN(GetUpdatesProcessorTest);
 };
 
 // Basic test to make sure nudges are expressed properly in the request.
-TEST_F(DownloadUpdatesTest, BookmarkNudge) {
+TEST_F(GetUpdatesProcessorTest, BookmarkNudge) {
   sessions::NudgeTracker nudge_tracker;
   nudge_tracker.RecordLocalChange(ModelTypeSet(BOOKMARKS));
 
-  sync_pb::GetUpdatesMessage gu_msg;
+  sync_pb::ClientToServerMessage message;
   NormalGetUpdatesDelegate normal_delegate(nudge_tracker);
   scoped_ptr<GetUpdatesProcessor> processor(
       BuildGetUpdatesProcessor(normal_delegate));
-  processor->PrepareGetUpdates(request_types(), &gu_msg);
+  processor->PrepareGetUpdates(request_types(), &message);
 
+  const sync_pb::GetUpdatesMessage& gu_msg = message.get_updates();
   EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::LOCAL,
             gu_msg.caller_info().source());
   EXPECT_EQ(sync_pb::SyncEnums::GU_TRIGGER, gu_msg.get_updates_origin());
@@ -115,7 +116,7 @@ TEST_F(DownloadUpdatesTest, BookmarkNudge) {
 }
 
 // Basic test to ensure invalidation payloads are expressed in the request.
-TEST_F(DownloadUpdatesTest, NotifyMany) {
+TEST_F(GetUpdatesProcessorTest, NotifyMany) {
   sessions::NudgeTracker nudge_tracker;
   nudge_tracker.RecordRemoteInvalidation(
       BuildInvalidationMap(AUTOFILL, 1, "autofill_payload"));
@@ -128,12 +129,13 @@ TEST_F(DownloadUpdatesTest, NotifyMany) {
   notified_types.Put(BOOKMARKS);
   notified_types.Put(PREFERENCES);
 
-  sync_pb::GetUpdatesMessage gu_msg;
+  sync_pb::ClientToServerMessage message;
   NormalGetUpdatesDelegate normal_delegate(nudge_tracker);
   scoped_ptr<GetUpdatesProcessor> processor(
       BuildGetUpdatesProcessor(normal_delegate));
-  processor->PrepareGetUpdates(request_types(), &gu_msg);
+  processor->PrepareGetUpdates(request_types(), &message);
 
+  const sync_pb::GetUpdatesMessage& gu_msg = message.get_updates();
   EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::NOTIFICATION,
             gu_msg.caller_info().source());
   EXPECT_EQ(sync_pb::SyncEnums::GU_TRIGGER, gu_msg.get_updates_origin());
@@ -159,14 +161,15 @@ TEST_F(DownloadUpdatesTest, NotifyMany) {
   }
 }
 
-TEST_F(DownloadUpdatesTest, ConfigureTest) {
-  sync_pb::GetUpdatesMessage gu_msg;
+TEST_F(GetUpdatesProcessorTest, ConfigureTest) {
+  sync_pb::ClientToServerMessage message;
   ConfigureGetUpdatesDelegate configure_delegate(
       sync_pb::GetUpdatesCallerInfo::RECONFIGURATION);
   scoped_ptr<GetUpdatesProcessor> processor(
       BuildGetUpdatesProcessor(configure_delegate));
-  processor->PrepareGetUpdates(request_types(), &gu_msg);
+  processor->PrepareGetUpdates(request_types(), &message);
 
+  const sync_pb::GetUpdatesMessage& gu_msg = message.get_updates();
   EXPECT_EQ(sync_pb::SyncEnums::RECONFIGURATION, gu_msg.get_updates_origin());
   EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::RECONFIGURATION,
             gu_msg.caller_info().source());
@@ -180,13 +183,14 @@ TEST_F(DownloadUpdatesTest, ConfigureTest) {
   EXPECT_TRUE(request_types().Equals(progress_types));
 }
 
-TEST_F(DownloadUpdatesTest, PollTest) {
-  sync_pb::GetUpdatesMessage gu_msg;
+TEST_F(GetUpdatesProcessorTest, PollTest) {
+  sync_pb::ClientToServerMessage message;
   PollGetUpdatesDelegate poll_delegate;
   scoped_ptr<GetUpdatesProcessor> processor(
       BuildGetUpdatesProcessor(poll_delegate));
-  processor->PrepareGetUpdates(request_types(), &gu_msg);
+  processor->PrepareGetUpdates(request_types(), &message);
 
+  const sync_pb::GetUpdatesMessage& gu_msg = message.get_updates();
   EXPECT_EQ(sync_pb::SyncEnums::PERIODIC, gu_msg.get_updates_origin());
   EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::PERIODIC,
             gu_msg.caller_info().source());
@@ -200,7 +204,7 @@ TEST_F(DownloadUpdatesTest, PollTest) {
   EXPECT_TRUE(request_types().Equals(progress_types));
 }
 
-TEST_F(DownloadUpdatesTest, RetryTest) {
+TEST_F(GetUpdatesProcessorTest, RetryTest) {
   sessions::NudgeTracker nudge_tracker;
 
   // Schedule a retry.
@@ -210,12 +214,13 @@ TEST_F(DownloadUpdatesTest, RetryTest) {
   // Get the nudge tracker to think the retry is due.
   nudge_tracker.SetSyncCycleStartTime(t1 + base::TimeDelta::FromSeconds(1));
 
-  sync_pb::GetUpdatesMessage gu_msg;
+  sync_pb::ClientToServerMessage message;
   RetryGetUpdatesDelegate retry_delegate;
   scoped_ptr<GetUpdatesProcessor> processor(
       BuildGetUpdatesProcessor(retry_delegate));
-  processor->PrepareGetUpdates(request_types(), &gu_msg);
+  processor->PrepareGetUpdates(request_types(), &message);
 
+  const sync_pb::GetUpdatesMessage& gu_msg = message.get_updates();
   EXPECT_EQ(sync_pb::SyncEnums::RETRY, gu_msg.get_updates_origin());
   EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::RETRY,
             gu_msg.caller_info().source());
@@ -230,7 +235,7 @@ TEST_F(DownloadUpdatesTest, RetryTest) {
   EXPECT_TRUE(request_types().Equals(progress_types));
 }
 
-TEST_F(DownloadUpdatesTest, NudgeWithRetryTest) {
+TEST_F(GetUpdatesProcessorTest, NudgeWithRetryTest) {
   sessions::NudgeTracker nudge_tracker;
 
   // Schedule a retry.
@@ -243,12 +248,13 @@ TEST_F(DownloadUpdatesTest, NudgeWithRetryTest) {
   // Record a local change, too.
   nudge_tracker.RecordLocalChange(ModelTypeSet(BOOKMARKS));
 
-  sync_pb::GetUpdatesMessage gu_msg;
+  sync_pb::ClientToServerMessage message;
   NormalGetUpdatesDelegate normal_delegate(nudge_tracker);
   scoped_ptr<GetUpdatesProcessor> processor(
       BuildGetUpdatesProcessor(normal_delegate));
-  processor->PrepareGetUpdates(request_types(), &gu_msg);
+  processor->PrepareGetUpdates(request_types(), &message);
 
+  const sync_pb::GetUpdatesMessage& gu_msg = message.get_updates();
   EXPECT_NE(sync_pb::SyncEnums::RETRY, gu_msg.get_updates_origin());
   EXPECT_NE(sync_pb::GetUpdatesCallerInfo::RETRY,
             gu_msg.caller_info().source());
@@ -257,7 +263,7 @@ TEST_F(DownloadUpdatesTest, NudgeWithRetryTest) {
 }
 
 // Verify that a bogus response message is detected.
-TEST_F(DownloadUpdatesTest, InvalidResponse) {
+TEST_F(GetUpdatesProcessorTest, InvalidResponse) {
   sync_pb::GetUpdatesResponse gu_response;
   InitFakeUpdateResponse(&gu_response);
 
@@ -270,15 +276,14 @@ TEST_F(DownloadUpdatesTest, InvalidResponse) {
   sessions::StatusController status;
   scoped_ptr<GetUpdatesProcessor> processor(
       BuildGetUpdatesProcessor(normal_delegate));
-  SyncerError error = download::ProcessResponse(gu_response,
-                                                request_types(),
-                                                processor.get(),
-                                                &status);
+  SyncerError error = processor->ProcessResponse(gu_response,
+                                                 request_types(),
+                                                 &status);
   EXPECT_EQ(error, SERVER_RESPONSE_VALIDATION_FAILED);
 }
 
 // Verify that we correctly detect when there's more work to be done.
-TEST_F(DownloadUpdatesTest, MoreToDownloadResponse) {
+TEST_F(GetUpdatesProcessorTest, MoreToDownloadResponse) {
   sync_pb::GetUpdatesResponse gu_response;
   InitFakeUpdateResponse(&gu_response);
   gu_response.set_changes_remaining(1);
@@ -288,15 +293,14 @@ TEST_F(DownloadUpdatesTest, MoreToDownloadResponse) {
   sessions::StatusController status;
   scoped_ptr<GetUpdatesProcessor> processor(
       BuildGetUpdatesProcessor(normal_delegate));
-  SyncerError error = download::ProcessResponse(gu_response,
-                                                request_types(),
-                                                processor.get(),
-                                                &status);
+  SyncerError error = processor->ProcessResponse(gu_response,
+                                                 request_types(),
+                                                 &status);
   EXPECT_EQ(error, SERVER_MORE_TO_DOWNLOAD);
 }
 
 // A simple scenario: No updates returned and nothing more to download.
-TEST_F(DownloadUpdatesTest, NormalResponseTest) {
+TEST_F(GetUpdatesProcessorTest, NormalResponseTest) {
   sync_pb::GetUpdatesResponse gu_response;
   InitFakeUpdateResponse(&gu_response);
   gu_response.set_changes_remaining(0);
@@ -306,10 +310,9 @@ TEST_F(DownloadUpdatesTest, NormalResponseTest) {
   sessions::StatusController status;
   scoped_ptr<GetUpdatesProcessor> processor(
       BuildGetUpdatesProcessor(normal_delegate));
-  SyncerError error = download::ProcessResponse(gu_response,
-                                                request_types(),
-                                                processor.get(),
-                                                &status);
+  SyncerError error = processor->ProcessResponse(gu_response,
+                                                 request_types(),
+                                                 &status);
   EXPECT_EQ(error, SYNCER_OK);
 }
 
@@ -339,16 +342,16 @@ class DownloadUpdatesDebugInfoTest : public ::testing::Test {
 // Verify CopyClientDebugInfo when there are no events to upload.
 TEST_F(DownloadUpdatesDebugInfoTest, VerifyCopyClientDebugInfo_Empty) {
   sync_pb::DebugInfo debug_info;
-  download::CopyClientDebugInfo(debug_info_getter(), &debug_info);
+  GetUpdatesProcessor::CopyClientDebugInfo(debug_info_getter(), &debug_info);
   EXPECT_EQ(0, debug_info.events_size());
 }
 
 TEST_F(DownloadUpdatesDebugInfoTest, VerifyCopyOverwrites) {
   sync_pb::DebugInfo debug_info;
   AddDebugEvent();
-  download::CopyClientDebugInfo(debug_info_getter(), &debug_info);
+  GetUpdatesProcessor::CopyClientDebugInfo(debug_info_getter(), &debug_info);
   EXPECT_EQ(1, debug_info.events_size());
-  download::CopyClientDebugInfo(debug_info_getter(), &debug_info);
+  GetUpdatesProcessor::CopyClientDebugInfo(debug_info_getter(), &debug_info);
   EXPECT_EQ(1, debug_info.events_size());
 }
 
diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc
index 21282de..240332c 100644
--- a/sync/engine/syncer.cc
+++ b/sync/engine/syncer.cc
@@ -14,7 +14,6 @@
 #include "sync/engine/commit.h"
 #include "sync/engine/commit_processor.h"
 #include "sync/engine/conflict_resolver.h"
-#include "sync/engine/download.h"
 #include "sync/engine/get_updates_delegate.h"
 #include "sync/engine/get_updates_processor.h"
 #include "sync/engine/net/server_connection_manager.h"
@@ -142,20 +141,10 @@ bool Syncer::DownloadAndApplyUpdates(
     bool create_mobile_bookmarks_folder) {
   SyncerError download_result = UNSET;
   do {
-    TRACE_EVENT0("sync", "DownloadUpdates");
-    sync_pb::ClientToServerMessage msg;
-    sync_pb::GetUpdatesMessage* gu_msg = msg.mutable_get_updates();
-
-    download::InitDownloadUpdatesContext(
-        session, create_mobile_bookmarks_folder, &msg);
-    get_updates_processor->PrepareGetUpdates(request_types, gu_msg);
-
-    download_result = download::ExecuteDownloadUpdates(request_types,
-                                                       session,
-                                                       get_updates_processor,
-                                                       &msg);
-    session->mutable_status_controller()->set_last_download_updates_result(
-        download_result);
+    download_result = get_updates_processor->DownloadUpdates(
+        request_types,
+        session,
+        create_mobile_bookmarks_folder);
   } while (download_result == SERVER_MORE_TO_DOWNLOAD);
 
   // Exit without applying if we're shutting down or an error was detected.
diff --git a/sync/sync_core.gypi b/sync/sync_core.gypi
index 5aa74f6..e973110 100644
--- a/sync/sync_core.gypi
+++ b/sync/sync_core.gypi
@@ -54,8 +54,6 @@
     'engine/directory_commit_contributor.h',
     'engine/directory_update_handler.cc',
     'engine/directory_update_handler.h',
-    'engine/download.cc',
-    'engine/download.h',
     'engine/get_commit_ids.cc',
     'engine/get_commit_ids.h',
     'engine/get_updates_delegate.cc',
diff --git a/sync/sync_tests.gypi b/sync/sync_tests.gypi
index e379107..634a741 100644
--- a/sync/sync_tests.gypi
+++ b/sync/sync_tests.gypi
@@ -274,9 +274,9 @@
           'internal_api/public/util/weak_handle_unittest.cc',
           'engine/apply_control_data_updates_unittest.cc',
           'engine/backoff_delay_provider_unittest.cc',
-          'engine/download_unittest.cc',
           'engine/directory_commit_contribution_unittest.cc',
           'engine/directory_update_handler_unittest.cc',
+          'engine/get_updates_processor_unittest.cc',
           'engine/sync_scheduler_unittest.cc',
           'engine/syncer_proto_util_unittest.cc',
           'engine/syncer_unittest.cc',