Invoke AttachmentUploader and update AttachmentIds.

Invoke AttachmentUploader and update AttachmentIds.

When GenericChangeProcessor calls AttachmentService::StoreAttachments
AttachmentService invokes AttachmentUploader to upload attachments.

When attachment gets uploaded AttachmentService invokes
AttachmentService::Delegate::OnAttachmentUploaded.

GenericChangeProcessor implements AttachmentService::Delegate for now.
When attachment is uploaded it call WriteTransaction to update entries
referring this attachment with new attachment id.

R=maniscalco@chromium.org
BUG=371629
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271578

[pavely, 2014-05-09 00:07:35]

PTAL.

I'll add test for FakeAttachmentService portion once FakeAttachmentService is
moved and renamed.

[maniscalco, 2014-05-09 18:12:11]

Very nice!  I've got some questions and feedback.  Let's chat in person about a
few these items.  Also, can you kick off a "git cl try"?

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

https://codereview.chromium.org/272043002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service.h:59: class Delegate {
I'm thinking about in which thread the delegate will be called.  It probably
deserves a comment that it will be called in the same thread from which the
AttachmentService was called.

https://codereview.chromium.org/272043002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service.h:71: // Set delegate pointer.
AttachmentService doesn't own delegate.
Can we avoid adding a no-op implementation here?  By putting this setter on the
interface (AttachmentService) we're telling the world that every
AttachmentService is capable of getting a delegate, but that's not really true
(e.g. AttachmentServiceProxy).  Let's chat about this one.  I see that we wan't
just pass the delegate to the ctor of the derived type because GCP *is* the
delegate so it's not immediately obvious to me how we can accomplish this.

https://codereview.chromium.org/272043002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service.h:72: virtual void SetDelegate(Delegate*
delegate) {}
In the future we may move the delegate functionality out of GCP at which point
the AS may outlive its delegate.  I suggest updating AS to take a Delegate via
WeakPtr.

https://codereview.chromium.org/272043002/diff/1/sync/api/attachments/fake_at...
File sync/api/attachments/fake_attachment_service.cc (right):

https://codereview.chromium.org/272043002/diff/1/sync/api/attachments/fake_at...
sync/api/attachments/fake_attachment_service.cc:140: DCHECK(delegate_);
I thought having a delegate_ was optional, no?

https://codereview.chromium.org/272043002/diff/1/sync/api/attachments/fake_at...
sync/api/attachments/fake_attachment_service.cc:141: if (result !=
AttachmentUploader::UPLOAD_SUCCESS)
Please add a TODO for dealing with error results.

https://codereview.chromium.org/272043002/diff/1/sync/internal_api/public/wri...
File sync/internal_api/public/write_transaction.h (right):

https://codereview.chromium.org/272043002/diff/1/sync/internal_api/public/wri...
sync/internal_api/public/write_transaction.h:57: void
UpdateEntriesWithAttachmentId(const AttachmentId& attachment_id);
How about a unit test for this method?  directory_unittest.cc might be a good
place for it.

https://codereview.chromium.org/272043002/diff/1/sync/protocol/attachments.proto
File sync/protocol/attachments.proto (right):

https://codereview.chromium.org/272043002/diff/1/sync/protocol/attachments.pr...
sync/protocol/attachments.proto:23: optional string url = 3;
URL may be too specific because it encodes both the service endpoint and the
attachment.  I'd like to explore some options:

Instead of url, we could store some blob id and have the client know which
endpoint to use (prod, test, staging, etc.).  The downloader would then need to
know how to form the URL for retrieval.

Another approach would be to replace url with a backend specific type (a proto
message) that's logically private to AttachmentUploader/Downloader.

Let's chat more about this in person.

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.cc#n...
sync/syncable/directory.cc:462: BaseTransaction* trans,
I want to confirm my understanding.  The only reason trans is passed here is
that you want to ensure the caller has a transaction, right?

