Refactor FakeAttachmentStore

sync/internal_api/public/attachments/attachment_store_proxy.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_ATTACHMENT_STORE_PROXY_H_
+#define SYNC_INTERNAL_API_PUBLIC_ATTACHMENTS_ATTACHMENT_STORE_PROXY_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;
+}  // namespace base
+
+namespace sync_pb {
+class AttachmentId;
+}  // namespace sync_pb
+
+namespace syncer {
+
+class Attachment;
+
+// AttachmentStoreproxy is helper to post AttachmentStore calls to backend on

[maniscalco, 2014/09/29 20:57:52]

s/Storeproxy/StoreProxy/

[pavely, 2014/09/29 23:27:08]

Done.

+// different thread. Backend is expected to know on which thread to post
+// callbacks with results.
+// AttachmentStoreProxy takes ownership of backend. Backend is deleted by

[maniscalco, 2014/09/29 20:57:52]

I'd remove the mention of using DeleteSoon as it's an implementation detail.  I
do, however, think it's worth mentioning that the backend will be destroyed on
the backend_task_runner (either here in the class comment or in the ctor comment
where backend is passed).

[pavely, 2014/09/29 23:27:08]

Done.

+// posting DeleteSoon on backend thread.
+class SYNC_EXPORT AttachmentStoreProxy : public AttachmentStore,

[maniscalco, 2014/09/29 20:57:52]

Consider adding a unit test for this class.  Sure, it's not gonna test much now,
but if this class expands to include more test-worthy functionality, it will be
easier for a future maintainer to do the right thing.

[pavely, 2014/09/29 23:27:08]

Done.

+                                         public base::NonThreadSafe {
+ public:
+  AttachmentStoreProxy(

[maniscalco, 2014/09/29 20:57:52]

As we discussed, AttachmentStoreProxy is similar to, but different from
AttachmentServiceProxy.  Consider renaming AttachmentStoreProxy to something
else (Handle? Wrapper? Forwarder?) so it's clear that it's has different
semantics.

Two key differences worth pointing out in AttachmentStoreFoo's class comment:

1. StoreFoo owns the backend (ServiceProxy does not).

2. An instance of StoreFoo must be accessed from only one thread (an instance of
SerivceProxy can be accessed from any thread).

[maniscalco, 2014/09/29 20:57:52]

It's worth documenting that this class promises to call and destroy the backend
in the backend_task_runner.

[maniscalco, 2014/09/29 20:57:52]

nit: I think the style guide would have us pass backend_task_runner first
because it's const-ref.

[pavely, 2014/09/29 23:27:08]

Done.

[pavely, 2014/09/29 23:27:08]

No, it is not that strict.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para...
The rules are: input then output. IMO it is more logical to pass backend before
its' task_runner.
On 2014/09/29 20:57:52, maniscalco wrote:

[pavely, 2014/09/29 23:27:08]

Done.

+      scoped_ptr<AttachmentStoreBase> backend,
+      const scoped_refptr<base::SequencedTaskRunner>& backend_task_runner);
+
+  // AttachmentStore implementation.
+  virtual void Read(const AttachmentIdList& id,
+                    const ReadCallback& callback) OVERRIDE;
+  virtual void Write(const AttachmentList& attachments,
+                     const WriteCallback& callback) OVERRIDE;
+  virtual void Drop(const AttachmentIdList& id,
+                    const DropCallback& callback) OVERRIDE;
+
+ private:
+  virtual ~AttachmentStoreProxy();
+
+  // AttachmentStoreProxy controls backend's lifetime. It is safe for
+  // AttachmentStoreProxy to bind backend through base::Unretained for posts.
+  // Backend is deleted by posting DeleteSoon on backend_task_runner, after this
+  // backend_ pointer is invalid.
+  scoped_ptr<AttachmentStoreBase> backend_;
+  scoped_refptr<base::SequencedTaskRunner> backend_task_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(AttachmentStoreProxy);
+};
+
+}  // namespace syncer
+
+#endif  // SYNC_INTERNAL_API_PUBLIC_ATTACHMENTS_ATTACHMENT_STORE_PROXY_H_