Replace deprecated 3-arg SyncData::CreateLocalData with 5-arg version.

Update unit tests that call SyncData::CreateLocalData to call the new
version.

Add AttachmentServiceProxyForTest to reduce boilerplate in tests.

Add a MessageLoop to ManagedUserSyncServiceTest, DomDistillerStoreTest,
and SyncChangeTest because AttachmentServiceProxyForTest needs one.

Format modified lines with clang-format.

TBR=pam,droger,asargent,pkasting,pkotwicz,cjhopman

BUG=353296

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

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

chrome/browser/extensions/api/sessions/sessions_apitest.cc

@@ -21,6 +21,8 @@
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/test_switches.h"
 #include "chrome/test/base/testing_browser_process.h"
+#include "sync/api/attachments/attachment_id.h"
+#include "sync/api/attachments/attachment_service_proxy_for_test.h"
 #include "sync/api/fake_sync_change_processor.h"
 #include "sync/api/sync_error_factory_mock.h"
 
@@ -182,13 +184,21 @@ void ExtensionSessionsTest::CreateSessionModels() {
             switches::kDisableSyncSessionsV2)) {
       sync_pb::EntitySpecifics entity;
       entity.mutable_session()->CopyFrom(meta);
-      initial_data.push_back(
-          syncer::SyncData::CreateRemoteData(1, entity, base::Time()));
+      initial_data.push_back(syncer::SyncData::CreateRemoteData(
+          1,
+          entity,
+          base::Time(),
+          syncer::AttachmentIdList(),
+          syncer::AttachmentServiceProxyForTest::Create()));
       for (size_t i = 0; i < tabs1.size(); i++) {
         sync_pb::EntitySpecifics entity;
         entity.mutable_session()->CopyFrom(tabs1[i]);
         initial_data.push_back(syncer::SyncData::CreateRemoteData(
-            i + 2, entity, base::Time()));
+            i + 2,
+            entity,
+            base::Time(),
+            syncer::AttachmentIdList(),
+            syncer::AttachmentServiceProxyForTest::Create()));
       }
     } else {
       // Update associator with the session's meta node containing one window.

chrome/browser/history/history_unittest.cc

@@ -60,6 +60,8 @@
 #include "content/public/browser/notification_source.h"
 #include "sql/connection.h"
 #include "sql/statement.h"
+#include "sync/api/attachments/attachment_id.h"
+#include "sync/api/attachments/attachment_service_proxy_for_test.h"
 #include "sync/api/fake_sync_change_processor.h"
 #include "sync/api/sync_change.h"
 #include "sync/api/sync_change_processor.h"
@@ -1633,8 +1635,12 @@ TEST_F(HistoryTest, ProcessGlobalIdDeleteDirective) {
       .ToInternalValue());
   global_id_directive->set_start_time_usec(3);
   global_id_directive->set_end_time_usec(10);
-  directives.push_back(
-      syncer::SyncData::CreateRemoteData(1, entity_specs, base::Time()));
+  directives.push_back(syncer::SyncData::CreateRemoteData(
+      1,
+      entity_specs,
+      base::Time(),
+      syncer::AttachmentIdList(),
+      syncer::AttachmentServiceProxyForTest::Create()));
 
   // 2nd directive.
   global_id_directive->Clear();
@@ -1643,8 +1649,12 @@ TEST_F(HistoryTest, ProcessGlobalIdDeleteDirective) {
       .ToInternalValue());
   global_id_directive->set_start_time_usec(13);
   global_id_directive->set_end_time_usec(19);
-  directives.push_back(
-      syncer::SyncData::CreateRemoteData(2, entity_specs, base::Time()));
+  directives.push_back(syncer::SyncData::CreateRemoteData(
+      2,
+      entity_specs,
+      base::Time(),
+      syncer::AttachmentIdList(),
+      syncer::AttachmentServiceProxyForTest::Create()));
 
   syncer::FakeSyncChangeProcessor change_processor;
   EXPECT_FALSE(
@@ -1713,17 +1723,23 @@ TEST_F(HistoryTest, ProcessTimeRangeDeleteDirective) {
           ->mutable_time_range_directive();
   time_range_directive->set_start_time_usec(2);
   time_range_directive->set_end_time_usec(5);
-  directives.push_back(syncer::SyncData::CreateRemoteData(1,
-                                                          entity_specs,
-                                                          base::Time()));
+  directives.push_back(syncer::SyncData::CreateRemoteData(
+      1,
+      entity_specs,
+      base::Time(),
+      syncer::AttachmentIdList(),
+      syncer::AttachmentServiceProxyForTest::Create()));
 
   // 2nd directive.
   time_range_directive->Clear();
   time_range_directive->set_start_time_usec(8);
   time_range_directive->set_end_time_usec(10);
-  directives.push_back(syncer::SyncData::CreateRemoteData(2,
-                                                          entity_specs,
-                                                          base::Time()));
+  directives.push_back(syncer::SyncData::CreateRemoteData(
+      2,
+      entity_specs,
+      base::Time(),
+      syncer::AttachmentIdList(),
+      syncer::AttachmentServiceProxyForTest::Create()));
 
   syncer::FakeSyncChangeProcessor change_processor;
   EXPECT_FALSE(

chrome/browser/managed_mode/managed_user_registration_utility_unittest.cc

@@ -19,6 +19,8 @@
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/browser/browser_thread.h"
 #include "google_apis/gaia/google_service_auth_error.h"
+#include "sync/api/attachments/attachment_id.h"
+#include "sync/api/attachments/attachment_service_proxy_for_test.h"
 #include "sync/api/sync_change.h"
 #include "sync/api/sync_error_factory_mock.h"
 #include "sync/protocol/sync.pb.h"
@@ -218,10 +220,14 @@ void ManagedUserRegistrationUtilityTest::Acknowledge() {
     EXPECT_FALSE(specifics.managed_user().acknowledged());
     specifics.mutable_managed_user()->set_acknowledged(true);
     new_changes.push_back(
-        SyncChange(FROM_HERE, SyncChange::ACTION_UPDATE,
-                   SyncData::CreateRemoteData(++sync_data_id_,
-                                              specifics,
-                                              base::Time())));
+        SyncChange(FROM_HERE,
+                   SyncChange::ACTION_UPDATE,
+                   SyncData::CreateRemoteData(
+                       ++sync_data_id_,
+                       specifics,
+                       base::Time(),
+                       syncer::AttachmentIdList(),
+                       syncer::AttachmentServiceProxyForTest::Create())));
   }
   service()->ProcessSyncChanges(FROM_HERE, new_changes);
 

chrome/browser/managed_mode/managed_user_sync_service_unittest.cc

@@ -5,6 +5,7 @@
 #include <string>
 
 #include "base/bind.h"
+#include "base/message_loop/message_loop.h"
 #include "base/prefs/scoped_user_pref_update.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -13,6 +14,8 @@
 #include "chrome/browser/managed_mode/managed_user_sync_service_factory.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_profile.h"
+#include "sync/api/attachments/attachment_id.h"
+#include "sync/api/attachments/attachment_service_proxy_for_test.h"
 #include "sync/api/sync_change.h"
 #include "sync/api/sync_error_factory_mock.h"
 #include "sync/protocol/sync.pb.h"
@@ -98,6 +101,7 @@ class ManagedUserSyncServiceTest : public ::testing::Test {
   MockChangeProcessor* change_processor() { return change_processor_; }
 
  private:
+  base::MessageLoop message_loop;
   TestingProfile profile_;
   ManagedUserSyncService* service_;
 
@@ -139,7 +143,12 @@ SyncData ManagedUserSyncServiceTest::CreateRemoteData(
   if (!chrome_avatar.empty())
     specifics.mutable_managed_user()->set_chrome_avatar(chrome_avatar);
 
-  return SyncData::CreateRemoteData(++sync_data_id_, specifics, base::Time());
+  return SyncData::CreateRemoteData(
+      ++sync_data_id_,
+      specifics,
+      base::Time(),
+      syncer::AttachmentIdList(),
+      syncer::AttachmentServiceProxyForTest::Create());
 }
 
 TEST_F(ManagedUserSyncServiceTest, MergeEmpty) {

chrome/browser/prefs/synced_pref_change_registrar_browsertest.cc

@@ -15,6 +15,8 @@
 #include "chrome/test/base/testing_pref_service_syncable.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/test/test_utils.h"
+#include "sync/api/attachments/attachment_id.h"
+#include "sync/api/attachments/attachment_service_proxy_for_test.h"
 #include "sync/api/fake_sync_change_processor.h"
 #include "sync/api/sync_change.h"
 #include "sync/api/sync_error_factory.h"
@@ -60,7 +62,11 @@ class SyncedPrefChangeRegistrarTest : public InProcessBrowserTest {
     pref_specifics->set_value(serialized_value);
 
     syncer::SyncData change_data = syncer::SyncData::CreateRemoteData(
-        ++next_sync_data_id_, specifics, base::Time());
+        ++next_sync_data_id_,
+        specifics,
+        base::Time(),
+        syncer::AttachmentIdList(),
+        syncer::AttachmentServiceProxyForTest::Create());
     syncer::SyncChange change(
         FROM_HERE, syncer::SyncChange::ACTION_UPDATE, change_data);
 

chrome/browser/sync/glue/generic_change_processor.cc

@@ -80,9 +80,9 @@ GenericChangeProcessor::GenericChangeProcessor(
       share_handle_(user_share),
       attachment_service_(attachment_service.Pass()),
       attachment_service_weak_ptr_factory_(attachment_service_.get()),
-      attachment_service_proxy_(syncer::AttachmentServiceProxy(
+      attachment_service_proxy_(
           base::MessageLoopProxy::current(),
-          attachment_service_weak_ptr_factory_.GetWeakPtr())) {
+          attachment_service_weak_ptr_factory_.GetWeakPtr()) {
   DCHECK(CalledOnValidThread());
   DCHECK(attachment_service_);
 }

chrome/browser/themes/theme_syncable_service_unittest.cc

@@ -22,6 +22,8 @@
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/permissions/api_permission_set.h"
 #include "extensions/common/permissions/permission_set.h"
+#include "sync/api/attachments/attachment_id.h"
+#include "sync/api/attachments/attachment_service_proxy_for_test.h"
 #include "sync/api/fake_sync_change_processor.h"
 #include "sync/api/sync_change_processor_wrapper_for_test.h"
 #include "sync/api/sync_error.h"
@@ -451,11 +453,15 @@ TEST_F(ThemeSyncableServiceTest, ProcessSyncThemeChange) {
   sync_pb::EntitySpecifics entity_specifics;
   entity_specifics.mutable_theme()->CopyFrom(theme_specifics);
   syncer::SyncChangeList change_list;
-  change_list.push_back(syncer::SyncChange(
-      FROM_HERE,
-      syncer::SyncChange::ACTION_UPDATE,
-      syncer::SyncData::CreateRemoteData(
-          1, entity_specifics, base::Time())));
+  change_list.push_back(
+      syncer::SyncChange(FROM_HERE,
+                         syncer::SyncChange::ACTION_UPDATE,
+                         syncer::SyncData::CreateRemoteData(
+                             1,
+                             entity_specifics,
+                             base::Time(),
+                             syncer::AttachmentIdList(),
+                             syncer::AttachmentServiceProxyForTest::Create())));
   error = theme_sync_service_->ProcessSyncChanges(FROM_HERE, change_list);
   EXPECT_FALSE(error.IsSet()) << error.message();
   EXPECT_EQ(fake_theme_service_->theme_extension(), theme_extension_.get());

chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.cc

@@ -11,6 +11,8 @@
 #include "chrome/browser/sync/glue/session_model_associator.h"
 #include "chrome/browser/sync/open_tabs_ui_delegate.h"
 #include "chrome/browser/sync/sessions2/sessions_sync_manager.h"
+#include "sync/api/attachments/attachment_id.h"
+#include "sync/api/attachments/attachment_service_proxy_for_test.h"
 #include "sync/protocol/session_specifics.pb.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -193,15 +195,25 @@ void RecentTabsBuilderTestHelper::ExportToSessionsSyncManager(
         sync_pb::SessionSpecifics* tab_base = entity.mutable_session();
         BuildTabSpecifics(s, w, t, tab_base);
         changes.push_back(syncer::SyncChange(
-            FROM_HERE, syncer::SyncChange::ACTION_ADD,
+            FROM_HERE,
+            syncer::SyncChange::ACTION_ADD,
             syncer::SyncData::CreateRemoteData(
-                tab_base->tab_node_id(), entity, GetTabTimestamp(s, w, t))));
+                tab_base->tab_node_id(),
+                entity,
+                GetTabTimestamp(s, w, t),
+                syncer::AttachmentIdList(),
+                syncer::AttachmentServiceProxyForTest::Create())));
       }
     }
     changes.push_back(syncer::SyncChange(
-        FROM_HERE, syncer::SyncChange::ACTION_ADD,
-        syncer::SyncData::CreateRemoteData(1, session_entity,
-                                          GetSessionTimestamp(s))));
+        FROM_HERE,
+        syncer::SyncChange::ACTION_ADD,
+        syncer::SyncData::CreateRemoteData(
+            1,
+            session_entity,
+            GetSessionTimestamp(s),
+            syncer::AttachmentIdList(),
+            syncer::AttachmentServiceProxyForTest::Create())));
   }
   manager->ProcessSyncChanges(FROM_HERE, changes);
   VerifyExport(manager);