nit: consider DCHECKing trans here as is done elsewhere.

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.cc#n...
sync/syncable/directory.cc:464: Metahandles* result) {
nit: consider DCHECKing result.

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.cc#n...
sync/syncable/directory.cc:466: IndexByAttachmentId::iterator index_iter =
Could this be made a const_iterator?

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.cc#n...
sync/syncable/directory.cc:469: return;
From the header comment and method name, it's not obvious if a caller should
expect its result to be clear()ed if the attachment id is not found in the
index.  Since this method is GetMetahandlesByAttachmentId and not
AppendMetahandlesByAttachmentId, I would assume it would clear result.

Can you please update the doc to say what happens in this case?

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.cc#n...
sync/syncable/directory.cc:471: copy(metahandle_set.begin(),
metahandle_set.end(), back_inserter(*result));
Huh, I would have expected this to need std::.  Maybe one of our #includes has a
"using std"...

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.h#ne...
sync/syncable/directory.h:402: void GetMetahandlesByAttachmentId(
Can you please add some test cases to directory_unittest.cc for this new method?

https://codereview.chromium.org/272043002/diff/1/sync/syncable/mutable_entry.cc
File sync/syncable/mutable_entry.cc (right):

https://codereview.chromium.org/272043002/diff/1/sync/syncable/mutable_entry....
sync/syncable/mutable_entry.cc:250: void MutableEntry::UpdateAttachmentId(
How about adding a unit test for this method?  directory_unittests.cc may be a
good place for it.

https://codereview.chromium.org/272043002/diff/1/sync/syncable/mutable_entry....
sync/syncable/mutable_entry.cc:252: DCHECK(kernel_);
Consider DCHECKing that attachment_id's unique_id is not empty string.

https://codereview.chromium.org/272043002/diff/1/sync/syncable/mutable_entry....
sync/syncable/mutable_entry.cc:261: record->set_is_on_server(true);
I wonder if we can make it more obvious that this method sets the is_on_server
flag?  In addition to updating the header comment, what do you think about
changing its name?

UpdateAttachmentIdOnServer?

UpdateAttachmentId(attachment_id, IS_ON_SERVER) ?

UpdateAttachmentIdAfterUpload(attachment_id) ?

https://codereview.chromium.org/272043002/diff/1/sync/syncable/mutable_entry.h
File sync/syncable/mutable_entry.h (right):

https://codereview.chromium.org/272043002/diff/1/sync/syncable/mutable_entry....
sync/syncable/mutable_entry.h:64: // Update attachment metadata, replace all
records matching attachment id's
nit: there should be a blank line above this comment.

https://codereview.chromium.org/272043002/diff/1/sync/syncable/mutable_entry....
sync/syncable/mutable_entry.h:66: void UpdateAttachmentId(const
sync_pb::AttachmentIdProto& attachment_id);
Consider renaming attachment_id to updated_attachment_id so it's even more clear
that this method isn't updating attachment_id.

[pavely, 2014-05-13 23:06:43]

PTAL

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

https://codereview.chromium.org/272043002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service.h:59: class Delegate {
On 2014/05/09 18:12:11, maniscalco wrote:
> I'm thinking about in which thread the delegate will be called.  It probably
> deserves a comment that it will be called in the same thread from which the
> AttachmentService was called.

Done.

https://codereview.chromium.org/272043002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service.h:71: // Set delegate pointer.
AttachmentService doesn't own delegate.
The reason was to not add implementation of it to AttachmentServiceProxy , but
you are right, I'd rather add NOTREACHED there.

On 2014/05/09 18:12:11, maniscalco wrote:
> Can we avoid adding a no-op implementation here?  By putting this setter on
the
> interface (AttachmentService) we're telling the world that every
> AttachmentService is capable of getting a delegate, but that's not really true
> (e.g. AttachmentServiceProxy).  Let's chat about this one.  I see that we
wan't
> just pass the delegate to the ctor of the derived type because GCP *is* the
> delegate so it's not immediately obvious to me how we can accomplish this.

https://codereview.chromium.org/272043002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service.h:72: virtual void SetDelegate(Delegate*
delegate) {}
Right now interface simple, explicit and will blow up if developer does
something wrong. It is similar to AddObserver/RemoveObserver pattern:
AttachmentService already exists when GCP is created, GCP sets itself as
delegate at init and removes when it goes away.

WeakPtr on the other hand offer pattern where mistakes go unnoticed. It will
silently work without reporting any errors, only sometimes (maybe often) it
won't update IDs after upload and we wouldn't even know it.

I'd say let's keep it simple for now and change it to WeakPtr when we have case
for it.
On 2014/05/09 18:12:11, maniscalco wrote:
> In the future we may move the delegate functionality out of GCP at which point
> the AS may outlive its delegate.  I suggest updating AS to take a Delegate via
> WeakPtr.

https://codereview.chromium.org/272043002/diff/1/sync/api/attachments/fake_at...
File sync/api/attachments/fake_attachment_service.cc (right):

https://codereview.chromium.org/272043002/diff/1/sync/api/attachments/fake_at...
sync/api/attachments/fake_attachment_service.cc:140: DCHECK(delegate_);
On 2014/05/09 18:12:11, maniscalco wrote:
> I thought having a delegate_ was optional, no?
No, having delegate is not optional, I would rather pass delegate through
AttachmentService constructor but the problem is that AttachmentService itself
is constructed before GenericChangeProcessor. So I decided to go with setter
method.

https://codereview.chromium.org/272043002/diff/1/sync/api/attachments/fake_at...
sync/api/attachments/fake_attachment_service.cc:141: if (result !=
AttachmentUploader::UPLOAD_SUCCESS)
On 2014/05/09 18:12:11, maniscalco wrote:
> Please add a TODO for dealing with error results.

Done.

https://codereview.chromium.org/272043002/diff/1/sync/internal_api/public/wri...
File sync/internal_api/public/write_transaction.h (right):

https://codereview.chromium.org/272043002/diff/1/sync/internal_api/public/wri...
sync/internal_api/public/write_transaction.h:57: void
UpdateEntriesWithAttachmentId(const AttachmentId& attachment_id);
directory_unittest tests the other WriteTransaction. This one is not accessible
from there.
Method is straightforward so I decided not to test it.
On 2014/05/09 18:12:11, maniscalco wrote:
> How about a unit test for this method?  directory_unittest.cc might be a good
> place for it.

https://codereview.chromium.org/272043002/diff/1/sync/protocol/attachments.proto
File sync/protocol/attachments.proto (right):

https://codereview.chromium.org/272043002/diff/1/sync/protocol/attachments.pr...
sync/protocol/attachments.proto:23: optional string url = 3;
I needed to start with something so I put url.
We need to decide what's it going to be, it needs to be flexible for future
change of attachment server.
Two options:
1. Have one string field and let code in AttachmentService interpret it.
2. Have attachment server specific proto and abandon it if we decide to migrate
to different attachment server.
I think #2 is simpler.

For now let me just remove this field from this change. We'll add it when we
know what it will look like.
On 2014/05/09 18:12:11, maniscalco wrote:
> URL may be too specific because it encodes both the service endpoint and the
> attachment.  I'd like to explore some options:
> 
> Instead of url, we could store some blob id and have the client know which
> endpoint to use (prod, test, staging, etc.).  The downloader would then need
to
> know how to form the URL for retrieval.
> 
> Another approach would be to replace url with a backend specific type (a proto
> message) that's logically private to AttachmentUploader/Downloader.
> 
> Let's chat more about this in person.

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.cc#n...
sync/syncable/directory.cc:462: BaseTransaction* trans,
Yes, that's right. If unaware developer tries to call Directory without
transaction compiler will complain about it right away.

Actually in most of cases trans is not DCHECKed (look for BaseWriteTransaction
in this file). The reason is that nothing is done with transaction object so it
is only protecting from forgetful developer, not from malicious.

On 2014/05/09 18:12:11, maniscalco wrote:
> I want to confirm my understanding.  The only reason trans is passed here is
> that you want to ensure the caller has a transaction, right?
> 
> nit: consider DCHECKing trans here as is done elsewhere.

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.cc#n...
sync/syncable/directory.cc:464: Metahandles* result) {
On 2014/05/09 18:12:11, maniscalco wrote:
> nit: consider DCHECKing result.

Done.

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.cc#n...
sync/syncable/directory.cc:466: IndexByAttachmentId::iterator index_iter =
On 2014/05/09 18:12:11, maniscalco wrote:
> Could this be made a const_iterator?

Done.

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.cc#n...
sync/syncable/directory.cc:469: return;
Yes, I should have cleared it.
Done.
On 2014/05/09 18:12:11, maniscalco wrote:
> From the header comment and method name, it's not obvious if a caller should
> expect its result to be clear()ed if the attachment id is not found in the
> index.  Since this method is GetMetahandlesByAttachmentId and not
> AppendMetahandlesByAttachmentId, I would assume it would clear result.
> 
> Can you please update the doc to say what happens in this case?

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.cc#n...
sync/syncable/directory.cc:471: copy(metahandle_set.begin(),
metahandle_set.end(), back_inserter(*result));
Maybe... I copied it from GetUnsyncedMetaHandles. std::copy is better.
Done.
On 2014/05/09 18:12:11, maniscalco wrote:
> Huh, I would have expected this to need std::.  Maybe one of our #includes has
a
> "using std"...

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.h#ne...
sync/syncable/directory.h:402: void GetMetahandlesByAttachmentId(
On 2014/05/09 18:12:11, maniscalco wrote:
> Can you please add some test cases to directory_unittest.cc for this new
method?

Done.

https://codereview.chromium.org/272043002/diff/1/sync/syncable/mutable_entry.cc
File sync/syncable/mutable_entry.cc (right):

https://codereview.chromium.org/272043002/diff/1/sync/syncable/mutable_entry....
sync/syncable/mutable_entry.cc:250: void MutableEntry::UpdateAttachmentId(
On 2014/05/09 18:12:11, maniscalco wrote:
> How about adding a unit test for this method?  directory_unittests.cc may be a
> good place for it.

Done.

https://codereview.chromium.org/272043002/diff/1/sync/syncable/mutable_entry....
sync/syncable/mutable_entry.cc:252: DCHECK(kernel_);
On 2014/05/09 18:12:11, maniscalco wrote:
> Consider DCHECKing that attachment_id's unique_id is not empty string.

Done.

https://codereview.chromium.org/272043002/diff/1/sync/syncable/mutable_entry....
sync/syncable/mutable_entry.cc:261: record->set_is_on_server(true);
Decided to name it UpdateAttachmentIdWithServerInfo.
On 2014/05/09 18:12:11, maniscalco wrote:
> I wonder if we can make it more obvious that this method sets the is_on_server
> flag?  In addition to updating the header comment, what do you think about
> changing its name?
> 
> UpdateAttachmentIdOnServer?
> 
> UpdateAttachmentId(attachment_id, IS_ON_SERVER) ?
> 
> UpdateAttachmentIdAfterUpload(attachment_id) ?

https://codereview.chromium.org/272043002/diff/1/sync/syncable/mutable_entry.h
File sync/syncable/mutable_entry.h (right):

https://codereview.chromium.org/272043002/diff/1/sync/syncable/mutable_entry....
sync/syncable/mutable_entry.h:64: // Update attachment metadata, replace all
records matching attachment id's
On 2014/05/09 18:12:11, maniscalco wrote:
> nit: there should be a blank line above this comment.

Done.

https://codereview.chromium.org/272043002/diff/1/sync/syncable/mutable_entry....
sync/syncable/mutable_entry.h:66: void UpdateAttachmentId(const
sync_pb::AttachmentIdProto& attachment_id);
On 2014/05/09 18:12:11, maniscalco wrote:
> Consider renaming attachment_id to updated_attachment_id so it's even more
clear
> that this method isn't updating attachment_id.

Done.

[pavely, 2014-05-14 21:57:24]

Nick, take a look at last patch. This is what we discussed in the morning. 

WDYT?

[pavely, 2014-05-15 18:44:49]

Yet another way to initialize.

PTAL.

[pavely, 2014-05-15 19:26:51]

The common problem of both of these approaches is that GCP needs AS pointer in
its constructor and AS needs GCP pointer in its constructor so they need to be
initialized in lockstep as in:
Allocate GCP
Allocate AS
call AS::ctor with GCP ptr
call GCP::ctor with AS ptr

No matter which approach we take it is not going to be flexible solution.

https://codereview.chromium.org/272043002/diff/120001/components/sync_driver/...
File components/sync_driver/generic_change_processor.h (right):

https://codereview.chromium.org/272043002/diff/120001/components/sync_driver/...
components/sync_driver/generic_change_processor.h:52: SyncApiComponentFactory*
sync_factory);
The problem with this approach is that now FakeGenericChangeProcessor depends on
some SyncApiComponentFactory and cannot be initialized without it.

[maniscalco, 2014-05-15 20:09:27]

I agree that there isn't an ideal way to construct these two classes, but
overall, I'm satisfied with the approach you laid out here.  I have only a few
remaining comments.

https://codereview.chromium.org/272043002/diff/1/sync/internal_api/public/wri...
File sync/internal_api/public/write_transaction.h (right):

https://codereview.chromium.org/272043002/diff/1/sync/internal_api/public/wri...
sync/internal_api/public/write_transaction.h:57: void
UpdateEntriesWithAttachmentId(const AttachmentId& attachment_id);
You are right, directory_unittest is for the other WriteTransaction.  I'm OK
with omitting a test for this method.

On 2014/05/13 23:06:44, pavely wrote:
> directory_unittest tests the other WriteTransaction. This one is not
accessible
> from there.
> Method is straightforward so I decided not to test it.
> On 2014/05/09 18:12:11, maniscalco wrote:
> > How about a unit test for this method?  directory_unittest.cc might be a
good
> > place for it.
>

https://codereview.chromium.org/272043002/diff/1/sync/protocol/attachments.proto
File sync/protocol/attachments.proto (right):

https://codereview.chromium.org/272043002/diff/1/sync/protocol/attachments.pr...
sync/protocol/attachments.proto:23: optional string url = 3;
SGTM

On 2014/05/13 23:06:44, pavely wrote:
> I needed to start with something so I put url.
> We need to decide what's it going to be, it needs to be flexible for future
> change of attachment server.
> Two options:
> 1. Have one string field and let code in AttachmentService interpret it.
> 2. Have attachment server specific proto and abandon it if we decide to
migrate
> to different attachment server.
> I think #2 is simpler.
> 
> For now let me just remove this field from this change. We'll add it when we
> know what it will look like.
> On 2014/05/09 18:12:11, maniscalco wrote:
> > URL may be too specific because it encodes both the service endpoint and the
> > attachment.  I'd like to explore some options:
> > 
> > Instead of url, we could store some blob id and have the client know which
> > endpoint to use (prod, test, staging, etc.).  The downloader would then need
> to
> > know how to form the URL for retrieval.
> > 
> > Another approach would be to replace url with a backend specific type (a
proto
> > message) that's logically private to AttachmentUploader/Downloader.
> > 
> > Let's chat more about this in person.
>

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/272043002/diff/1/sync/syncable/directory.cc#n...
sync/syncable/directory.cc:462: BaseTransaction* trans,
Yeah, the only reason I suggesting DCHECKing the transaction is to ensure
someone didn't pass NULL intentionally, believing that transaction is optional. 
Of course, I'm assuming that calling this method when there is no transaction
object on the stack is a bug (no transaction object, no transaction lock
acquisition, right?).

On 2014/05/13 23:06:44, pavely wrote:
> Yes, that's right. If unaware developer tries to call Directory without
> transaction compiler will complain about it right away.
> 
> Actually in most of cases trans is not DCHECKed (look for BaseWriteTransaction
> in this file). The reason is that nothing is done with transaction object so
it
> is only protecting from forgetful developer, not from malicious.
> 
> On 2014/05/09 18:12:11, maniscalco wrote:
> > I want to confirm my understanding.  The only reason trans is passed here is
> > that you want to ensure the caller has a transaction, right?
> > 
> > nit: consider DCHECKing trans here as is done elsewhere.
>

https://codereview.chromium.org/272043002/diff/120001/components/sync_driver/...
File components/sync_driver/generic_change_processor.h (right):

https://codereview.chromium.org/272043002/diff/120001/components/sync_driver/...
components/sync_driver/generic_change_processor.h:52: SyncApiComponentFactory*
sync_factory);
I think I see what you mean.  Creating a mock SyncApiComponentsFactory for use
in tests doesn't seem too bad to me.  Overall, I like this approach a lot better
that the setter approach.

On 2014/05/15 19:26:52, pavely wrote:
> The problem with this approach is that now FakeGenericChangeProcessor depends
on
> some SyncApiComponentFactory and cannot be initialized without it.

https://codereview.chromium.org/272043002/diff/120001/components/sync_driver/...
File components/sync_driver/generic_change_processor_factory.h (right):

https://codereview.chromium.org/272043002/diff/120001/components/sync_driver/...
components/sync_driver/generic_change_processor_factory.h:39:
SyncApiComponentFactory* sync_factory);
Can you document the sync_factory parameter?  I think it's worth calling out
that GCP won't own or manage the sync_factory and that it will only use it in
its ctor (that way, the caller doesn't have to guarantee the sync_factory
outlives the GCP).

https://codereview.chromium.org/272043002/diff/120001/components/sync_driver/...
File components/sync_driver/sync_api_component_factory.h (right):

https://codereview.chromium.org/272043002/diff/120001/components/sync_driver/...
components/sync_driver/sync_api_component_factory.h:11: #include
"sync/api/attachments/attachment_store.h"
No longer needed?

https://codereview.chromium.org/272043002/diff/120001/components/sync_driver/...
components/sync_driver/sync_api_component_factory.h:28: // Note: Should only be
called from the model type thread.
Please document the delegate parameter (does not own, can be null, must outlive
if non-null, etc.)

https://codereview.chromium.org/272043002/diff/120001/sync/api/attachments/at...
File sync/api/attachments/attachment_service_impl.cc (right):

https://codereview.chromium.org/272043002/diff/120001/sync/api/attachments/at...
sync/api/attachments/attachment_service_impl.cc:138: DCHECK(delegate_);
Based on our last talk, I was thinking delegate_ was optional.  Is that still
the case?

https://codereview.chromium.org/272043002/diff/120001/sync/api/attachments/at...
File sync/api/attachments/attachment_service_impl.h (right):

https://codereview.chromium.org/272043002/diff/120001/sync/api/attachments/at...
sync/api/attachments/attachment_service_impl.h:23: Delegate* delegate);
Can you please document the delegate parameter?  We should call out some things
explicitly:

