Checkpointing some Sync Attachment work. Added SyncAttachment and SyncAttachmentId.

Checkpointing some Sync Attachment work. Added SyncAttachment and SyncAttachmentId. The interfaces are still in flux and will likely change.

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

[Nicolas Zea, 2013-12-30 19:10:31]

FYI, unless you "publish and mail comments" no email is sent to the reviewer, so
it's easy to miss

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

https://codereview.chromium.org/102193004/diff/20001/sync/api/sync_attachment...
sync/api/sync_attachment.cc:41: DCHECK(id.has_unique_id());
I think this dcheck is redundant given the one below?

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

https://codereview.chromium.org/102193004/diff/20001/sync/api/sync_attachment...
sync/api/sync_attachment.h:29: // Creates an attachment with the supplied id and
bytes.
Maybe comment some more about why someone would supply their own id vs having
sync create the id for them?

https://codereview.chromium.org/102193004/diff/20001/sync/api/sync_attachment...
sync/api/sync_attachment.h:37: // Returns this attachments bytes.
nit: attachments -> attachment's

https://codereview.chromium.org/102193004/diff/20001/sync/api/sync_attachment...
sync/api/sync_attachment.h:41: SyncAttachment(const sync_pb::SyncAttachmentId&
id,
nit: move this below the member variables below

https://codereview.chromium.org/102193004/diff/20001/sync/api/sync_attachment...
File sync/api/sync_attachment_unittest.cc (right):

https://codereview.chromium.org/102193004/diff/20001/sync/api/sync_attachment...
sync/api/sync_attachment_unittest.cc:26: bytes->data() = "some data";
nit: make this a anon const variable, and reuse it below?

https://codereview.chromium.org/102193004/diff/20001/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/102193004/diff/20001/sync/protocol/sync.proto...
sync/protocol/sync.proto:899: message SyncAttachmentId {
Should we have this in its own proto file? This file is pretty big as is

https://codereview.chromium.org/102193004/diff/20001/sync/sync_api.gypi
File sync/sync_api.gypi (right):

https://codereview.chromium.org/102193004/diff/20001/sync/sync_api.gypi#newco...
sync/sync_api.gypi:19: 'api/sync_attachment.h',
nit: technically the order of these files should have cc before h (although that
wasn't observed below). May as well fix that up now.

[maniscalco, 2013-12-30 22:17:18]

Thanks for the detailed feedback!  All applied.  RFAL.

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

https://codereview.chromium.org/102193004/diff/20001/sync/api/sync_attachment...
sync/api/sync_attachment.cc:41: DCHECK(id.has_unique_id());
On 2013/12/30 19:10:31, Nicolas Zea wrote:
> I think this dcheck is redundant given the one below?

You're right.  Done.

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

https://codereview.chromium.org/102193004/diff/20001/sync/api/sync_attachment...
sync/api/sync_attachment.h:29: // Creates an attachment with the supplied id and
bytes.
On 2013/12/30 19:10:31, Nicolas Zea wrote:
> Maybe comment some more about why someone would supply their own id vs having
> sync create the id for them?

Expanded the comments to clarify.  Done.

https://codereview.chromium.org/102193004/diff/20001/sync/api/sync_attachment...
sync/api/sync_attachment.h:37: // Returns this attachments bytes.
On 2013/12/30 19:10:31, Nicolas Zea wrote:
> nit: attachments -> attachment's

Done.

https://codereview.chromium.org/102193004/diff/20001/sync/api/sync_attachment...
sync/api/sync_attachment.h:41: SyncAttachment(const sync_pb::SyncAttachmentId&
id,
On 2013/12/30 19:10:31, Nicolas Zea wrote:
> nit: move this below the member variables below

Done.

https://codereview.chromium.org/102193004/diff/20001/sync/api/sync_attachment...
File sync/api/sync_attachment_unittest.cc (right):

https://codereview.chromium.org/102193004/diff/20001/sync/api/sync_attachment...
sync/api/sync_attachment_unittest.cc:26: bytes->data() = "some data";
On 2013/12/30 19:10:31, Nicolas Zea wrote:
> nit: make this a anon const variable, and reuse it below?

Done.

https://codereview.chromium.org/102193004/diff/20001/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/102193004/diff/20001/sync/protocol/sync.proto...
sync/protocol/sync.proto:899: message SyncAttachmentId {
On 2013/12/30 19:10:31, Nicolas Zea wrote:
> Should we have this in its own proto file? This file is pretty big as is

Good call.  Done.  I moved it to attachments.proto.  LMK if it should be called
attachment.proto instead.

https://codereview.chromium.org/102193004/diff/20001/sync/sync_api.gypi
File sync/sync_api.gypi (right):

https://codereview.chromium.org/102193004/diff/20001/sync/sync_api.gypi#newco...
sync/sync_api.gypi:19: 'api/sync_attachment.h',
On 2013/12/30 19:10:31, Nicolas Zea wrote:
> nit: technically the order of these files should have cc before h (although
that
> wasn't observed below). May as well fix that up now.

Done.

[Nicolas Zea, 2013-12-30 23:04:21]

Mostly LGTM. The only thing I wonder about is whether we should be using
SyncAttachment, or just Attachment, given that it's already in the syncer
namespace. We've had this issue with other classes too, where we wind up
overusing sync in a redundant way. I guess everything else in sync/api/ follows
the Sync* pattern, so maybe it makes sense to leave it as is.

https://codereview.chromium.org/102193004/diff/210001/sync/protocol/attachmen...
File sync/protocol/attachments.proto (right):

https://codereview.chromium.org/102193004/diff/210001/sync/protocol/attachmen...
sync/protocol/attachments.proto:20: optional string unique_id = 1;
This message and field should probably be added to proto_value_conversions.h/cc
as well.

[maniscalco, 2013-12-30 23:54:22]

All applied.  As we talked about in person, sticking with SyncAttachment over
Attachment to preserve consistency.  RFAL.

https://codereview.chromium.org/102193004/diff/210001/sync/protocol/attachmen...
File sync/protocol/attachments.proto (right):

https://codereview.chromium.org/102193004/diff/210001/sync/protocol/attachmen...
sync/protocol/attachments.proto:20: optional string unique_id = 1;
On 2013/12/30 23:04:22, Nicolas Zea wrote:
> This message and field should probably be added to
proto_value_conversions.h/cc
> as well.

Done.

[Nicolas Zea, 2013-12-30 23:56:11]

LGTM

[commit-bot: I haz the power, 2014-01-02 17:39:39]

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

[commit-bot: I haz the power, 2014-01-02 22:42:18]

Change committed as 242818