components/dom_distiller/core/dom_distiller_store_unittest.cc

@@ -7,11 +7,14 @@
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/time/time.h"
 #include "components/dom_distiller/core/article_entry.h"
 #include "components/dom_distiller/core/dom_distiller_test_util.h"
 #include "components/dom_distiller/core/fake_db.h"
+#include "sync/api/attachments/attachment_id.h"
+#include "sync/api/attachments/attachment_service_proxy_for_test.h"
 #include "sync/protocol/sync.pb.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -157,7 +160,11 @@ class DomDistillerStoreTest : public testing::Test {
   SyncData CreateSyncData(const ArticleEntry& entry) {
     EntitySpecifics specifics = SpecificsFromEntry(entry);
     return SyncData::CreateRemoteData(
-        next_sync_id_++, specifics, Time::UnixEpoch());
+        next_sync_id_++,
+        specifics,
+        Time::UnixEpoch(),
+        syncer::AttachmentIdList(),
+        syncer::AttachmentServiceProxyForTest::Create());
   }
 
   SyncDataList SyncDataFromEntryMap(const EntryMap& model) {
@@ -168,6 +175,8 @@ class DomDistillerStoreTest : public testing::Test {
     return data;
   }
 
+  base::MessageLoop message_loop_;
+
   EntryMap db_model_;
   EntryMap sync_model_;
   FakeDB::EntryMap store_model_;

sync/api/attachments/attachment_service_proxy.cc

@@ -36,16 +36,26 @@ void ProxyDropCallback(
 
 }  // namespace
 
-AttachmentServiceProxy::AttachmentServiceProxy() {}
+AttachmentServiceProxy::AttachmentServiceProxy() {
+}
 
 AttachmentServiceProxy::AttachmentServiceProxy(
     const scoped_refptr<base::SequencedTaskRunner>& wrapped_task_runner,
-    base::WeakPtr<syncer::AttachmentService> wrapped)
-    : wrapped_task_runner_(wrapped_task_runner), wrapped_(wrapped) {
+    const base::WeakPtr<syncer::AttachmentService>& wrapped)
+    : wrapped_task_runner_(wrapped_task_runner), core_(new Core(wrapped)) {
   DCHECK(wrapped_task_runner_);
 }
 
-AttachmentServiceProxy::~AttachmentServiceProxy() {}
+AttachmentServiceProxy::AttachmentServiceProxy(
+    const scoped_refptr<base::SequencedTaskRunner>& wrapped_task_runner,
+    const scoped_refptr<Core>& core)
+    : wrapped_task_runner_(wrapped_task_runner), core_(core) {
+  DCHECK(wrapped_task_runner_);
+  DCHECK(core_);
+}
+
+AttachmentServiceProxy::~AttachmentServiceProxy() {
+}
 
 void AttachmentServiceProxy::GetOrDownloadAttachments(
     const AttachmentIdList& attachment_ids,
@@ -56,7 +66,7 @@ void AttachmentServiceProxy::GetOrDownloadAttachments(
   wrapped_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&AttachmentService::GetOrDownloadAttachments,
-                 wrapped_,
+                 core_,
                  attachment_ids,
                  proxy_callback));
 }
