Revert of Refactor FakeAttachmentStore (patchset #3 id:40001 of...

Revert of Refactor FakeAttachmentStore (patchset #3 id:40001 of https://codereview.chromium.org/601553004/)

Reason for revert:
Speculative revert:
looks like it causes failures on the Linux asan: http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Tests%20%283%29/builds/8113

Original issue's description:
> Refactor FakeAttachmentStore
>
> Refactor FakeAttachmentStore into AttachmentStoreProxy and
> InMemoryAttachmentStore backend. Break AttachmentStore interface into
> AttachmentStoreBase (common interface for backends) and AttachmentStore
> (referenced by datatype and AttachmentService).
>
> BUG=
> R=maniscalco@chromium.org
>
> Committed: https://crrev.com/6ae2b341c73909cf902078446346d6c900273409
> Cr-Commit-Position: refs/heads/master@{#297339}

TBR=maniscalco@chromium.org,pavely@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=

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

Cr-Commit-Position: refs/heads/master@{#297379}

chrome/browser/sync/profile_sync_components_factory_impl.cc

@@ -65,6 +65,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "google_apis/gaia/oauth2_token_service_request.h"
 #include "net/url_request/url_request_context_getter.h"
+#include "sync/api/attachments/fake_attachment_store.h"
 #include "sync/api/syncable_service.h"
 #include "sync/internal_api/public/attachments/attachment_downloader.h"
 #include "sync/internal_api/public/attachments/attachment_service.h"

chrome/browser/sync/profile_sync_components_factory_mock.cc

@@ -8,7 +8,7 @@
 #include "components/sync_driver/change_processor.h"
 #include "components/sync_driver/model_associator.h"
 #include "content/public/browser/browser_thread.h"
-#include "sync/api/attachments/attachment_store.h"
+#include "sync/api/attachments/fake_attachment_store.h"
 #include "sync/internal_api/public/attachments/attachment_service_impl.h"
 
 using sync_driver::AssociatorInterface;

components/sync_driver/generic_change_processor_unittest.cc

@@ -12,7 +12,7 @@
 #include "components/sync_driver/data_type_error_handler_mock.h"
 #include "components/sync_driver/sync_api_component_factory.h"
 #include "sync/api/attachments/attachment_id.h"
-#include "sync/api/attachments/attachment_store.h"
+#include "sync/api/attachments/fake_attachment_store.h"
 #include "sync/api/fake_syncable_service.h"
 #include "sync/api/sync_change.h"
 #include "sync/api/sync_merge_result.h"
@@ -147,8 +147,8 @@ class SyncGenericChangeProcessorTest : public testing::Test {
   }
 
   void ConstructGenericChangeProcessor(syncer::ModelType type) {
-    scoped_refptr<syncer::AttachmentStore> attachment_store =
-        syncer::AttachmentStore::CreateInMemoryStore();
+    scoped_refptr<syncer::AttachmentStore> attachment_store(
+        new syncer::FakeAttachmentStore(base::MessageLoopProxy::current()));
     scoped_ptr<MockAttachmentService> mock_attachment_service(
         new MockAttachmentService(attachment_store));
     // GenericChangeProcessor takes ownership of the AttachmentService, but we

sync/BUILD.gn

@@ -20,6 +20,8 @@ source_set("sync_core") {
     "api/attachments/attachment_id.h",
     "api/attachments/attachment_store.cc",
     "api/attachments/attachment_store.h",
+    "api/attachments/fake_attachment_store.cc",
+    "api/attachments/fake_attachment_store.h",
     "api/string_ordinal.h",
     "api/syncable_service.cc",
     "api/syncable_service.h",
@@ -120,12 +122,16 @@ source_set("sync_core") {
     "internal_api/attachments/attachment_service_impl.cc",
     "internal_api/attachments/attachment_service_proxy.cc",
     "internal_api/attachments/attachment_service_proxy_for_test.cc",
-    "internal_api/attachments/attachment_store_handle.cc",
     "internal_api/attachments/attachment_uploader.cc",
     "internal_api/attachments/attachment_uploader_impl.cc",
     "internal_api/attachments/fake_attachment_downloader.cc",
     "internal_api/attachments/fake_attachment_uploader.cc",
-    "internal_api/attachments/in_memory_attachment_store.cc",
+    "internal_api/attachments/public/attachment_downloader.h",
+    "internal_api/attachments/public/attachment_service.h",
+    "internal_api/attachments/public/attachment_service_impl.h",
+    "internal_api/attachments/public/attachment_service_proxy_for_test.h",
+    "internal_api/attachments/public/attachment_service_proxy.h",
+    "internal_api/attachments/public/attachment_uploader.h",
     "internal_api/base_node.cc",
     "internal_api/base_transaction.cc",
     "internal_api/change_record.cc",
@@ -152,18 +158,10 @@ source_set("sync_core") {
     "internal_api/js_sync_manager_observer.h",
     "internal_api/protocol_event_buffer.cc",
     "internal_api/protocol_event_buffer.h",
-    "internal_api/public/attachments/attachment_downloader.h",
     "internal_api/public/attachments/attachment_downloader_impl.h",
-    "internal_api/public/attachments/attachment_service.h",
-    "internal_api/public/attachments/attachment_service_impl.h",
-    "internal_api/public/attachments/attachment_service_proxy.h",
-    "internal_api/public/attachments/attachment_service_proxy_for_test.h",
-    "internal_api/public/attachments/attachment_store_handle.h",
-    "internal_api/public/attachments/attachment_uploader.h",
     "internal_api/public/attachments/attachment_uploader_impl.h",
     "internal_api/public/attachments/fake_attachment_downloader.h",
     "internal_api/public/attachments/fake_attachment_uploader.h",
-    "internal_api/public/attachments/in_memory_attachment_store.h",
     "internal_api/public/base/attachment_id_proto.cc",
     "internal_api/public/base/attachment_id_proto.h",
     "internal_api/public/base/cancelation_observer.cc",
@@ -563,7 +561,6 @@ test("sync_unit_tests") {
     "internal_api/attachments/attachment_downloader_impl_unittest.cc",
     "internal_api/attachments/attachment_service_impl_unittest.cc",
     "internal_api/attachments/attachment_service_proxy_unittest.cc",
-    "internal_api/attachments/attachment_store_handle_unittest.cc",
     "internal_api/attachments/attachment_uploader_impl_unittest.cc",
     "internal_api/attachments/fake_attachment_downloader_unittest.cc",
     "internal_api/attachments/fake_attachment_uploader_unittest.cc",

sync/api/attachments/attachment_store.cc

@@ -4,24 +4,9 @@
 
 #include "sync/api/attachments/attachment_store.h"
 
-#include "base/thread_task_runner_handle.h"
-#include "sync/internal_api/public/attachments/attachment_store_handle.h"
-#include "sync/internal_api/public/attachments/in_memory_attachment_store.h"
-
 namespace syncer {
 
-AttachmentStoreBase::AttachmentStoreBase() {}
-AttachmentStoreBase::~AttachmentStoreBase() {}
-
 AttachmentStore::AttachmentStore() {}
 AttachmentStore::~AttachmentStore() {}
 
-scoped_refptr<AttachmentStore> AttachmentStore::CreateInMemoryStore() {
-  // Both frontend and backend of attachment store will live on current thread.
-  scoped_ptr<AttachmentStoreBase> backend(
-      new InMemoryAttachmentStore(base::ThreadTaskRunnerHandle::Get()));
-  return scoped_refptr<AttachmentStore>(new AttachmentStoreHandle(
-      backend.Pass(), base::ThreadTaskRunnerHandle::Get()));
-}
-
 }  // namespace syncer

sync/api/attachments/attachment_store.h

@@ -25,8 +25,10 @@ class AttachmentId;
 //
 // Destroying this object does not necessarily cancel outstanding async
 // operations. If you need cancel like semantics, use WeakPtr in the callbacks.
-class SYNC_EXPORT AttachmentStoreBase {
+class SYNC_EXPORT AttachmentStore : public base::RefCounted<AttachmentStore> {
  public:
+  AttachmentStore();
+
   // TODO(maniscalco): Consider udpating Read and Write methods to support
   // resumable transfers (bug 353292).
 
@@ -41,9 +43,6 @@ class SYNC_EXPORT AttachmentStoreBase {
   typedef base::Callback<void(const Result&)> WriteCallback;
   typedef base::Callback<void(const Result&)> DropCallback;
 
-  AttachmentStoreBase();
-  virtual ~AttachmentStoreBase();
-
   // Asynchronously reads the attachments identified by |ids|.
   //
   // |callback| will be invoked when finished. AttachmentStore will attempt to
@@ -82,22 +81,9 @@ class SYNC_EXPORT AttachmentStoreBase {
   // successfully.
   virtual void Drop(const AttachmentIdList& ids,
                     const DropCallback& callback) = 0;
-};
-
-// AttachmentStore is an interface exposed to data type and AttachmentService
-// code. Also contains factory methods for default implementations.
-class SYNC_EXPORT AttachmentStore
-    : public AttachmentStoreBase,
-      public base::RefCountedThreadSafe<AttachmentStore> {
- public:
-  AttachmentStore();
-
-  // Creates an AttachmentStoreHandle backed by in-memory implementation of
-  // attachment store. For now frontend lives on the same thread as backend.
-  static scoped_refptr<AttachmentStore> CreateInMemoryStore();
 
  protected:
-  friend class base::RefCountedThreadSafe<AttachmentStore>;
+  friend class base::RefCounted<AttachmentStore>;
   virtual ~AttachmentStore();
 };
 

sync/internal_api/attachments/in_memory_attachment_store.cc → sync/api/attachments/fake_attachment_store.cc

@@ -2,28 +2,47 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "sync/internal_api/public/attachments/in_memory_attachment_store.h"
+#include "sync/api/attachments/fake_attachment_store.h"
 
 #include "base/bind.h"
-#include "base/callback.h"
 #include "base/location.h"
-#include "base/memory/scoped_ptr.h"
+#include "base/memory/ref_counted_memory.h"
+#include "base/sequenced_task_runner.h"
+#include "base/single_thread_task_runner.h"
+#include "base/thread_task_runner_handle.h"
+#include "sync/api/attachments/attachment.h"
 
 namespace syncer {
 
-InMemoryAttachmentStore::InMemoryAttachmentStore(
-    const scoped_refptr<base::SingleThreadTaskRunner>& callback_task_runner)
-    : callback_task_runner_(callback_task_runner) {
-  // Object is created on one thread but used on another.
-  DetachFromThread();
-}
+// Backend is where all the work happens.
+class FakeAttachmentStore::Backend
+    : public base::RefCountedThreadSafe<FakeAttachmentStore::Backend> {
+ public:
+  // Construct a Backend that posts its results to |frontend_task_runner|.
+  Backend(
+      const scoped_refptr<base::SingleThreadTaskRunner>& frontend_task_runner);
 
-InMemoryAttachmentStore::~InMemoryAttachmentStore() {
-}
+  void Read(const AttachmentIdList& ids, const ReadCallback& callback);
+  void Write(const AttachmentList& attachments, const WriteCallback& callback);
+  void Drop(const AttachmentIdList& ids, const DropCallback& callback);
+
+ private:
+  friend class base::RefCountedThreadSafe<Backend>;
+
+  ~Backend();
+
+  scoped_refptr<base::SingleThreadTaskRunner> frontend_task_runner_;
+  AttachmentMap attachments_;
+};
 
-void InMemoryAttachmentStore::Read(const AttachmentIdList& ids,
-                                   const ReadCallback& callback) {
-  DCHECK(CalledOnValidThread());
+FakeAttachmentStore::Backend::Backend(
+    const scoped_refptr<base::SingleThreadTaskRunner>& frontend_task_runner)
+    : frontend_task_runner_(frontend_task_runner) {}
+
+FakeAttachmentStore::Backend::~Backend() {}
+
+void FakeAttachmentStore::Backend::Read(const AttachmentIdList& ids,
+                                        const ReadCallback& callback) {
   Result result_code = SUCCESS;
   AttachmentIdList::const_iterator id_iter = ids.begin();
   AttachmentIdList::const_iterator id_end = ids.end();
@@ -43,7 +62,7 @@ void InMemoryAttachmentStore::Read(const AttachmentIdList& ids,
   if (!unavailable_attachments->empty()) {
     result_code = UNSPECIFIED_ERROR;
   }
-  callback_task_runner_->PostTask(
+  frontend_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(callback,
                  result_code,
@@ -51,20 +70,18 @@ void InMemoryAttachmentStore::Read(const AttachmentIdList& ids,
                  base::Passed(&unavailable_attachments)));
 }
 
-void InMemoryAttachmentStore::Write(const AttachmentList& attachments,
-                                    const WriteCallback& callback) {
-  DCHECK(CalledOnValidThread());
+void FakeAttachmentStore::Backend::Write(const AttachmentList& attachments,
+                                         const WriteCallback& callback) {
   AttachmentList::const_iterator iter = attachments.begin();
   AttachmentList::const_iterator end = attachments.end();
   for (; iter != end; ++iter) {
     attachments_.insert(std::make_pair(iter->GetId(), *iter));
   }
-  callback_task_runner_->PostTask(FROM_HERE, base::Bind(callback, SUCCESS));
+  frontend_task_runner_->PostTask(FROM_HERE, base::Bind(callback, SUCCESS));
 }
 
-void InMemoryAttachmentStore::Drop(const AttachmentIdList& ids,
-                                   const DropCallback& callback) {
-  DCHECK(CalledOnValidThread());
+void FakeAttachmentStore::Backend::Drop(const AttachmentIdList& ids,
+                                        const DropCallback& callback) {
   Result result = SUCCESS;
   AttachmentIdList::const_iterator ids_iter = ids.begin();
   AttachmentIdList::const_iterator ids_end = ids.end();
@@ -74,7 +91,38 @@ void InMemoryAttachmentStore::Drop(const AttachmentIdList& ids,
       attachments_.erase(attachments_iter);
     }
   }
-  callback_task_runner_->PostTask(FROM_HERE, base::Bind(callback, result));
+  frontend_task_runner_->PostTask(FROM_HERE, base::Bind(callback, result));
+}
+
+FakeAttachmentStore::FakeAttachmentStore(
+    const scoped_refptr<base::SequencedTaskRunner>& backend_task_runner)
+    : backend_(new Backend(base::ThreadTaskRunnerHandle::Get())),
+      backend_task_runner_(backend_task_runner) {}
+
+FakeAttachmentStore::~FakeAttachmentStore() {}
+
+void FakeAttachmentStore::Read(const AttachmentIdList& ids,
+                               const ReadCallback& callback) {
+  backend_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&FakeAttachmentStore::Backend::Read, backend_, ids, callback));
+}
+
+void FakeAttachmentStore::Write(const AttachmentList& attachments,
+                                const WriteCallback& callback) {
+  backend_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&FakeAttachmentStore::Backend::Write,
+                 backend_,
+                 attachments,
+                 callback));
+}
+
+void FakeAttachmentStore::Drop(const AttachmentIdList& ids,
+                               const DropCallback& callback) {
+  backend_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&FakeAttachmentStore::Backend::Drop, backend_, ids, callback));
 }
 
 }  // namespace syncer

sync/internal_api/public/attachments/attachment_store_handle.h → sync/api/attachments/fake_attachment_store.h

@@ -2,20 +2,20 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef SYNC_INTERNAL_API_PUBLIC_ATTACHMENTS_ATTACHMENT_STORE_HANDLE_H_
-#define SYNC_INTERNAL_API_PUBLIC_ATTACHMENTS_ATTACHMENT_STORE_HANDLE_H_
+#ifndef SYNC_API_ATTACHMENTS_FAKE_ATTACHMENT_STORE_H_
+#define SYNC_API_ATTACHMENTS_FAKE_ATTACHMENT_STORE_H_
 
 #include <map>
 
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/stl_util.h"
-#include "base/threading/non_thread_safe.h"
 #include "sync/api/attachments/attachment_store.h"
 #include "sync/base/sync_export.h"
 
 namespace base {
 class SequencedTaskRunner;
+class SingleThreadTaskRunner;
 }  // namespace base
 
 namespace sync_pb {
@@ -26,18 +26,14 @@ namespace syncer {
 
 class Attachment;
 
-// AttachmentStoreHandle is helper to post AttachmentStore calls to backend on
-// different thread. Backend is expected to know on which thread to post
-// callbacks with results.
-// AttachmentStoreHandle takes ownership of backend. Backend is deleted on
-// backend thread.
-// AttachmentStoreHandle is not thread safe, it should only be accessed from
-// model thread.
-class SYNC_EXPORT AttachmentStoreHandle : public AttachmentStore,
-                                          public base::NonThreadSafe {
+// A in-memory only implementation of AttachmentStore used for testing.
+//
+// Requires that the current thread has a MessageLoop.
+class SYNC_EXPORT FakeAttachmentStore : public AttachmentStore {
  public:
-  AttachmentStoreHandle(
-      scoped_ptr<AttachmentStoreBase> backend,
+  // Construct a FakeAttachmentStore whose "IO" will be performed in
+  // |backend_task_runner|.
+  explicit FakeAttachmentStore(
       const scoped_refptr<base::SequencedTaskRunner>& backend_task_runner);
 
   // AttachmentStore implementation.
@@ -49,18 +45,16 @@ class SYNC_EXPORT AttachmentStoreHandle : public AttachmentStore,
                     const DropCallback& callback) OVERRIDE;
 
  private:
-  virtual ~AttachmentStoreHandle();
+  class Backend;
 
-  // AttachmentStoreHandle controls backend's lifetime. It is safe for
-  // AttachmentStoreHandle to bind backend through base::Unretained for posts.
-  // Backend is deleted on backend_task_runner, after that backend_ pointer is
-  // invalid.
-  scoped_ptr<AttachmentStoreBase> backend_;
+  virtual ~FakeAttachmentStore();
+
+  scoped_refptr<Backend> backend_;
   scoped_refptr<base::SequencedTaskRunner> backend_task_runner_;
 
-  DISALLOW_COPY_AND_ASSIGN(AttachmentStoreHandle);
+  DISALLOW_COPY_AND_ASSIGN(FakeAttachmentStore);
 };
 
 }  // namespace syncer
 
-#endif  // SYNC_INTERNAL_API_PUBLIC_ATTACHMENTS_ATTACHMENT_STORE_HANDLE_H_
+#endif  // SYNC_API_ATTACHMENTS_FAKE_ATTACHMENT_STORE_H_

sync/internal_api/attachments/in_memory_attachment_store_unittest.cc → sync/api/attachments/fake_attachment_store_unittest.cc

@@ -2,7 +2,7 @@
 // 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_store.h"
+#include "sync/api/attachments/fake_attachment_store.h"
 
 #include "base/bind.h"
 #include "base/memory/ref_counted_memory.h"
@@ -18,10 +18,10 @@ namespace syncer {
 const char kTestData1[] = "test data 1";
 const char kTestData2[] = "test data 2";
 
-class InMemoryAttachmentStoreTest : public testing::Test {
+class FakeAttachmentStoreTest : public testing::Test {
  protected:
   base::MessageLoop message_loop;
-  scoped_refptr<AttachmentStore> store;
+  scoped_refptr<FakeAttachmentStore> store;
   AttachmentStore::Result result;
   scoped_ptr<AttachmentMap> attachments;
   scoped_ptr<AttachmentIdList> failed_attachment_ids;
@@ -33,20 +33,18 @@ class InMemoryAttachmentStoreTest : public testing::Test {
   scoped_refptr<base::RefCountedString> some_data1;
   scoped_refptr<base::RefCountedString> some_data2;
 
-  InMemoryAttachmentStoreTest()
-      : store(AttachmentStore::CreateInMemoryStore()) {}
+  FakeAttachmentStoreTest()
+      : store(new FakeAttachmentStore(base::ThreadTaskRunnerHandle::Get())) {}
 
   virtual void SetUp() {
     Clear();
-    read_callback =
-        base::Bind(&InMemoryAttachmentStoreTest::CopyResultAttachments,
-                   base::Unretained(this),
-                   &result,
-                   &attachments,
-                   &failed_attachment_ids);
-    write_callback = base::Bind(&InMemoryAttachmentStoreTest::CopyResult,
-                                base::Unretained(this),
-                                &result);
+    read_callback = base::Bind(&FakeAttachmentStoreTest::CopyResultAttachments,
+                               base::Unretained(this),
+                               &result,
+                               &attachments,
+                               &failed_attachment_ids);
+    write_callback = base::Bind(
+        &FakeAttachmentStoreTest::CopyResult, base::Unretained(this), &result);
     drop_callback = write_callback;
 
     some_data1 = new base::RefCountedString;
@@ -88,7 +86,7 @@ class InMemoryAttachmentStoreTest : public testing::Test {
 
 // Verify that we do not overwrite existing attachments and that we do not treat
 // it as an error.
-TEST_F(InMemoryAttachmentStoreTest, Write_NoOverwriteNoError) {
+TEST_F(FakeAttachmentStoreTest, Write_NoOverwriteNoError) {
   // Create two attachments with the same id but different data.
   Attachment attachment1 = Attachment::Create(some_data1);
   Attachment attachment2 =
@@ -122,7 +120,7 @@ TEST_F(InMemoryAttachmentStoreTest, Write_NoOverwriteNoError) {
 }
 
 // Verify that we can write some attachments and read them back.
-TEST_F(InMemoryAttachmentStoreTest, Write_RoundTrip) {
+TEST_F(FakeAttachmentStoreTest, Write_RoundTrip) {
   Attachment attachment1 = Attachment::Create(some_data1);
   Attachment attachment2 = Attachment::Create(some_data2);
   AttachmentList some_attachments;
@@ -152,7 +150,7 @@ TEST_F(InMemoryAttachmentStoreTest, Write_RoundTrip) {
 }
 
 // Try to read two attachments when only one exists.
-TEST_F(InMemoryAttachmentStoreTest, Read_OneNotFound) {
+TEST_F(FakeAttachmentStoreTest, Read_OneNotFound) {
   Attachment attachment1 = Attachment::Create(some_data1);
   Attachment attachment2 = Attachment::Create(some_data2);
 
@@ -178,7 +176,7 @@ TEST_F(InMemoryAttachmentStoreTest, Read_OneNotFound) {
 
 // Try to drop two attachments when only one exists. Verify that no error occurs
 // and that the existing attachment was dropped.
-TEST_F(InMemoryAttachmentStoreTest, Drop_DropTwoButOnlyOneExists) {
+TEST_F(FakeAttachmentStoreTest, Drop_DropTwoButOnlyOneExists) {
   // First, create two attachments.
   Attachment attachment1 = Attachment::Create(some_data1);
   Attachment attachment2 = Attachment::Create(some_data2);
@@ -225,7 +223,7 @@ TEST_F(InMemoryAttachmentStoreTest, Drop_DropTwoButOnlyOneExists) {
 
 // Verify that attempting to drop an attachment that does not exist is not an
 // error.
-TEST_F(InMemoryAttachmentStoreTest, Drop_DoesNotExist) {
+TEST_F(FakeAttachmentStoreTest, Drop_DoesNotExist) {
   Attachment attachment1 = Attachment::Create(some_data1);
   AttachmentList some_attachments;
   some_attachments.push_back(attachment1);

sync/internal_api/attachments/attachment_service_impl.cc

@@ -11,6 +11,7 @@
 #include "base/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "sync/api/attachments/attachment.h"
+#include "sync/api/attachments/fake_attachment_store.h"
 #include "sync/internal_api/public/attachments/fake_attachment_downloader.h"
 #include "sync/internal_api/public/attachments/fake_attachment_uploader.h"
 
@@ -144,8 +145,8 @@ AttachmentServiceImpl::~AttachmentServiceImpl() {
 
 // Static.
 scoped_ptr<syncer::AttachmentService> AttachmentServiceImpl::CreateForTest() {
-  scoped_refptr<syncer::AttachmentStore> attachment_store =
-      AttachmentStore::CreateInMemoryStore();
+  scoped_refptr<syncer::AttachmentStore> attachment_store(
+      new syncer::FakeAttachmentStore(base::ThreadTaskRunnerHandle::Get()));
   scoped_ptr<AttachmentUploader> attachment_uploader(
       new FakeAttachmentUploader);
   scoped_ptr<AttachmentDownloader> attachment_downloader(

sync/internal_api/attachments/attachment_service_impl_unittest.cc

@@ -16,8 +16,6 @@
 
 namespace syncer {
 
-namespace {
-
 class MockAttachmentStore : public AttachmentStore,
                             public base::SupportsWeakPtr<MockAttachmentStore> {
  public:
@@ -152,8 +150,6 @@ class MockAttachmentUploader
   DISALLOW_COPY_AND_ASSIGN(MockAttachmentUploader);
 };
 
-}  // namespace
-
 class AttachmentServiceImplTest : public testing::Test,
                                   public AttachmentService::Delegate {
  protected:

sync/internal_api/attachments/attachment_store_handle.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/internal_api/public/attachments/attachment_store_handle.h"
-
-#include "base/bind.h"
-#include "base/location.h"
-#include "base/sequenced_task_runner.h"
-#include "sync/api/attachments/attachment.h"
-
-namespace syncer {
-
-AttachmentStoreHandle::AttachmentStoreHandle(
-    scoped_ptr<AttachmentStoreBase> backend,
-    const scoped_refptr<base::SequencedTaskRunner>& backend_task_runner)
-    : backend_(backend.Pass()), backend_task_runner_(backend_task_runner) {
-  DCHECK(backend_);
-  DCHECK(backend_task_runner_.get());
-}
-
-AttachmentStoreHandle::~AttachmentStoreHandle() {
-  DCHECK(backend_);
-  backend_task_runner_->DeleteSoon(FROM_HERE, backend_.release());
-}
-
-void AttachmentStoreHandle::Read(const AttachmentIdList& ids,
-                                 const ReadCallback& callback) {
-  DCHECK(CalledOnValidThread());
-  backend_task_runner_->PostTask(FROM_HERE,
-                                 base::Bind(&AttachmentStoreBase::Read,
-                                            base::Unretained(backend_.get()),
-                                            ids,
-                                            callback));
-}
-
-void AttachmentStoreHandle::Write(const AttachmentList& attachments,
-                                  const WriteCallback& callback) {
-  DCHECK(CalledOnValidThread());
-  backend_task_runner_->PostTask(FROM_HERE,
-                                 base::Bind(&AttachmentStoreBase::Write,
-                                            base::Unretained(backend_.get()),
-                                            attachments,
-                                            callback));
-}
-
-void AttachmentStoreHandle::Drop(const AttachmentIdList& ids,
-                                 const DropCallback& callback) {
-  DCHECK(CalledOnValidThread());
-  backend_task_runner_->PostTask(FROM_HERE,
-                                 base::Bind(&AttachmentStoreBase::Drop,
-                                            base::Unretained(backend_.get()),
-                                            ids,
-                                            callback));
-}
-
-}  // namespace syncer

sync/internal_api/attachments/attachment_store_handle_unittest.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/internal_api/public/attachments/attachment_store_handle.h"
-
-#include "base/bind.h"
-#include "base/callback.h"
-#include "base/memory/ref_counted.h"
-#include "base/memory/scoped_ptr.h"
-#include "base/message_loop/message_loop.h"
-#include "base/run_loop.h"
-#include "base/thread_task_runner_handle.h"
-#include "sync/api/attachments/attachment.h"
-#include "sync/api/attachments/attachment_id.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace syncer {
-
-namespace {
-
-class MockAttachmentStore : public AttachmentStoreBase {
- public:
-  MockAttachmentStore(const base::Closure& read_called,
-                      const base::Closure& write_called,
-                      const base::Closure& drop_called,
-                      const base::Closure& dtor_called)
-      : read_called_(read_called),
-        write_called_(write_called),
-        drop_called_(drop_called),
-        dtor_called_(dtor_called) {}
-
-  virtual ~MockAttachmentStore() {
-    dtor_called_.Run();
-  }
-
-  virtual void Read(const AttachmentIdList& ids,
-                    const ReadCallback& callback) OVERRIDE {
-    read_called_.Run();
-  }
-
-  virtual void Write(const AttachmentList& attachments,
-                     const WriteCallback& callback) OVERRIDE {
-    write_called_.Run();
-  }
-
-  virtual void Drop(const AttachmentIdList& ids,
-                    const DropCallback& callback) OVERRIDE {
-    drop_called_.Run();
-  }
-
-  base::Closure read_called_;
-  base::Closure write_called_;
-  base::Closure drop_called_;
-  base::Closure dtor_called_;
-};
-
-}  // namespace
-
-class AttachmentStoreHandleTest : public testing::Test {
- protected:
-  AttachmentStoreHandleTest()
-      : read_call_count_(0),
-        write_call_count_(0),
-        drop_call_count_(0),
-        dtor_call_count_(0) {}
-
-  virtual void SetUp() {
-    scoped_ptr<AttachmentStoreBase> backend(new MockAttachmentStore(
-        base::Bind(&AttachmentStoreHandleTest::ReadCalled,
-                   base::Unretained(this)),
-        base::Bind(&AttachmentStoreHandleTest::WriteCalled,
-                   base::Unretained(this)),
-        base::Bind(&AttachmentStoreHandleTest::DropCalled,
-                   base::Unretained(this)),
-        base::Bind(&AttachmentStoreHandleTest::DtorCalled,
-                   base::Unretained(this))));
-    attachment_store_handle_ = new AttachmentStoreHandle(
-        backend.Pass(), base::ThreadTaskRunnerHandle::Get());
-  }
-
-  static void ReadDone(const AttachmentStore::Result& result,
-                       scoped_ptr<AttachmentMap> attachments,
-                       scoped_ptr<AttachmentIdList> unavailable_attachments) {
-    NOTREACHED();
-  }
-
-  static void WriteDone(const AttachmentStore::Result& result) {
-    NOTREACHED();
-  }
-
-  static void DropDone(const AttachmentStore::Result& result) {
-    NOTREACHED();
-  }
-
-  void ReadCalled() { ++read_call_count_; }
-
-  void WriteCalled() { ++write_call_count_; }
-
-  void DropCalled() { ++drop_call_count_; }
-
-  void DtorCalled() { ++dtor_call_count_; }
-
-  void RunMessageLoop() {
-    base::RunLoop run_loop;
-    run_loop.RunUntilIdle();
-  }
-
-  base::MessageLoop message_loop_;
-  scoped_refptr<AttachmentStoreHandle> attachment_store_handle_;
-  int read_call_count_;
-  int write_call_count_;
-  int drop_call_count_;
-  int dtor_call_count_;
-};
-
-// Test that method calls are forwarded to backend loop
-TEST_F(AttachmentStoreHandleTest, MethodsCalled) {
-  AttachmentIdList ids;
-  AttachmentList attachments;
-
-  attachment_store_handle_->Read(
-      ids, base::Bind(&AttachmentStoreHandleTest::ReadDone));
-  EXPECT_EQ(read_call_count_, 0);
-  RunMessageLoop();
-  EXPECT_EQ(read_call_count_, 1);
-
-  attachment_store_handle_->Write(
-      attachments, base::Bind(&AttachmentStoreHandleTest::WriteDone));
-  EXPECT_EQ(write_call_count_, 0);
-  RunMessageLoop();
-  EXPECT_EQ(write_call_count_, 1);
-
-  attachment_store_handle_->Drop(
-      ids, base::Bind(&AttachmentStoreHandleTest::DropDone));
-  EXPECT_EQ(drop_call_count_, 0);
-  RunMessageLoop();
-  EXPECT_EQ(drop_call_count_, 1);
-
-  // Releasing referehce to AttachmentStoreHandle should result in
-  // MockAttachmentStore being deleted on backend loop.
-  attachment_store_handle_ = NULL;
-  EXPECT_EQ(dtor_call_count_, 0);
-  RunMessageLoop();
-  EXPECT_EQ(dtor_call_count_, 1);
-}
-
-}  // namespace syncer

sync/internal_api/public/attachments/in_memory_attachment_store.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_INTERNAL_API_PUBLIC_ATTACHMENTS_IN_MEMORY_ATTACHMENT_STORE_H_
-#define SYNC_INTERNAL_API_PUBLIC_ATTACHMENTS_IN_MEMORY_ATTACHMENT_STORE_H_
-
-#include "base/memory/ref_counted.h"
-#include "base/single_thread_task_runner.h"
-#include "base/threading/non_thread_safe.h"
-#include "sync/api/attachments/attachment.h"
-#include "sync/api/attachments/attachment_id.h"
-#include "sync/api/attachments/attachment_store.h"
-#include "sync/base/sync_export.h"
-
-namespace syncer {
-
-// An in-memory implementation of AttachmentStore used for testing.
-// InMemoryAttachmentStore is not threadsafe, it lives on backend thread and
-// posts callbacks with results on |callback_task_runner|.
-class SYNC_EXPORT InMemoryAttachmentStore : public AttachmentStoreBase,
-                                            public base::NonThreadSafe {
- public:
-  InMemoryAttachmentStore(
-      const scoped_refptr<base::SingleThreadTaskRunner>& callback_task_runner);
-  virtual ~InMemoryAttachmentStore();
-
-  // AttachmentStoreBase implementation.
-  virtual void Read(const AttachmentIdList& ids,
-                    const ReadCallback& callback) OVERRIDE;
-  virtual void Write(const AttachmentList& attachments,
-                     const WriteCallback& callback) OVERRIDE;
-  virtual void Drop(const AttachmentIdList& ids,
-                    const DropCallback& callback) OVERRIDE;
-
- private:
-  scoped_refptr<base::SingleThreadTaskRunner> callback_task_runner_;
-  AttachmentMap attachments_;
-};
-
-}  // namespace syncer
-
-#endif  // SYNC_INTERNAL_API_PUBLIC_ATTACHMENTS_IN_MEMORY_ATTACHMENT_STORE_H_

sync/sync.gyp

@@ -63,6 +63,8 @@
         'api/attachments/attachment_id.h',
         'api/attachments/attachment_store.cc',
         'api/attachments/attachment_store.h',
+        'api/attachments/fake_attachment_store.cc',
+        'api/attachments/fake_attachment_store.h',
         'api/string_ordinal.h',
         'api/sync_change.cc',
         'api/sync_change.h',
@@ -163,12 +165,10 @@
         'internal_api/attachments/attachment_service_impl.cc',
         'internal_api/attachments/attachment_service_proxy.cc',
         'internal_api/attachments/attachment_service_proxy_for_test.cc',
-        'internal_api/attachments/attachment_store_handle.cc',
         'internal_api/attachments/attachment_uploader.cc',
         'internal_api/attachments/attachment_uploader_impl.cc',
         'internal_api/attachments/fake_attachment_downloader.cc',
         'internal_api/attachments/fake_attachment_uploader.cc',
-        'internal_api/attachments/in_memory_attachment_store.cc',
         'internal_api/attachments/task_queue.cc',
         'internal_api/base_node.cc',
         'internal_api/base_transaction.cc',
@@ -196,18 +196,16 @@
         'internal_api/js_sync_manager_observer.h',
         'internal_api/protocol_event_buffer.cc',
         'internal_api/protocol_event_buffer.h',
-        'internal_api/public/attachments/attachment_downloader.h',
+        'internal_api/attachments/public/attachment_downloader.h',
+        'internal_api/attachments/public/attachment_service.h',
+        'internal_api/attachments/public/attachment_service_impl.h',
+        'internal_api/attachments/public/attachment_service_proxy_for_test.h',
+        'internal_api/attachments/public/attachment_service_proxy.h',
+        'internal_api/attachments/public/attachment_uploader.h',
         'internal_api/public/attachments/attachment_downloader_impl.h',
-        'internal_api/public/attachments/attachment_service.h',
-        'internal_api/public/attachments/attachment_service_impl.h',
-        'internal_api/public/attachments/attachment_service_proxy.h',
-        'internal_api/public/attachments/attachment_service_proxy_for_test.h',
-        'internal_api/public/attachments/attachment_store_handle.h',
-        'internal_api/public/attachments/attachment_uploader.h',
         'internal_api/public/attachments/attachment_uploader_impl.h',
         'internal_api/public/attachments/fake_attachment_downloader.h',
         'internal_api/public/attachments/fake_attachment_uploader.h',
-        'internal_api/public/attachments/in_memory_attachment_store.h',
         'internal_api/public/attachments/task_queue.h',
         'internal_api/public/base/attachment_id_proto.cc',
         'internal_api/public/base/attachment_id_proto.h',

sync/sync_tests.gypi

@@ -260,6 +260,7 @@
       'sources': [
         'api/attachments/attachment_id_unittest.cc',
         'api/attachments/attachment_unittest.cc',
+        'api/attachments/fake_attachment_store_unittest.cc',
         'api/sync_change_unittest.cc',
         'api/sync_data_unittest.cc',
         'api/sync_error_unittest.cc',
@@ -280,11 +281,9 @@
         'internal_api/attachments/attachment_downloader_impl_unittest.cc',
         'internal_api/attachments/attachment_service_impl_unittest.cc',
         'internal_api/attachments/attachment_service_proxy_unittest.cc',
-        'internal_api/attachments/attachment_store_handle_unittest.cc',
         'internal_api/attachments/attachment_uploader_impl_unittest.cc',
         'internal_api/attachments/fake_attachment_downloader_unittest.cc',
         'internal_api/attachments/fake_attachment_uploader_unittest.cc',
-        'internal_api/attachments/in_memory_attachment_store_unittest.cc',
         'internal_api/attachments/task_queue_unittest.cc',
         'internal_api/debug_info_event_listener_unittest.cc',
         'internal_api/http_bridge_unittest.cc',