1. The delegate is not owned by the ASI.

2. The delegate must outlive the ASI.

3. The delegate is optional (pass NULL if you don't want/need a delegate).

[pavely, 2014-05-16 00:19:10]

https://codereview.chromium.org/272043002/diff/120001/components/sync_driver/...
File components/sync_driver/generic_change_processor_factory.h (right):

https://codereview.chromium.org/272043002/diff/120001/components/sync_driver/...
components/sync_driver/generic_change_processor_factory.h:39:
SyncApiComponentFactory* sync_factory);
On 2014/05/15 20:09:28, maniscalco wrote:
> Can you document the sync_factory parameter?  I think it's worth calling out
> that GCP won't own or manage the sync_factory and that it will only use it in
> its ctor (that way, the caller doesn't have to guarantee the sync_factory
> outlives the GCP).

Done.

https://codereview.chromium.org/272043002/diff/120001/components/sync_driver/...
File components/sync_driver/sync_api_component_factory.h (right):

https://codereview.chromium.org/272043002/diff/120001/components/sync_driver/...
components/sync_driver/sync_api_component_factory.h:11: #include
"sync/api/attachments/attachment_store.h"
On 2014/05/15 20:09:28, maniscalco wrote:
> No longer needed?

Done.

https://codereview.chromium.org/272043002/diff/120001/components/sync_driver/...
components/sync_driver/sync_api_component_factory.h:28: // Note: Should only be
called from the model type thread.
On 2014/05/15 20:09:28, maniscalco wrote:
> Please document the delegate parameter (does not own, can be null, must
outlive
> if non-null, etc.)