@@ -69,7 +79,7 @@ void AttachmentServiceProxy::DropAttachments(
       &ProxyDropCallback, base::MessageLoopProxy::current(), callback);
   wrapped_task_runner_->PostTask(FROM_HERE,
                                  base::Bind(&AttachmentService::DropAttachments,
-                                            wrapped_,
+                                            core_,
                                             attachment_ids,
                                             proxy_callback));
 }
@@ -78,14 +88,14 @@ void AttachmentServiceProxy::OnSyncDataAdd(const SyncData& sync_data) {
   DCHECK(wrapped_task_runner_);
   wrapped_task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(&AttachmentService::OnSyncDataAdd, wrapped_, sync_data));
+      base::Bind(&AttachmentService::OnSyncDataAdd, core_, sync_data));
 }
 
 void AttachmentServiceProxy::OnSyncDataDelete(const SyncData& sync_data) {
   DCHECK(wrapped_task_runner_);
   wrapped_task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(&AttachmentService::OnSyncDataDelete, wrapped_, sync_data));
+      base::Bind(&AttachmentService::OnSyncDataDelete, core_, sync_data));
 }
 
 void AttachmentServiceProxy::OnSyncDataUpdate(
@@ -95,9 +105,58 @@ void AttachmentServiceProxy::OnSyncDataUpdate(
   wrapped_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&AttachmentService::OnSyncDataUpdate,
-                 wrapped_,
+                 core_,
                  old_attachment_ids,
                  updated_sync_data));
 }
 
