Update sync API to support attachments.

Update sync API to support attachments.

The purpose of this change is to start exposing attachment support in
the sync API. At this point there is no real functionality added.

SyncData and SyncChangeProcessor now know about attachments.

GenericChangeProcessor's ctor now requires an AttachmentService.

Add AttachmentService and FakeAttachmentService.

Update Attachment::Create* methods to return by value.

Make Attachment::CreateId public because it is useful in
sync_data_unittest.cc.

Make some ctors explicit.

BUG=348624
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260778

[maniscalco, 2014-03-06 00:48:23]

Hi Tim, mind reviewing this change?  As you'll see there are a bunch of TODOs in
the code.  I plan to resolve many of them before committing.

[tim (not reviewing), 2014-03-10 22:55:57]

https://codereview.chromium.org/187303006/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/generic_change_processor.cc (right):

https://codereview.chromium.org/187303006/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/generic_change_processor.cc:78:
attachment_service_weak_handle_(
Do we actually need a weak_handle_ member? Is attachment_service safe?

https://codereview.chromium.org/187303006/diff/20001/sync/api/attachments/att...
File sync/api/attachments/attachment_service.h (right):

https://codereview.chromium.org/187303006/diff/20001/sync/api/attachments/att...
sync/api/attachments/attachment_service.h:73: #endif  //
SYNC_API_ATTACHMENTS_ATTACHMENT_SERVaICE_H_
did this pass presubmit?  SERVaICE_H

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_change_pro...
File sync/api/sync_change_processor.h (right):

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_change_pro...
sync/api/sync_change_processor.h:53: // |callback|. If no such SyncData exists,
IsValid on the SyncData passed to
"... invokes |callback| asynchronously."

Also, what does "does not exist" mean here?  Locally?

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_change_pro...
sync/api/sync_change_processor.h:59: // implentation is provide here just to
verify that everything compiles.
'implementation is provided'

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_change_pro...
sync/api/sync_change_processor.h:61: virtual void GetSyncData(const ModelType&
type,
I want to call this GetSyncDataAsync, but Sync/Async, sigh.

Maybe (Request | Lookup)SyncData(...) is better. Comment on whether this is a
local only operation?

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_change_pro...
sync/api/sync_change_processor.h:62: const std::string& sync_tag,
Comment for sync_tag, refer to sync_data.h for details.

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.cc
File sync/api/sync_data.cc (right):

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.cc#ne...
sync/api/sync_data.cc:233: attachment_service_.Call(
As much as I do like the succinct-ness of WeakHandle I think we're starting to
move away from it in favor of DIY (with a task_runner + weak ptr) for
consistency with the rest of the codebase. 

But yeah, having to define your own AttServProxy object that posts via
TaskRunner to a ref-counted core object with a WeakPtr to the AttachmentService
is a bit of a pain :\

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.h
File sync/api/sync_data.h (right):

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:67: // the 4-arg version.
For stuff like this, a recommended chromium convention is to file a bug to track
the TODO.

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:68: //
Remove the trailing // line.

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:127: void GetAttachments(const AttachmentIdList&
attachment_ids,
As in the other file, I'm wondering if we should establish a convention to
disambiguate between "downloading" and "looking up" when we say "Get" and are
referring to an Attachment. Perhaps "Download" and "Lookup", where Lookup is
always taken to mean looking-up in a local index, never triggering any server
downloading.  If Lookup fails, items may exist somewhere, just not on this
client (yet). 

Although from the comment neither of those would apply for this method, would
they.. so maybe Get is the right call for the hybrid cases... more below.

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:138: void DropAttachments(const AttachmentIdList&
attachment_ids,
We talked about this but pointing out for posterity: At first glance, this
conflicts with the "immutability" of this class. But only if you take
"immutability" to cover the entire logical entity referred to by this SyncData
vs the physical instance of this class, and only if you take "dropping
attachment from local index" to mean an actual logical mutation of that entity,
which it really isn't.

Before, the logical and physical entities were roughly (conveniently) the same
thing, but this isn't true with attachments, and DropAttachments is where that
fact hits API in this patch. 

That works and you can argue (as I just did) that this "mutation" is justified.
An alternative that tries to make it simpler to reason about SyncData's
immutability might be to avoid putting any operations with data side-effects on
this class directly, and have a helper object (or, something like
Profile::ServiceAccessType) for when you want to modify the attachment cache.
That would work well for Drop, but less so for 'Get' in the event that you want
to actually download from server, unless we make 'Get' here be only a Lookup. 
It's also way less convenient than having Drop right here on this object, and
doesn't seem that dangerous, so I'm not sure it's worth it.

[maniscalco, 2014-03-18 20:49:18]

Thanks for the feedback!  PTAL.

https://codereview.chromium.org/187303006/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/generic_change_processor.cc (right):

https://codereview.chromium.org/187303006/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/generic_change_processor.cc:78:
attachment_service_weak_handle_(
On 2014/03/10 22:55:58, timsteele wrote:
> Do we actually need a weak_handle_ member? Is attachment_service safe?

Good question.  Since it's related to your comment in sync_data.cc, I'll reply
there.

https://codereview.chromium.org/187303006/diff/20001/sync/api/attachments/att...
File sync/api/attachments/attachment_service.h (right):

https://codereview.chromium.org/187303006/diff/20001/sync/api/attachments/att...
sync/api/attachments/attachment_service.h:73: #endif  //
SYNC_API_ATTACHMENTS_ATTACHMENT_SERVaICE_H_
On 2014/03/10 22:55:58, timsteele wrote:
> did this pass presubmit?  SERVaICE_H

Heh, unfortunately, it does pass.  Fixed.

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_change_pro...
File sync/api/sync_change_processor.h (right):

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_change_pro...
sync/api/sync_change_processor.h:53: // |callback|. If no such SyncData exists,
IsValid on the SyncData passed to
On 2014/03/10 22:55:58, timsteele wrote:
> "... invokes |callback| asynchronously."

Done.

> Also, what does "does not exist" mean here?  Locally?

That should have read "does not exist locally".  Comment updated.

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_change_pro...
sync/api/sync_change_processor.h:59: // implentation is provide here just to
verify that everything compiles.
On 2014/03/10 22:55:58, timsteele wrote:
> 'implementation is provided'

Done.

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_change_pro...
sync/api/sync_change_processor.h:61: virtual void GetSyncData(const ModelType&
type,
On 2014/03/10 22:55:58, timsteele wrote:
> I want to call this GetSyncDataAsync, but Sync/Async, sigh.
> 
> Maybe (Request | Lookup)SyncData(...) is better. Comment on whether this is a
> local only operation?

Yes, it's local only.  I've updated the comment in an effort to make that more
clear.

Re Get vs Request vs Lookup... I'm ambivalent.  On one hand would like to use an
name that has clear local/remote semantics.  On the other hand, I want
GetSyncData's name to be similar to GetAllSyncData, because they are similar
(both are local operations) and only differ in that one is synchronous and it
returns all the SyncDatas, not just one SyncData.

I don't feel strongly on the naming here.

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_change_pro...
sync/api/sync_change_processor.h:62: const std::string& sync_tag,
On 2014/03/10 22:55:58, timsteele wrote:
> Comment for sync_tag, refer to sync_data.h for details.

Done.

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.cc
File sync/api/sync_data.cc (right):

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.cc#ne...
sync/api/sync_data.cc:233: attachment_service_.Call(
On 2014/03/10 22:55:58, timsteele wrote:
> As much as I do like the succinct-ness of WeakHandle I think we're starting to
> move away from it in favor of DIY (with a task_runner + weak ptr) for
> consistency with the rest of the codebase. 
> 
> But yeah, having to define your own AttServProxy object that posts via
> TaskRunner to a ref-counted core object with a WeakPtr to the
AttachmentService
> is a bit of a pain :\

In some ways the explicit proxy object approach seems less error prone because,
to the caller, it looks like any other object (no need to remember a special way
of invoking methods in the right thread).  LMK if you agree and I'll drop my use
of WeakHandle.

Assuming I drop WeakHandle, I see a couple options.

1. Build the proxy/post functionality into FakeAttachmentService such that it's
transparent to users of FAS.  That is, FAS will look like a thread-safe
implementation of AS.

2. Leave FAS as it is now, not thread-safe.  Create an AttachmentServiceProxy
class that wraps an implementation of AS (like FAS).

I think I like #2 because it's more composable and will work with any
implementation of AS.

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.h
File sync/api/sync_data.h (right):

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:67: // the 4-arg version.
On 2014/03/10 22:55:58, timsteele wrote:
> For stuff like this, a recommended chromium convention is to file a bug to
track
> the TODO. 

Thanks for the reminder.  Done.

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:68: //
On 2014/03/10 22:55:58, timsteele wrote:
> Remove the trailing // line.

Done.

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:127: void GetAttachments(const AttachmentIdList&
attachment_ids,
On 2014/03/10 22:55:58, timsteele wrote:
> As in the other file, I'm wondering if we should establish a convention to
> disambiguate between "downloading" and "looking up" when we say "Get" and are
> referring to an Attachment. Perhaps "Download" and "Lookup", where Lookup is
> always taken to mean looking-up in a local index, never triggering any server
> downloading.  If Lookup fails, items may exist somewhere, just not on this
> client (yet). 
> 
> Although from the comment neither of those would apply for this method, would
> they.. so maybe Get is the right call for the hybrid cases... more below.

Good point.  I agree.  I would like the verb to more clearly indicate the
method's semantics.  It seems like the most important thing to convey is whether
the operation is local-only or not.  With that in mind, what do you think of
this:

1. "Get" implies local-only.  No network involved.  Why not "Lookup"?  Because
"Get" matches the semantics of SCP::GetAllSyncData and the new SCP::GetSyncData
(see also my reply to your comment in SCP).

2. "Retrieve" for when it may come from the local machine or from the network. 
Why not "Download"?  Because "Download" implies network access and this
operation may or may not actually go over the network.

How does that sound?

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:138: void DropAttachments(const AttachmentIdList&
attachment_ids,
On 2014/03/10 22:55:58, timsteele wrote:
> We talked about this but pointing out for posterity: At first glance, this
> conflicts with the "immutability" of this class. But only if you take
> "immutability" to cover the entire logical entity referred to by this SyncData
> vs the physical instance of this class, and only if you take "dropping
> attachment from local index" to mean an actual logical mutation of that
entity,
> which it really isn't.
> 
> Before, the logical and physical entities were roughly (conveniently) the same
> thing, but this isn't true with attachments, and DropAttachments is where that
> fact hits API in this patch. 
> 
> That works and you can argue (as I just did) that this "mutation" is
justified.
> An alternative that tries to make it simpler to reason about SyncData's
> immutability might be to avoid putting any operations with data side-effects
on
> this class directly, and have a helper object (or, something like
> Profile::ServiceAccessType) for when you want to modify the attachment cache.
> That would work well for Drop, but less so for 'Get' in the event that you
want
> to actually download from server, unless we make 'Get' here be only a Lookup. 
> It's also way less convenient than having Drop right here on this object, and
> doesn't seem that dangerous, so I'm not sure it's worth it.
>  

Agreed.

[tim (not reviewing), 2014-03-19 22:32:48]

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.cc
File sync/api/sync_data.cc (right):

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.cc#ne...
sync/api/sync_data.cc:233: attachment_service_.Call(
On 2014/03/18 20:49:18, maniscalco wrote:
> On 2014/03/10 22:55:58, timsteele wrote:
> > As much as I do like the succinct-ness of WeakHandle I think we're starting
to
> > move away from it in favor of DIY (with a task_runner + weak ptr) for
> > consistency with the rest of the codebase. 
> > 
> > But yeah, having to define your own AttServProxy object that posts via
> > TaskRunner to a ref-counted core object with a WeakPtr to the
> AttachmentService
> > is a bit of a pain :\
> 
> In some ways the explicit proxy object approach seems less error prone
because,
> to the caller, it looks like any other object (no need to remember a special
way
> of invoking methods in the right thread).  LMK if you agree and I'll drop my
use
> of WeakHandle.
> 
> Assuming I drop WeakHandle, I see a couple options.
> 
> 1. Build the proxy/post functionality into FakeAttachmentService such that
it's
> transparent to users of FAS.  That is, FAS will look like a thread-safe
> implementation of AS.
> 
> 2. Leave FAS as it is now, not thread-safe.  Create an AttachmentServiceProxy
> class that wraps an implementation of AS (like FAS).
> 
> I think I like #2 because it's more composable and will work with any
> implementation of AS.

I agree with everything.  At the same time I wouldn't strongly oppose keeping
WeakHandle and converting things later if we ever officially decide to drop
WeakHandle.  But if I was starting from scratch I'd probably do #2.

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.h
File sync/api/sync_data.h (right):

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:127: void GetAttachments(const AttachmentIdList&
attachment_ids,
On 2014/03/18 20:49:18, maniscalco wrote:
> On 2014/03/10 22:55:58, timsteele wrote:
> > As in the other file, I'm wondering if we should establish a convention to
> > disambiguate between "downloading" and "looking up" when we say "Get" and
are
> > referring to an Attachment. Perhaps "Download" and "Lookup", where Lookup is
> > always taken to mean looking-up in a local index, never triggering any
server
> > downloading.  If Lookup fails, items may exist somewhere, just not on this
> > client (yet). 
> > 
> > Although from the comment neither of those would apply for this method,
would
> > they.. so maybe Get is the right call for the hybrid cases... more below.
> 
> Good point.  I agree.  I would like the verb to more clearly indicate the
> method's semantics.  It seems like the most important thing to convey is
whether
> the operation is local-only or not.  With that in mind, what do you think of
> this:
> 
> 1. "Get" implies local-only.  No network involved.  Why not "Lookup"?  Because
> "Get" matches the semantics of SCP::GetAllSyncData and the new
SCP::GetSyncData
> (see also my reply to your comment in SCP).
> 
> 2. "Retrieve" for when it may come from the local machine or from the network.

> Why not "Download"?  Because "Download" implies network access and this
> operation may or may not actually go over the network.
> 
> How does that sound?

How about 'Get' as you say and 'GetOrDownload', then?

[maniscalco, 2014-03-25 21:40:30]

All comments addressed, RFAL!  Lots of changes since you last reviewed.

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.cc
File sync/api/sync_data.cc (right):

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.cc#ne...
sync/api/sync_data.cc:233: attachment_service_.Call(
On 2014/03/19 22:32:48, timsteele wrote:
> On 2014/03/18 20:49:18, maniscalco wrote:
> > On 2014/03/10 22:55:58, timsteele wrote:
> > > As much as I do like the succinct-ness of WeakHandle I think we're
starting
> to
> > > move away from it in favor of DIY (with a task_runner + weak ptr) for
> > > consistency with the rest of the codebase. 
> > > 
> > > But yeah, having to define your own AttServProxy object that posts via
> > > TaskRunner to a ref-counted core object with a WeakPtr to the
> > AttachmentService
> > > is a bit of a pain :\
> > 
> > In some ways the explicit proxy object approach seems less error prone
> because,
> > to the caller, it looks like any other object (no need to remember a special
> way
> > of invoking methods in the right thread).  LMK if you agree and I'll drop my
> use
> > of WeakHandle.
> > 
> > Assuming I drop WeakHandle, I see a couple options.
> > 
> > 1. Build the proxy/post functionality into FakeAttachmentService such that
> it's
> > transparent to users of FAS.  That is, FAS will look like a thread-safe
> > implementation of AS.
> > 
> > 2. Leave FAS as it is now, not thread-safe.  Create an
AttachmentServiceProxy
> > class that wraps an implementation of AS (like FAS).
> > 
> > I think I like #2 because it's more composable and will work with any
> > implementation of AS.
> 
> I agree with everything.  At the same time I wouldn't strongly oppose keeping
> WeakHandle and converting things later if we ever officially decide to drop
> WeakHandle.  But if I was starting from scratch I'd probably do #2.

SGTM.  I've gotten rid of WeakHandle and created AttachmentServiceProxy.  PTAL.

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.h
File sync/api/sync_data.h (right):

https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:127: void GetAttachments(const AttachmentIdList&
attachment_ids,
On 2014/03/19 22:32:48, timsteele wrote:
> On 2014/03/18 20:49:18, maniscalco wrote:
> > On 2014/03/10 22:55:58, timsteele wrote:
> > > As in the other file, I'm wondering if we should establish a convention to
> > > disambiguate between "downloading" and "looking up" when we say "Get" and
> are
> > > referring to an Attachment. Perhaps "Download" and "Lookup", where Lookup
is
> > > always taken to mean looking-up in a local index, never triggering any
> server
> > > downloading.  If Lookup fails, items may exist somewhere, just not on this
> > > client (yet). 
> > > 
> > > Although from the comment neither of those would apply for this method,
> would
> > > they.. so maybe Get is the right call for the hybrid cases... more below.
> > 
> > Good point.  I agree.  I would like the verb to more clearly indicate the
> > method's semantics.  It seems like the most important thing to convey is
> whether
> > the operation is local-only or not.  With that in mind, what do you think of
> > this:
> > 
> > 1. "Get" implies local-only.  No network involved.  Why not "Lookup"? 
Because
> > "Get" matches the semantics of SCP::GetAllSyncData and the new
> SCP::GetSyncData
> > (see also my reply to your comment in SCP).
> > 
> > 2. "Retrieve" for when it may come from the local machine or from the
network.
> 
> > Why not "Download"?  Because "Download" implies network access and this
> > operation may or may not actually go over the network.
> > 
> > How does that sound?
> 
> How about 'Get' as you say and 'GetOrDownload', then?

Done.

[maniscalco, 2014-03-27 00:03:25]

BTW, based on our offline conversation earlier today, I've changed
AttachmentServiceProxy so that it no longer has AsWeakPtr, is no longer derived
from RefCountedThreadSafe, and can now be a data member of SyncData (no pointers
needed!).  RFAL.

On 2014/03/25 21:40:30, maniscalco wrote:
> All comments addressed, RFAL!  Lots of changes since you last reviewed.
> 
> https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.cc
> File sync/api/sync_data.cc (right):
> 
>
https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.cc#ne...
> sync/api/sync_data.cc:233: attachment_service_.Call(
> On 2014/03/19 22:32:48, timsteele wrote:
> > On 2014/03/18 20:49:18, maniscalco wrote:
> > > On 2014/03/10 22:55:58, timsteele wrote:
> > > > As much as I do like the succinct-ness of WeakHandle I think we're
> starting
> > to
> > > > move away from it in favor of DIY (with a task_runner + weak ptr) for
> > > > consistency with the rest of the codebase. 
> > > > 
> > > > But yeah, having to define your own AttServProxy object that posts via
> > > > TaskRunner to a ref-counted core object with a WeakPtr to the
> > > AttachmentService
> > > > is a bit of a pain :\
> > > 
> > > In some ways the explicit proxy object approach seems less error prone
> > because,
> > > to the caller, it looks like any other object (no need to remember a
special
> > way
> > > of invoking methods in the right thread).  LMK if you agree and I'll drop
my
> > use
> > > of WeakHandle.
> > > 
> > > Assuming I drop WeakHandle, I see a couple options.
> > > 
> > > 1. Build the proxy/post functionality into FakeAttachmentService such that
> > it's
> > > transparent to users of FAS.  That is, FAS will look like a thread-safe
> > > implementation of AS.
> > > 
> > > 2. Leave FAS as it is now, not thread-safe.  Create an
> AttachmentServiceProxy
> > > class that wraps an implementation of AS (like FAS).
> > > 
> > > I think I like #2 because it's more composable and will work with any
> > > implementation of AS.
> > 
> > I agree with everything.  At the same time I wouldn't strongly oppose
keeping
> > WeakHandle and converting things later if we ever officially decide to drop
> > WeakHandle.  But if I was starting from scratch I'd probably do #2.
> 
> SGTM.  I've gotten rid of WeakHandle and created AttachmentServiceProxy. 
PTAL.
> 
> https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.h
> File sync/api/sync_data.h (right):
> 
>
https://codereview.chromium.org/187303006/diff/20001/sync/api/sync_data.h#new...
> sync/api/sync_data.h:127: void GetAttachments(const AttachmentIdList&
> attachment_ids,
> On 2014/03/19 22:32:48, timsteele wrote:
> > On 2014/03/18 20:49:18, maniscalco wrote:
> > > On 2014/03/10 22:55:58, timsteele wrote:
> > > > As in the other file, I'm wondering if we should establish a convention
to
> > > > disambiguate between "downloading" and "looking up" when we say "Get"
and
> > are
> > > > referring to an Attachment. Perhaps "Download" and "Lookup", where
Lookup
> is
> > > > always taken to mean looking-up in a local index, never triggering any
> > server
> > > > downloading.  If Lookup fails, items may exist somewhere, just not on
this
> > > > client (yet). 
> > > > 
> > > > Although from the comment neither of those would apply for this method,
> > would
> > > > they.. so maybe Get is the right call for the hybrid cases... more
below.
> > > 
> > > Good point.  I agree.  I would like the verb to more clearly indicate the
> > > method's semantics.  It seems like the most important thing to convey is
> > whether
> > > the operation is local-only or not.  With that in mind, what do you think
of
> > > this:
> > > 
> > > 1. "Get" implies local-only.  No network involved.  Why not "Lookup"? 
> Because
> > > "Get" matches the semantics of SCP::GetAllSyncData and the new
> > SCP::GetSyncData
> > > (see also my reply to your comment in SCP).
> > > 
> > > 2. "Retrieve" for when it may come from the local machine or from the
> network.
> > 
> > > Why not "Download"?  Because "Download" implies network access and this
> > > operation may or may not actually go over the network.
> > > 
> > > How does that sound?
> > 
> > How about 'Get' as you say and 'GetOrDownload', then?
> 
> Done.

[tim (not reviewing), 2014-03-27 17:01:43]

https://codereview.chromium.org/187303006/diff/270001/sync/api/attachments/at...
File sync/api/attachments/attachment_service_proxy.h (right):

https://codereview.chromium.org/187303006/diff/270001/sync/api/attachments/at...
sync/api/attachments/attachment_service_proxy.h:38: public
base::RefCountedThreadSafe<AttachmentServiceProxy> {
I still think having a hidden ref-counted core object is preferable to make ASP
is easier to use.  The core would own the weak ptr and live on the AS thread,
and would allow ASP to not inherit RefCountedThreadSafe.

https://codereview.chromium.org/187303006/diff/330001/sync/api/attachments/at...
File sync/api/attachments/attachment_service_base.h (right):

https://codereview.chromium.org/187303006/diff/330001/sync/api/attachments/at...
sync/api/attachments/attachment_service_base.h:25: virtual
base::WeakPtr<AttachmentServiceBase> AsWeakPtr() = 0;
The more I look at it, the less I think of this as a responsibility of
AttachmentService and more as a responsibility of GenericChangeProcessor, which
owns the AS. GCP is the only thing that vends pointers to an AS -- it can ensure
that it only vends weak ptrs via a weak_ptr_factory member declared after the AS
scoped_ptr, and you're done.  No AttachmentServiceBase, no worrying about AS
implementations keeping the factory as the last member, no referencing
SupportsWeakPtr... WDYT?

[tim (not reviewing), 2014-03-27 17:02:09]

https://codereview.chromium.org/187303006/diff/270001/sync/api/attachments/at...
File sync/api/attachments/attachment_service_proxy.h (right):

https://codereview.chromium.org/187303006/diff/270001/sync/api/attachments/at...
sync/api/attachments/attachment_service_proxy.h:38: public
base::RefCountedThreadSafe<AttachmentServiceProxy> {
On 2014/03/27 17:01:44, timsteele wrote:
> I still think having a hidden ref-counted core object is preferable to make
ASP
> is easier to use.  The core would own the weak ptr and live on the AS thread,
> and would allow ASP to not inherit RefCountedThreadSafe.

Ignore this comment.  It was stale from before a few of our whiteboard
discussions.

[maniscalco, 2014-03-27 17:36:24]

(new patch uploaded)

https://codereview.chromium.org/187303006/diff/330001/sync/api/attachments/at...
File sync/api/attachments/attachment_service_base.h (right):

https://codereview.chromium.org/187303006/diff/330001/sync/api/attachments/at...
sync/api/attachments/attachment_service_base.h:25: virtual
base::WeakPtr<AttachmentServiceBase> AsWeakPtr() = 0;
On 2014/03/27 17:01:44, timsteele wrote:
> The more I look at it, the less I think of this as a responsibility of
> AttachmentService and more as a responsibility of GenericChangeProcessor,
which
> owns the AS. GCP is the only thing that vends pointers to an AS -- it can
ensure
> that it only vends weak ptrs via a weak_ptr_factory member declared after the
AS
> scoped_ptr, and you're done.  No AttachmentServiceBase, no worrying about AS
> implementations keeping the factory as the last member, no referencing
> SupportsWeakPtr... WDYT?

I like this approach.  It's nice to eliminate AttachmentServiceBase (one fewer
class to understand).  Done.

[tim (not reviewing), 2014-03-27 18:35:45]

https://codereview.chromium.org/187303006/diff/350002/sync/api/attachments/at...
File sync/api/attachments/attachment_service_proxy.h (right):

https://codereview.chromium.org/187303006/diff/350002/sync/api/attachments/at...
sync/api/attachments/attachment_service_proxy.h:38: 
nit: remove extra newline.

https://codereview.chromium.org/187303006/diff/350002/sync/api/sync_data.h
File sync/api/sync_data.h (right):

https://codereview.chromium.org/187303006/diff/350002/sync/api/sync_data.h#ne...
sync/api/sync_data.h:112: // The attachments may or may not be present on this
host.
s/host/device to be more consistent with the terminology we tend to use in sync.
host is a bit ambiguous, SyncBackendHost, etc. I was going to suggest "s/present
on this host/present locally", but that, too is ambiguous in this file since
"Local" has a special meaning.

https://codereview.chromium.org/187303006/diff/350002/sync/api/sync_data.h#ne...
sync/api/sync_data.h:115: // Return a list of this SyncData's attachments.
This is only ever the list of attachments intended for upload... so that comment
suggestion wouldn't really jive with "may only be called when IsLocal is true".

This is kind of an inherent problem of SyncData.  We just chatted offline about
potentially splitting state out into SyncData.local()[.tag(),.title(),...] and
SyncData.remote()[.modification_time(), id()] to make it more obvious what's
going on.  Keeping things like GetOrDownloadAttachments on SyncData seems fine
(it would dcheck internally), but for state that is actually currently hanging
off of a SyncData, that split up might really help.

[tim (not reviewing), 2014-03-27 19:16:43]

LGTM with comment addressed.

https://codereview.chromium.org/187303006/diff/350002/sync/api/sync_data.h
File sync/api/sync_data.h (right):

https://codereview.chromium.org/187303006/diff/350002/sync/api/sync_data.h#ne...
sync/api/sync_data.h:115: // Return a list of this SyncData's attachments.
On 2014/03/27 18:35:46, timsteele wrote:
> This is only ever the list of attachments intended for upload... so that
comment
> suggestion wouldn't really jive with "may only be called when IsLocal is
true".
> 
> This is kind of an inherent problem of SyncData.  We just chatted offline
about
> potentially splitting state out into SyncData.local()[.tag(),.title(),...] and
> SyncData.remote()[.modification_time(), id()] to make it more obvious what's
> going on.  Keeping things like GetOrDownloadAttachments on SyncData seems fine
> (it would dcheck internally), but for state that is actually currently hanging
> off of a SyncData, that split up might really help.  

Alternatively, there's the subclass + factory method [ e.g RemoteSyncData*
SyncData::BuildRemoteSyncDataOutOf(SyncData data)].  

For now though, given that this is mostly an existing problem let's try to
checkpoint what you have now.  Perhaps rename GetAttachments to
GetLocalAttachmentsForUpload with a bug/todo about how you plan to fix this, and
then focus on fixing it next.

[maniscalco, 2014-03-27 21:17:30]

Thanks for the feedback.  All applied.

https://codereview.chromium.org/187303006/diff/350002/sync/api/attachments/at...
File sync/api/attachments/attachment_service_proxy.h (right):

https://codereview.chromium.org/187303006/diff/350002/sync/api/attachments/at...
sync/api/attachments/attachment_service_proxy.h:38: 
On 2014/03/27 18:35:46, timsteele wrote:
> nit: remove extra newline.

Done.

https://codereview.chromium.org/187303006/diff/350002/sync/api/sync_data.h
File sync/api/sync_data.h (right):

https://codereview.chromium.org/187303006/diff/350002/sync/api/sync_data.h#ne...
sync/api/sync_data.h:112: // The attachments may or may not be present on this
host.
On 2014/03/27 18:35:46, timsteele wrote:
> s/host/device to be more consistent with the terminology we tend to use in
sync.
> host is a bit ambiguous, SyncBackendHost, etc. I was going to suggest
"s/present
> on this host/present locally", but that, too is ambiguous in this file since
> "Local" has a special meaning.

Done.

https://codereview.chromium.org/187303006/diff/350002/sync/api/sync_data.h#ne...
sync/api/sync_data.h:115: // Return a list of this SyncData's attachments.
On 2014/03/27 19:16:44, timsteele wrote:
> On 2014/03/27 18:35:46, timsteele wrote:
> > This is only ever the list of attachments intended for upload... so that
> comment
> > suggestion wouldn't really jive with "may only be called when IsLocal is
> true".
> > 
> > This is kind of an inherent problem of SyncData.  We just chatted offline
> about
> > potentially splitting state out into SyncData.local()[.tag(),.title(),...]
and
> > SyncData.remote()[.modification_time(), id()] to make it more obvious what's
> > going on.  Keeping things like GetOrDownloadAttachments on SyncData seems
fine
> > (it would dcheck internally), but for state that is actually currently
hanging
> > off of a SyncData, that split up might really help.  
> 
> Alternatively, there's the subclass + factory method [ e.g RemoteSyncData*
> SyncData::BuildRemoteSyncDataOutOf(SyncData data)].  
> 
> For now though, given that this is mostly an existing problem let's try to
> checkpoint what you have now.  Perhaps rename GetAttachments to
> GetLocalAttachmentsForUpload with a bug/todo about how you plan to fix this,
and
> then focus on fixing it next.

SGTM.  Renamed to GetLocalAttachmentsForUpload.  Bug created (crbug/357305).

[maniscalco, 2014-03-31 21:02:23]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-03-31 21:03:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maniscalco@chromium.org/187303006/380001

[maniscalco, 2014-04-01 03:16:19]

The CQ bit was unchecked by maniscalco@chromium.org

[maniscalco, 2014-04-01 03:16:42]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-04-01 03:16:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maniscalco@chromium.org/187303006/380001

[commit-bot: I haz the power, 2014-04-01 05:04:50]

Change committed as 260778