Refactor FakeAttachmentStore

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}

[pavely, 2014-09-29 18:17:32]

PTAL.

Here is how classes will look like:
http://www.plantuml.com/plantuml/png/VP3D2i8m3CVlVOgS1ziJU75uyJ0Gz0LY6ntgcb4Z...

[maniscalco, 2014-09-29 20:57:53]

Nice work.  Here's my feedback.  Some of this we talked about in person already.

https://codereview.chromium.org/601553004/diff/1/sync/api/attachments/attachm...
File sync/api/attachments/attachment_store.h (right):

https://codereview.chromium.org/601553004/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_store.h:28: class SYNC_EXPORT
AttachmentStoreBase {
Can/should ASB be made SYNC_PRIVATE?

https://codereview.chromium.org/601553004/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_store.h:87: // AttachmentStore is an interface
exposed to device type and AttachmentService
s/device/data/

https://codereview.chromium.org/601553004/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_store.h:90: public
base::RefCounted<AttachmentStore> {
I would be inclined to use RefCountedThreadSafe (here and elsewhere) instead of
just RC.  Here's my thinking.  Given how this type is used, the cost of mutexes
or atomic inc/dec in AddRef/Release is negligible.  I *think* RC might be good
enough for our uses, but there's a fair chance we may need to use RCTS in the
future and it would be easy to forget to upgrade from RC to RCTS.

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/attachment...
File sync/internal_api/attachments/attachment_store_proxy.cc (right):

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/attachment_store_proxy.cc:18: DCHECK(backend_);
DCHECK on task runner as well?

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/attachment_store_proxy.cc:29: DCHECK(backend_);
Since the ctor guarantees backend_ starts off as non-null and it's only nulled
in the dtor, I think it's probably fine to omit DCHECK(backend_) in the other
methods.

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
File sync/internal_api/public/attachments/attachment_store_proxy.h (right):

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/attachment_store_proxy.h:29: //
AttachmentStoreproxy is helper to post AttachmentStore calls to backend on
s/Storeproxy/StoreProxy/

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/attachment_store_proxy.h:32: //
AttachmentStoreProxy takes ownership of backend. Backend is deleted by
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).

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/attachment_store_proxy.h:34: class
SYNC_EXPORT AttachmentStoreProxy : public AttachmentStore,
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.

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/attachment_store_proxy.h:37:
AttachmentStoreProxy(
nit: I think the style guide would have us pass backend_task_runner first
because it's const-ref.

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/attachment_store_proxy.h:37:
AttachmentStoreProxy(
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).

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/attachment_store_proxy.h:37:
AttachmentStoreProxy(
It's worth documenting that this class promises to call and destroy the backend
in the backend_task_runner.

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
File sync/internal_api/public/attachments/in_memory_attachment_store.h (right):

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/in_memory_attachment_store.h:5: #ifndef
SYNC_INTERNAL_API_ATTACHMENTS_IN_MEMORY_ATTACHMENT_STORE_H_
There should be a PUBLIC_ in here, right?

https://codereview.chromium.org/601553004/diff/1/sync/sync.gyp
File sync/sync.gyp (left):

https://codereview.chromium.org/601553004/diff/1/sync/sync.gyp#oldcode199
sync/sync.gyp:199: 'internal_api/attachments/public/attachment_downloader.h',
Good catch.  I'm surprised build/gyp_chromium didn't catch it.

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

PTAL

https://codereview.chromium.org/601553004/diff/1/sync/api/attachments/attachm...
File sync/api/attachments/attachment_store.h (right):

https://codereview.chromium.org/601553004/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_store.h:28: class SYNC_EXPORT
AttachmentStoreBase {
It is base class for AttachmentStore which should be accessible from
tests/consumers, I don't think it should be SYNC_EXPORT_PRIVATE.
On 2014/09/29 20:57:52, maniscalco wrote:
> Can/should ASB be made SYNC_PRIVATE?

https://codereview.chromium.org/601553004/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_store.h:87: // AttachmentStore is an interface
exposed to device type and AttachmentService
On 2014/09/29 20:57:52, maniscalco wrote:
> s/device/data/

Done.

https://codereview.chromium.org/601553004/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_store.h:90: public
base::RefCounted<AttachmentStore> {
On 2014/09/29 20:57:52, maniscalco wrote:
> I would be inclined to use RefCountedThreadSafe (here and elsewhere) instead
of
> just RC.  Here's my thinking.  Given how this type is used, the cost of
mutexes
> or atomic inc/dec in AddRef/Release is negligible.  I *think* RC might be good
> enough for our uses, but there's a fair chance we may need to use RCTS in the
> future and it would be easy to forget to upgrade from RC to RCTS.

Done.

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/attachment...
File sync/internal_api/attachments/attachment_store_proxy.cc (right):

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/attachment_store_proxy.cc:18: DCHECK(backend_);
On 2014/09/29 20:57:52, maniscalco wrote:
> DCHECK on task runner as well?

Done.

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/attachment_store_proxy.cc:29: DCHECK(backend_);
On 2014/09/29 20:57:52, maniscalco wrote:
> Since the ctor guarantees backend_ starts off as non-null and it's only nulled
> in the dtor, I think it's probably fine to omit DCHECK(backend_) in the other
> methods.

Done.

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
File sync/internal_api/public/attachments/attachment_store_proxy.h (right):

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/attachment_store_proxy.h:29: //
AttachmentStoreproxy is helper to post AttachmentStore calls to backend on
On 2014/09/29 20:57:52, maniscalco wrote:
> s/Storeproxy/StoreProxy/

Done.

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/attachment_store_proxy.h:32: //
AttachmentStoreProxy takes ownership of backend. Backend is deleted by
On 2014/09/29 20:57:52, maniscalco wrote:
> 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).

Done.

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/attachment_store_proxy.h:34: class
SYNC_EXPORT AttachmentStoreProxy : public AttachmentStore,
On 2014/09/29 20:57:52, maniscalco wrote:
> 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.

Done.

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/attachment_store_proxy.h:37:
AttachmentStoreProxy(
On 2014/09/29 20:57:52, maniscalco wrote:
> 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).

Done.

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/attachment_store_proxy.h:37:
AttachmentStoreProxy(
On 2014/09/29 20:57:52, maniscalco wrote:
> It's worth documenting that this class promises to call and destroy the
backend
> in the backend_task_runner.

Done.

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/attachment_store_proxy.h:37:
AttachmentStoreProxy(
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:
> nit: I think the style guide would have us pass backend_task_runner first
> because it's const-ref.

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
File sync/internal_api/public/attachments/in_memory_attachment_store.h (right):

https://codereview.chromium.org/601553004/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/in_memory_attachment_store.h:5: #ifndef
SYNC_INTERNAL_API_ATTACHMENTS_IN_MEMORY_ATTACHMENT_STORE_H_
On 2014/09/29 20:57:52, maniscalco wrote:
> There should be a PUBLIC_ in here, right?

Done.

[maniscalco, 2014-09-30 00:26:13]

LGTM with one nit.

https://codereview.chromium.org/601553004/diff/1/sync/api/attachments/attachm...
File sync/api/attachments/attachment_store.h (right):

https://codereview.chromium.org/601553004/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_store.h:28: class SYNC_EXPORT
AttachmentStoreBase {
On 2014/09/29 23:27:08, pavely wrote:
> It is base class for AttachmentStore which should be accessible from
> tests/consumers, I don't think it should be SYNC_EXPORT_PRIVATE.
> On 2014/09/29 20:57:52, maniscalco wrote:
> > Can/should ASB be made SYNC_PRIVATE?
> 

You're right, I agree.

https://codereview.chromium.org/601553004/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_service_impl_unittest.cc (right):

https://codereview.chromium.org/601553004/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_service_impl_unittest.cc:156: class
AttachmentServiceImplTest : public testing::Test,
nit: new line above?

[pavely, 2014-09-30 00:30:38]

https://codereview.chromium.org/601553004/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_service_impl_unittest.cc (right):

https://codereview.chromium.org/601553004/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_service_impl_unittest.cc:156: class
AttachmentServiceImplTest : public testing::Test,
On 2014/09/30 00:26:13, maniscalco wrote:
> nit: new line above?

Done.

[pavely, 2014-09-30 00:30:46]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2014-09-30 00:31:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/601553004/40001

[commit-bot: I haz the power, 2014-09-30 01:58:06]

Committed patchset #3 (id:40001) as fe15e6882cf934d93691953d83510059b2afac2f

[commit-bot: I haz the power, 2014-09-30 01:58:48]

Patchset 3 (id:??) landed as
https://crrev.com/6ae2b341c73909cf902078446346d6c900273409
Cr-Commit-Position: refs/heads/master@{#297339}

[sergeyv, 2014-09-30 07:47:10]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/619563002/ by sergeyv@chromium.org.

The reason for reverting is: Speculative revert:
looks like it causes failures on the Linux asan:
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....