+AttachmentServiceProxy::Core::Core(
+    const base::WeakPtr<syncer::AttachmentService>& wrapped)
+    : wrapped_(wrapped) {
+}
+
+AttachmentServiceProxy::Core::~Core() {
+}
+
+void AttachmentServiceProxy::Core::GetOrDownloadAttachments(
+    const AttachmentIdList& attachment_ids,
+    const GetOrDownloadCallback& callback) {
+  if (!wrapped_) {
+    return;
+  }
+  wrapped_->GetOrDownloadAttachments(attachment_ids, callback);
+}
+
+void AttachmentServiceProxy::Core::DropAttachments(
+    const AttachmentIdList& attachment_ids,
+    const DropCallback& callback) {
+  if (!wrapped_) {
+    return;
+  }
+  wrapped_->DropAttachments(attachment_ids, callback);
+}
+
+void AttachmentServiceProxy::Core::OnSyncDataAdd(const SyncData& sync_data) {
+  if (!wrapped_) {
+    return;
+  }
+  wrapped_->OnSyncDataAdd(sync_data);
+}
+
+void AttachmentServiceProxy::Core::OnSyncDataDelete(const SyncData& sync_data) {
+  if (!wrapped_) {
+    return;
+  }
+  wrapped_->OnSyncDataDelete(sync_data);
+}
+
+void AttachmentServiceProxy::Core::OnSyncDataUpdate(
+    const AttachmentIdList& old_attachment_ids,
+    const SyncData& updated_sync_data) {
+  if (!wrapped_) {
+    return;
+  }
+  wrapped_->OnSyncDataUpdate(old_attachment_ids, updated_sync_data);
+}
+
 }  // namespace syncer