Done.

https://codereview.chromium.org/272043002/diff/120001/sync/api/attachments/at...
File sync/api/attachments/attachment_service_impl.cc (right):

https://codereview.chromium.org/272043002/diff/120001/sync/api/attachments/at...
sync/api/attachments/attachment_service_impl.cc:138: DCHECK(delegate_);
I made this change but lost it between revisions 5 and 6.
Done
On 2014/05/15 20:09:28, maniscalco wrote:
> Based on our last talk, I was thinking delegate_ was optional.  Is that still
> the case?

https://codereview.chromium.org/272043002/diff/120001/sync/api/attachments/at...
File sync/api/attachments/attachment_service_impl.h (right):

https://codereview.chromium.org/272043002/diff/120001/sync/api/attachments/at...
sync/api/attachments/attachment_service_impl.h:23: Delegate* delegate);
On 2014/05/15 20:09:28, maniscalco wrote:
> Can you please document the delegate parameter?  We should call out some
things
> explicitly:
> 
> 1. The delegate is not owned by the ASI.
> 
> 2. The delegate must outlive the ASI.
> 
> 3. The delegate is optional (pass NULL if you don't want/need a delegate).

Done.

[maniscalco, 2014-05-16 00:28:24]

lgtm

[pavely, 2014-05-16 00:45:35]

Adding Tim as Nick is not on OWNERS yet.
I'll get a change to put Nick into OWNERS tomorrow.

[pavely, 2014-05-16 18:45:15]

Rebased and fixed components_unittests

[pavely, 2014-05-19 22:07:03]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2014-05-19 22:08:21]

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

[commit-bot: I haz the power, 2014-05-19 22:08:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-19 22:08:24]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[pavely, 2014-05-19 22:11:14]

Nicolas, Tim is out and Nick is not on OWNERS. Could you lgtm?

[Nicolas Zea, 2014-05-20 00:11:18]

lgtm

[pavely, 2014-05-20 00:11:43]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2014-05-20 00:12:05]

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

[commit-bot: I haz the power, 2014-05-20 06:14:58]

Change committed as 271578