Add AttachmentMetadata to Sync's EntryKernel.

Add AttachmentMetadata to Sync's EntryKernel.

In future CLs, AttachmentMetadata will be extended to include metadata
about the sync entries attachments such as attachment ids and whether
they have been upload to the server.

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

[maniscalco, 2014-03-25 18:29:33]

Hi Nicolas, would you please review this change?

Do you see any issues with committing this and later adding fields to the
AttachmentMetadata proto?  I think it's OK, but I want to be careful given CL
changes the on disk storage.

[Nicolas Zea, 2014-03-26 20:32:19]

Mostly LG with a comment.

One question though: are you planning to store all information for a particular
attachment within the AttachmentMetadata proto? I.e., AttachmentMetaData will
contains a repeated list of attachments or something?

That seems a departure from the way we do things now, namely where
IS_UNSYNCED/IS_UNAPPLIED and such are columns of their own, but given that we
can have multiple attachments per entry this might be the best approach. Just
want to make sure I'm understanding though.

https://codereview.chromium.org/211523002/diff/30001/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/211523002/diff/30001/sync/protocol/sync.proto...
sync/protocol/sync.proto:22: import "attachments.proto";
is this necessary?

[maniscalco, 2014-03-26 21:58:10]

Thanks for the quick review!

Exactly.  In a future CL, I plan to give AttachmentMetadata a repeated field of
messages that contain attachment id and status of whether the attachment has
been uploaded, etc.

Yes, because a single entry can have many attachments, I felt it was simpler to
use a single proto per entry that can contain a repeated field.  This probably
deserves a comment... OK, I've add a comment to syncable_columns.h.  Hopefully
that's more clear.

https://codereview.chromium.org/211523002/diff/30001/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/211523002/diff/30001/sync/protocol/sync.proto...
sync/protocol/sync.proto:22: import "attachments.proto";
On 2014/03/26 20:32:19, Nicolas Zea wrote:
> is this necessary?

Good catch.  Removed.

[Nicolas Zea, 2014-03-26 22:00:34]

lgtm

[maniscalco, 2014-03-31 15:47:54]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-03-31 15:48:04]

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

[commit-bot: I haz the power, 2014-03-31 17:31:07]

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

[commit-bot: I haz the power, 2014-03-31 17:31:08]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[maniscalco, 2014-03-31 17:34:53]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-03-31 17:36:55]

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

[maniscalco, 2014-03-31 21:06:06]

The CQ bit was checked by maniscalco@chromium.org

[maniscalco, 2014-03-31 22:14:21]

The CQ bit was unchecked by maniscalco@chromium.org

[maniscalco, 2014-04-01 18:29:07]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-04-01 18:29:08]

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

[maniscalco, 2014-04-01 20:44:26]

The CQ bit was unchecked by maniscalco@chromium.org

[maniscalco, 2014-04-01 21:00:10]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-04-01 21:01:19]

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

[commit-bot: I haz the power, 2014-04-01 22:52:57]

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

[commit-bot: I haz the power, 2014-04-02 02:05:12]

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

[commit-bot: I haz the power, 2014-04-02 03:40:31]

Change committed as 261046