sync/api/attachments/attachment_service_proxy.h

@@ -20,7 +20,8 @@ namespace syncer {
 class SyncData;
 
 // AttachmentServiceProxy wraps an AttachmentService allowing multiple threads
-// to share the wrapped AttachmentService.
+// to share the wrapped AttachmentService and invoke its methods in the
+// appropriate thread.
 //
 // Callbacks passed to methods on this class will be invoked in the same thread
 // from which the method was called.
@@ -29,10 +30,10 @@ class SyncData;
 // holds a WeakPtr to the wrapped object.  Once the the wrapped object is
 // destroyed, method calls on this object will be no-ops.
 //
-//  Users of this class should take care to destroy the wrapped object on the
-//  correct thread (wrapped_task_runner).
+// Users of this class should take care to destroy the wrapped object on the
+// correct thread (wrapped_task_runner).
 //
-// This class is thread-safe.
+// This class is thread-safe and is designed to be passed by const-ref.
 class SYNC_EXPORT AttachmentServiceProxy : public AttachmentService {
  public:
   // Default copy and assignment are welcome.
@@ -42,16 +43,19 @@ class SYNC_EXPORT AttachmentServiceProxy : public AttachmentService {
 
   // Construct an AttachmentServiceProxy that forwards calls to |wrapped| on the
   // |wrapped_task_runner| thread.
+  //
+  // Note, this object does not own |wrapped|.  When |wrapped| is destroyed,
+  // calls to this object become no-ops.
   AttachmentServiceProxy(
       const scoped_refptr<base::SequencedTaskRunner>& wrapped_task_runner,
-      base::WeakPtr<syncer::AttachmentService> wrapped);
+      const base::WeakPtr<syncer::AttachmentService>& wrapped);
 
   virtual ~AttachmentServiceProxy();
 
   // AttachmentService implementation.
-  virtual void GetOrDownloadAttachments(const AttachmentIdList& attachment_ids,
-                                        const GetOrDownloadCallback& callback)
-      OVERRIDE;
+  virtual void GetOrDownloadAttachments(
+      const AttachmentIdList& attachment_ids,
+      const GetOrDownloadCallback& callback) OVERRIDE;
   virtual void DropAttachments(const AttachmentIdList& attachment_ids,
                                const DropCallback& callback) OVERRIDE;
   virtual void OnSyncDataAdd(const SyncData& sync_data) OVERRIDE;
@@ -59,10 +63,55 @@ class SYNC_EXPORT AttachmentServiceProxy : public AttachmentService {
   virtual void OnSyncDataUpdate(const AttachmentIdList& old_attachment_ids,
                                 const SyncData& updated_sync_data) OVERRIDE;
 
+ protected:
+  // Core does the work of proxying calls to AttachmentService methods from one
+  // thread to another so AttachmentServiceProxy can be an easy-to-use,
+  // non-ref-counted A ref-counted class.
+  //
+  // Callback from AttachmentService are proxied back using free functions
+  // defined in the .cc file (ProxyFooCallback functions).
+  //
+  // Core is ref-counted because we want to allow AttachmentServiceProxy to be
+  // copy-constructable while allowing for different implementations of Core
+  // (e.g. one type of core might own the wrapped AttachmentService).
+  //
+  // Calls to objects of this class become no-ops once its wrapped object is
+  // destroyed.
+  class SYNC_EXPORT Core : public AttachmentService,
+                           public base::RefCountedThreadSafe<Core> {
+   public:
+    // Construct an AttachmentServiceProxyCore that forwards calls to |wrapped|.
+    Core(const base::WeakPtr<syncer::AttachmentService>& wrapped);
+
+    // AttachmentService implementation.
+    virtual void GetOrDownloadAttachments(
+        const AttachmentIdList& attachment_ids,
+        const GetOrDownloadCallback& callback) OVERRIDE;
+    virtual void DropAttachments(const AttachmentIdList& attachment_ids,
+                                 const DropCallback& callback) OVERRIDE;
+    virtual void OnSyncDataAdd(const SyncData& sync_data) OVERRIDE;
+    virtual void OnSyncDataDelete(const SyncData& sync_data) OVERRIDE;
+    virtual void OnSyncDataUpdate(const AttachmentIdList& old_attachment_ids,
+                                  const SyncData& updated_sync_data) OVERRIDE;
+
+   protected:
+    friend class base::RefCountedThreadSafe<Core>;
+    virtual ~Core();
+
+   private:
+    base::WeakPtr<AttachmentService> wrapped_;
+
+    DISALLOW_COPY_AND_ASSIGN(Core);
+  };
+
+  // Used in tests to create an AttachmentServiceProxy with a custom Core.
+  AttachmentServiceProxy(
+      const scoped_refptr<base::SequencedTaskRunner>& wrapped_task_runner,
+      const scoped_refptr<Core>& core);
+
  private:
   scoped_refptr<base::SequencedTaskRunner> wrapped_task_runner_;
-  // wrapped_ must only be dereferenced on the wrapped_task_runner_ thread.
-  base::WeakPtr<AttachmentService> wrapped_;
+  scoped_refptr<Core> core_;
 };
 
 }  // namespace syncer

sync/api/attachments/attachment_service_proxy_for_test.cc

+// 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/api/attachments/attachment_service_proxy_for_test.h"
+
+#include "base/message_loop/message_loop_proxy.h"
+#include "sync/api/attachments/fake_attachment_service.h"
+
+namespace syncer {
+
+AttachmentServiceProxyForTest::OwningCore::OwningCore(
+    scoped_ptr<AttachmentService> wrapped,
+    scoped_ptr<base::WeakPtrFactory<AttachmentService> > weak_ptr_factory)
+    : Core(weak_ptr_factory->GetWeakPtr()),
+      wrapped_(wrapped.Pass()),
+      weak_ptr_factory_(weak_ptr_factory.Pass()) {
+  DCHECK(wrapped_);
+}
+
+AttachmentServiceProxyForTest::OwningCore::~OwningCore() {
+}
+
+// Static.
+AttachmentServiceProxy AttachmentServiceProxyForTest::Create() {
+  scoped_ptr<AttachmentService> wrapped(FakeAttachmentService::CreateForTest());
+  // This class's base class, AttachmentServiceProxy, must be initialized with a
+  // WeakPtr to an AttachmentService.  Because the base class ctor must be
+  // invoked before any of this class's members are initialized, we create the
+  // WeakPtrFactory here and pass it to the ctor so that it may initialize its
+  // base class and own the WeakPtrFactory.
+  //
+  // We must pass by scoped_ptr because WeakPtrFactory has no copy constructor.
+  scoped_ptr<base::WeakPtrFactory<AttachmentService> > weak_ptr_factory(
+      new base::WeakPtrFactory<AttachmentService>(wrapped.get()));
+
+  scoped_refptr<Core> core_for_test(
+      new OwningCore(wrapped.Pass(), weak_ptr_factory.Pass()));
+  return AttachmentServiceProxyForTest(base::MessageLoopProxy::current(),
+                                       core_for_test);
+}
+
+AttachmentServiceProxyForTest::~AttachmentServiceProxyForTest() {
+}
+
+AttachmentServiceProxyForTest::AttachmentServiceProxyForTest(
+    const scoped_refptr<base::SequencedTaskRunner>& wrapped_task_runner,
+    const scoped_refptr<Core>& core)
+    : AttachmentServiceProxy(wrapped_task_runner, core) {
+}
+
+}  // namespace syncer

sync/api/attachments/attachment_service_proxy_for_test.h

+// 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.
+
+#ifndef SYNC_API_ATTACHMENTS_ATTACHMENT_SERVICE_PROXY_FOR_TEST_H_
+#define SYNC_API_ATTACHMENTS_ATTACHMENT_SERVICE_PROXY_FOR_TEST_H_
+
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "sync/api/attachments/attachment_service_proxy.h"
+#include "sync/base/sync_export.h"
+
+namespace syncer {
+
+// An self-contained AttachmentServiceProxy to reduce boilerplate code in tests.
+//
+// Constructs and owns an AttachmentService suitable for use in tests.  Assumes
+// the current thread has a MessageLoop.
+class SYNC_EXPORT AttachmentServiceProxyForTest
+    : public AttachmentServiceProxy {
+ public:
+  static AttachmentServiceProxy Create();
+  virtual ~AttachmentServiceProxyForTest();
+
+ private:
+  // A Core that owns the wrapped AttachmentService.
+  class OwningCore : public AttachmentServiceProxy::Core {
+   public:
+    OwningCore(
+        scoped_ptr<AttachmentService>,
+        scoped_ptr<base::WeakPtrFactory<AttachmentService> > weak_ptr_factory);
+
+   private:
+    scoped_ptr<AttachmentService> wrapped_;
+    // WeakPtrFactory for wrapped_.  See Create() for why this is a scoped_ptr.
+    scoped_ptr<base::WeakPtrFactory<AttachmentService> > weak_ptr_factory_;
+
+    virtual ~OwningCore();
+
+    DISALLOW_COPY_AND_ASSIGN(OwningCore);
+  };
+
+  AttachmentServiceProxyForTest(
+      const scoped_refptr<base::SequencedTaskRunner>& wrapped_task_runner,
+      const scoped_refptr<Core>& core);
+};
+
+}  // namespace syncer
+
+#endif  // SYNC_API_ATTACHMENTS_ATTACHMENT_SERVICE_PROXY_FOR_TEST_H_

sync/api/sync_change_unittest.cc

@@ -7,8 +7,11 @@
 #include <string>
 
 #include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
 #include "base/time/time.h"
 #include "base/values.h"
+#include "sync/api/attachments/attachment_id.h"
+#include "sync/api/attachments/attachment_service_proxy_for_test.h"
 #include "sync/protocol/preference_specifics.pb.h"
 #include "sync/protocol/proto_value_conversions.h"
 #include "sync/protocol/sync.pb.h"
@@ -21,7 +24,10 @@ typedef std::vector<SyncChange> SyncChangeList;
 
 namespace {
 
-typedef testing::Test SyncChangeTest;
+class SyncChangeTest : public testing::Test {
+ private:
+  base::MessageLoop message_loop;
+};
 
 TEST_F(SyncChangeTest, LocalDelete) {
   SyncChange::SyncChangeType change_type = SyncChange::ACTION_DELETE;
@@ -82,28 +88,43 @@ TEST_F(SyncChangeTest, SyncerChanges) {
   sync_pb::PreferenceSpecifics* pref_specifics =
       update_specifics.mutable_preference();
   pref_specifics->set_name("update");
-  change_list.push_back(SyncChange(
-      FROM_HERE,
-      SyncChange::ACTION_UPDATE,
-      SyncData::CreateRemoteData(1, update_specifics, base::Time())));
+  change_list.push_back(
+      SyncChange(FROM_HERE,
+                 SyncChange::ACTION_UPDATE,
+                 SyncData::CreateRemoteData(
+                     1,
+                     update_specifics,
+                     base::Time(),
+                     syncer::AttachmentIdList(),
+                     syncer::AttachmentServiceProxyForTest::Create())));
 
   // Create an add.
   sync_pb::EntitySpecifics add_specifics;
   pref_specifics = add_specifics.mutable_preference();
   pref_specifics->set_name("add");
-  change_list.push_back(SyncChange(
-      FROM_HERE,
-      SyncChange::ACTION_ADD,
-      SyncData::CreateRemoteData(2, add_specifics, base::Time())));
+  change_list.push_back(
+      SyncChange(FROM_HERE,
+                 SyncChange::ACTION_ADD,
+                 SyncData::CreateRemoteData(
+                     2,
+                     add_specifics,
+                     base::Time(),
+                     syncer::AttachmentIdList(),
+                     syncer::AttachmentServiceProxyForTest::Create())));
 
   // Create a delete.
   sync_pb::EntitySpecifics delete_specifics;
   pref_specifics = delete_specifics.mutable_preference();
   pref_specifics->set_name("add");
-  change_list.push_back(SyncChange(
-      FROM_HERE,
-      SyncChange::ACTION_DELETE,
-      SyncData::CreateRemoteData(3, delete_specifics, base::Time())));
+  change_list.push_back(
+      SyncChange(FROM_HERE,
+                 SyncChange::ACTION_DELETE,
+                 SyncData::CreateRemoteData(
+                     3,
+                     delete_specifics,
+                     base::Time(),
+                     syncer::AttachmentIdList(),
+                     syncer::AttachmentServiceProxyForTest::Create())));
 
   ASSERT_EQ(3U, change_list.size());
 

sync/api/sync_data.cc

@@ -143,17 +143,6 @@ SyncData SyncData::CreateRemoteData(
       id, &entity, &attachments, modification_time, attachment_service);
 }
 
-// Static.
-SyncData SyncData::CreateRemoteData(int64 id,
-                                    const sync_pb::EntitySpecifics& specifics,
-                                    const base::Time& modification_time) {
-  return CreateRemoteData(id,
-                          specifics,
-                          modification_time,
-                          AttachmentIdList(),
-                          AttachmentServiceProxy());
-}
-
 bool SyncData::IsValid() const { return is_valid_; }
 
 const sync_pb::EntitySpecifics& SyncData::GetSpecifics() const {

sync/api/sync_data.h

@@ -11,6 +11,7 @@
 
 #include "base/basictypes.h"
 #include "base/callback.h"
+#include "base/memory/ref_counted.h"
 #include "base/stl_util.h"
 #include "base/time/time.h"
 #include "sync/api/attachments/attachment.h"
@@ -64,18 +65,12 @@ class SYNC_EXPORT SyncData {
       const AttachmentList& attachments);
 
   // Helper method for creating SyncData objects originating from the syncer.
-  //
-  // TODO(maniscalco): Replace all calls to 3-arg CreateRemoteData with calls to
-  // the 5-arg version (bug 353296).
   static SyncData CreateRemoteData(
       int64 id,
       const sync_pb::EntitySpecifics& specifics,
       const base::Time& last_modified_time,
       const AttachmentIdList& attachment_ids,
       const syncer::AttachmentServiceProxy& attachment_service);
-  static SyncData CreateRemoteData(int64 id,
-                                   const sync_pb::EntitySpecifics& specifics,
-                                   const base::Time& last_modified_time);
 
   // Whether this SyncData holds valid data. The only way to have a SyncData
   // without valid data is to use the default constructor.

sync/api/sync_data_unittest.cc

@@ -120,16 +120,6 @@ TEST_F(SyncDataTest, CreateRemoteData) {
   EXPECT_TRUE(data.GetAttachmentIds().empty());
 }
 
-TEST_F(SyncDataTest, CreateRemoteData_WithoutAttachmentService) {
-  specifics.mutable_preference();
-  SyncData data = SyncData::CreateRemoteData(kId, specifics, kLastModifiedTime);
-  EXPECT_TRUE(data.IsValid());
-  EXPECT_FALSE(data.IsLocal());
-  EXPECT_EQ(kId, SyncDataRemote(data).GetId());
-  EXPECT_EQ(kLastModifiedTime, SyncDataRemote(data).GetModifiedTime());
-  EXPECT_TRUE(data.GetSpecifics().has_preference());
-}
-
 // TODO(maniscalco): Add test cases that verify GetLocalAttachmentsForUpload and
 // DropAttachments calls are passed through to the underlying AttachmentService.
 

sync/sync_api.gypi

@@ -22,6 +22,8 @@
     'api/attachments/attachment_service.h',
     'api/attachments/attachment_service_proxy.cc',
     'api/attachments/attachment_service_proxy.h',
+    'api/attachments/attachment_service_proxy_for_test.cc',
+    'api/attachments/attachment_service_proxy_for_test.h',
     'api/attachments/attachment_store.cc',
     'api/attachments/attachment_store.h',
     'api/attachments/fake_attachment_service.cc',