Move AttachmentStore ownership to datatype

Move AttachmentStore ownership to datatype

With this change attachment store ownership is moved to datatype. If 
datatype wants to use attachments it needs to instantiate attachment 
store and return it to sync via GetAttachmentStore.
GenericChangeProcessorFactory calls SyncableService::GetAttachmentStore 
and passes attachment_store through GenericChangeProcessor::ctor to 
AttachmentService::ctor.
GetAttachmentStore returning NULL means datatype doesn\u2019t use attachments, 
therefore GenericChangeProcessor doesn\u2019t create AttachmentService.

R=maniscalco@chromium.org
BUG=412070
Committed: https://crrev.com/e39fad3ae70d872f045afb6230d0cff54f63eafe
Cr-Commit-Position: refs/heads/master@{#294070}

[pavely, 2014-09-08 21:24:32]

Nick, please take a look.
I've broken this change into two parts, One related to changes in
SyncableService and co, another one to not create AttachmentService when it is
not needed. This way it is easier to revert/redo second one if you don't like
it.

[maniscalco, 2014-09-08 21:56:35]

Nice work!  Here's my feedback on patch sets #1 and #2.

Also, can you wrap the submit notes?  My understanding is that submit notes
should be wrapped to 72 columns.

https://codereview.chromium.org/548373003/diff/1/components/sync_driver/gener...
File components/sync_driver/generic_change_processor_factory.cc (right):

https://codereview.chromium.org/548373003/diff/1/components/sync_driver/gener...
components/sync_driver/generic_change_processor_factory.cc:31:
local_service->GetAttachmentStore())).Pass();
Can you confirm that this method (CreateGenericChangeProcess) is called on the
datatype's thread?  I believe it is, but we should make sure since we're
dereferencing the local_service WeakPtr.

https://codereview.chromium.org/548373003/diff/1/components/sync_driver/gener...
components/sync_driver/generic_change_processor_factory.cc:31:
local_service->GetAttachmentStore())).Pass();
Any situations where local_service could be null at this point?  If so, we
should probably be checking for null and passing in a null
scoped_refptr<AttachmentStore>.

https://codereview.chromium.org/548373003/diff/1/components/sync_driver/sync_...
File components/sync_driver/sync_api_component_factory.h (right):

https://codereview.chromium.org/548373003/diff/1/components/sync_driver/sync_...
components/sync_driver/sync_api_component_factory.h:35: const
scoped_refptr<syncer::AttachmentStore>& attachment_store,
nit: Document the attachment_store parameter.

https://codereview.chromium.org/548373003/diff/1/sync/api/syncable_service.h
File sync/api/syncable_service.h (right):

https://codereview.chromium.org/548373003/diff/1/sync/api/syncable_service.h#...
sync/api/syncable_service.h:75: // attachemnts should create attachment store
and implement GetAttachmentStore
typo: attachemnts

https://codereview.chromium.org/548373003/diff/1/sync/api/syncable_service.h#...
sync/api/syncable_service.h:77: virtual scoped_refptr<AttachmentStore>
GetAttachmentStore();
I imagine an implementer of a datatype would wonder about which threads might
invoke the dtor of the attachment store.  Can we (sync) guarantee that we will
only clear scoped_refptr's to AttachmentStore in the datatype's thread?  If so
let's document it here.

https://codereview.chromium.org/548373003/diff/20001/components/sync_driver/g...
File components/sync_driver/generic_change_processor.cc (right):

https://codereview.chromium.org/548373003/diff/20001/components/sync_driver/g...
components/sync_driver/generic_change_processor.cc:102: if (attachment_store) {
I thought scoped_refptr no longer automatically converted to *.  I *think*
you'll need to call .get() explicitly.

https://codereview.chromium.org/548373003/diff/20001/components/sync_driver/g...
components/sync_driver/generic_change_processor.cc:476:
StoreAttachments(attachment_service_.get(), new_attachments);
If we get to this point and attachment_service_ is null, I think we have a
problem.  WDYT about triggering an unrecoverable error here?

https://codereview.chromium.org/548373003/diff/20001/components/sync_driver/g...
File components/sync_driver/generic_change_processor.h (right):

https://codereview.chromium.org/548373003/diff/20001/components/sync_driver/g...
components/sync_driver/generic_change_processor.h:54: const
scoped_refptr<syncer::AttachmentStore>& attachment_store);
nit: Document that attachment_store can be null.

https://codereview.chromium.org/548373003/diff/20001/components/sync_driver/g...
components/sync_driver/generic_change_processor.h:153:
scoped_ptr<syncer::AttachmentService> attachment_service_;
Can you note that attaachment_service_ may be null.

https://codereview.chromium.org/548373003/diff/20001/components/sync_driver/g...
components/sync_driver/generic_change_processor.h:156:
scoped_ptr<base::WeakPtrFactory<syncer::AttachmentService> >
It's probably worth noting that attachment_service_weak_ptr_factory_ itself may
be null.

[pavely, 2014-09-09 23:52:31]

PTAL

https://codereview.chromium.org/548373003/diff/20001/components/sync_driver/g...
File components/sync_driver/generic_change_processor.cc (right):

https://codereview.chromium.org/548373003/diff/20001/components/sync_driver/g...
components/sync_driver/generic_change_processor.cc:102: if (attachment_store) {
On 2014/09/08 21:56:35, maniscalco wrote:
> I thought scoped_refptr no longer automatically converted to *.  I *think*
> you'll need to call .get() explicitly.

Done.

https://codereview.chromium.org/548373003/diff/20001/components/sync_driver/g...
components/sync_driver/generic_change_processor.cc:476:
StoreAttachments(attachment_service_.get(), new_attachments);
On 2014/09/08 21:56:35, maniscalco wrote:
> If we get to this point and attachment_service_ is null, I think we have a
> problem.  WDYT about triggering an unrecoverable error here?

Done.

https://codereview.chromium.org/548373003/diff/20001/components/sync_driver/g...
File components/sync_driver/generic_change_processor.h (right):

https://codereview.chromium.org/548373003/diff/20001/components/sync_driver/g...
components/sync_driver/generic_change_processor.h:54: const
scoped_refptr<syncer::AttachmentStore>& attachment_store);
On 2014/09/08 21:56:35, maniscalco wrote:
> nit: Document that attachment_store can be null.

Done.

https://codereview.chromium.org/548373003/diff/20001/components/sync_driver/g...
components/sync_driver/generic_change_processor.h:153:
scoped_ptr<syncer::AttachmentService> attachment_service_;
On 2014/09/08 21:56:35, maniscalco wrote:
> Can you note that attaachment_service_ may be null.

Done.

https://codereview.chromium.org/548373003/diff/20001/components/sync_driver/g...
components/sync_driver/generic_change_processor.h:156:
scoped_ptr<base::WeakPtrFactory<syncer::AttachmentService> >
On 2014/09/08 21:56:35, maniscalco wrote:
> It's probably worth noting that attachment_service_weak_ptr_factory_ itself
may
> be null.

Done.

[maniscalco, 2014-09-09 23:59:39]

lgtm

[pavely, 2014-09-10 00:26:08]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2014-09-10 00:30:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pavely@chromium.org/548373003/40001

[commit-bot: I haz the power, 2014-09-10 01:43:38]

Committed patchset #3 (id:40001) as e733afbf527bb6ece11a511245ce77fe06335291

[commit-bot: I haz the power, 2014-09-10 03:57:34]

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