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

9 files changed, 266 insertions, 59 deletions

diff --git a/sync/api/attachments/attachment_service_proxy.cc b/sync/api/attachments/attachment_service_proxy.cc
index 065f467..b95a41f 100644
--- a/sync/api/attachments/attachment_service_proxy.cc
+++ b/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
diff --git a/sync/api/attachments/attachment_service_proxy.h b/sync/api/attachments/attachment_service_proxy.h
index 5bec1b3..7085625 100644
--- a/sync/api/attachments/attachment_service_proxy.h
+++ b/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
diff --git a/sync/api/attachments/attachment_service_proxy_for_test.cc b/sync/api/attachments/attachment_service_proxy_for_test.cc
new file mode 100644
index 0000000..7f9f873
--- /dev/null
+++ b/sync/api/attachments/attachment_service_proxy_for_test.cc
@@ -0,0 +1,52 @@
+// 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
diff --git a/sync/api/attachments/attachment_service_proxy_for_test.h b/sync/api/attachments/attachment_service_proxy_for_test.h
new file mode 100644
index 0000000..916e788
--- /dev/null
+++ b/sync/api/attachments/attachment_service_proxy_for_test.h
@@ -0,0 +1,50 @@
+// 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_
diff --git a/sync/api/sync_change_unittest.cc b/sync/api/sync_change_unittest.cc
index 5f937ab..a09776e 100644
--- a/sync/api/sync_change_unittest.cc
+++ b/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());
 
diff --git a/sync/api/sync_data.cc b/sync/api/sync_data.cc
index 5d849412..fa48ea5 100644
--- a/sync/api/sync_data.cc
+++ b/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 {
diff --git a/sync/api/sync_data.h b/sync/api/sync_data.h
index 717b1e1..bb99b74 100644
--- a/sync/api/sync_data.h
+++ b/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.
diff --git a/sync/api/sync_data_unittest.cc b/sync/api/sync_data_unittest.cc
index 7ca39bf..4eab286 100644
--- a/sync/api/sync_data_unittest.cc
+++ b/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.
 
diff --git a/sync/sync_api.gypi b/sync/sync_api.gypi
index 02ff2c4..8e5b911 100644
--- a/sync/sync_api.gypi
+++ b/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',