[Sync] Add size and crc32c to AttachmentId.

[Sync] Add size and crc32c to AttachmentId.

The purpose of this change is to ensure that if a client knows about an
attachment (i.e. has an AttachmentId or AttachmentIdProto) it will know
the attachment's size even if the attachment has never been available on
the local device.

The idea is that by storing size locally, we can simplify remote storage
management.

Move crc32c out of Attachment now that it's part of AttachmentId.

BUG=464431
Committed: https://crrev.com/23ae3128db0d84a6b1ffa640568a5ec90cfc8808
Cr-Commit-Position: refs/heads/master@{#319794}

Committed: https://crrev.com/4c80a31653994ec6f112985b3493b1809e8e3415
Cr-Commit-Position: refs/heads/master@{#320103}

[maniscalco, 2015-03-05 18:20:20]

maniscalco@chromium.org changed reviewers:
+ pavely@chromium.org

[maniscalco, 2015-03-05 18:20:21]

Pavel, would you please review this change?

Side note, yesterday I was looking for the crc32c of an empty string (or at
least what leveldb's implementation returns). Turns out it's 0.

[maniscalco, 2015-03-07 00:07:01]

On 2015/03/05 18:20:21, maniscalco wrote:
> Pavel, would you please review this change?
> 
> Side note, yesterday I was looking for the crc32c of an empty string (or at
> least what leveldb's implementation returns). Turns out it's 0.

Just uploaded patch set #5, which fixes the problem with AttachmentStoreTest you
pointed out a few minutes ago.  PTAL.

[pavely, 2015-03-07 00:07:25]

lgtm with few comments.

https://codereview.chromium.org/982883002/diff/60001/sync/api/attachments/att...
File sync/api/attachments/attachment_metadata_unittest.cc (right):

https://codereview.chromium.org/982883002/diff/60001/sync/api/attachments/att...
sync/api/attachments/attachment_metadata_unittest.cc:17: AttachmentMetadata
metadata(id, size);
Now AttachmentMetadata has size both in Id and as separate field. Could you
leave TODO comment for me in attachment_metadata.h to sort it out?

https://codereview.chromium.org/982883002/diff/60001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_downloader_impl.cc (right):

https://codereview.chromium.org/982883002/diff/60001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_downloader_impl.cc:196:
ReportResult(download_state, result, attachment_data);
I feel there should be a check here to make sure that crc calculated on data
matches crc from id.

https://codereview.chromium.org/982883002/diff/60001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_store_test_template.h (right):

https://codereview.chromium.org/982883002/diff/60001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_store_test_template.h:138:
AttachmentId::Create(this->some_data2->size(), crc32c), this->some_data2);
Restore the spirit. Discussed in person.

https://codereview.chromium.org/982883002/diff/60001/sync/internal_api/attach...
File sync/internal_api/attachments/on_disk_attachment_store.cc (right):

https://codereview.chromium.org/982883002/diff/60001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:324: new
Attachment(Attachment::CreateFromParts(attachment_id, data)));
Maybe check that attachment_id's crc matches the one from metadata record?
You can just leave TODO for me if you want. I'll be partying on this code next
week.

[maniscalco, 2015-03-09 17:09:00]

Thanks for the feedback.  All applied.

https://codereview.chromium.org/982883002/diff/60001/sync/api/attachments/att...
File sync/api/attachments/attachment_metadata_unittest.cc (right):

https://codereview.chromium.org/982883002/diff/60001/sync/api/attachments/att...
sync/api/attachments/attachment_metadata_unittest.cc:17: AttachmentMetadata
metadata(id, size);
On 2015/03/07 00:07:25, pavely wrote:
> Now AttachmentMetadata has size both in Id and as separate field. Could you
> leave TODO comment for me in attachment_metadata.h to sort it out?

Done (added TODO to attachment_metadata.h).

https://codereview.chromium.org/982883002/diff/60001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_downloader_impl.cc (right):

https://codereview.chromium.org/982883002/diff/60001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_downloader_impl.cc:196:
ReportResult(download_state, result, attachment_data);
On 2015/03/07 00:07:25, pavely wrote:
> I feel there should be a check here to make sure that crc calculated on data
> matches crc from id.

Done and added test case to verify.

https://codereview.chromium.org/982883002/diff/60001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_store_test_template.h (right):

https://codereview.chromium.org/982883002/diff/60001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_store_test_template.h:138:
AttachmentId::Create(this->some_data2->size(), crc32c), this->some_data2);
On 2015/03/07 00:07:25, pavely wrote:
> Restore the spirit. Discussed in person.

Done.

https://codereview.chromium.org/982883002/diff/60001/sync/internal_api/attach...
File sync/internal_api/attachments/on_disk_attachment_store.cc (right):

https://codereview.chromium.org/982883002/diff/60001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:324: new
Attachment(Attachment::CreateFromParts(attachment_id, data)));
On 2015/03/07 00:07:25, pavely wrote:
> Maybe check that attachment_id's crc matches the one from metadata record?
> You can just leave TODO for me if you want. I'll be partying on this code next
> week.

Good idea.  Done with test case to verify.

[pavely, 2015-03-09 18:14:06]

still lgtm

[maniscalco, 2015-03-09 21:54:49]

The CQ bit was checked by maniscalco@chromium.org

[maniscalco, 2015-03-09 21:54:49]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/982883002/#ps140001
(title: "Merge with master.")

[commit-bot: I haz the power, 2015-03-09 21:56:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/982883002/140001

[commit-bot: I haz the power, 2015-03-10 00:40:52]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-03-10 00:41:48]

Patchset 8 (id:??) landed as
https://crrev.com/23ae3128db0d84a6b1ffa640568a5ec90cfc8808
Cr-Commit-Position: refs/heads/master@{#319794}

[dgrogan, 2015-03-10 01:32:17]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/995683002/ by dgrogan@chromium.org.

The reason for reverting is: Broke build on iOS_Device

http://build.chromium.org/p/chromium.mac/builders/iOS_Device/builds/26684/ste...

Undefined symbols for architecture armv7:
  "syncer::Attachment::CreateFromParts(syncer::AttachmentId const&,
scoped_refptr<base::RefCountedMemory> const&, unsigned int)", referenced from:
      syncer::(anonymous
namespace)::MockAttachmentStore::RespondToRead(std::__1::set<syncer::AttachmentId,
std::__1::less<syncer::AttachmentId>, std::__1::allocator<syncer::AttachmentId>
> const&) in attachment_service_impl_unittest.o
      syncer::(anonymous
namespace)::MockAttachmentDownloader::RespondToDownload(syncer::AttachmentId
const&, syncer::AttachmentDownloader::DownloadResult const&) in
attachment_service_impl_unittest.o
     
syncer::gtest_case_AttachmentStoreTest_::Write_NoOverwriteNoError<syncer::InMemoryAttachmentStoreFactory>::TestBody()
in in_memory_attachment_store_unittest.o
      syncer::OnDiskAttachmentStoreSpecificTest_MismatchedCrc_Test::TestBody()
in on_disk_attachment_store_unittest.o
     
syncer::gtest_case_AttachmentStoreTest_::Write_NoOverwriteNoError<syncer::OnDiskAttachmentStoreFactory>::TestBody()
in on_disk_attachment_store_unittest.o
  "syncer::CreateAttachmentIdProto()", referenced from:
      syncer::AddAttachment(sync_pb::AttachmentMetadata*, bool) in
directory_commit_contribution_unittest.o
     
syncer::DirectoryUpdateHandlerProcessUpdateTest_ProcessAndApplyUpdatesWithAttachments_Test::TestBody()
in directory_update_handler_unittest.o
     
syncer::DirectoryUpdateHandlerApplyUpdateTest_SimpleConflictDifferentAttachmentMetadata_Test::TestBody()
in directory_update_handler_unittest.o
     
syncer::DirectoryUpdateHandlerApplyUpdateTest_SimpleConflictSameAttachmentMetadataDifferentOrder_Test::TestBody()
in directory_update_handler_unittest.o
      syncer::AttachmentIdProtoTest_UniqueIdFormat_Test::TestBody() in
attachment_id_proto_unittest.o
     
syncer::AttachmentIdProtoTest_CreateAttachmentMetadata_NonEmpty_Test::TestBody()
in attachment_id_proto_unittest.o
     
syncer::SyncManagerChangeProcessingTest_AttachmentMetadataOnlyChanges_Test::TestBody()
in sync_manager_impl_unittest.o
      ...
  "syncer::AttachmentId::Create()", referenced from:
      syncer::(anonymous
namespace)::SyncDataTest_CreateLocalDataWithAttachments_Test::TestBody() in
sync_data_unittest.o
      syncer::AttachmentDownloaderImplTest_HappyCase_Test::TestBody() in
attachment_downloader_impl_unittest.o
     
syncer::AttachmentDownloaderImplTest_SameIdMultipleDownloads_Test::TestBody() in
attachment_downloader_impl_unittest.o
     
syncer::AttachmentDownloaderImplTest_RequestAccessTokenFails_Test::TestBody() in
attachment_downloader_impl_unittest.o
      syncer::AttachmentDownloaderImplTest_URLFetcher_BadToken_Test::TestBody()
in attachment_downloader_impl_unittest.o
     
syncer::AttachmentDownloaderImplTest_URLFetcher_ServiceUnavailable_Test::TestBody()
in attachment_downloader_impl_unittest.o
      syncer::AttachmentDownloaderImplTest_NoHash_Test::TestBody() in
attachment_downloader_impl_unittest.o
      ...
ld: symbol(s) not found for architecture armv7
clang:error: linker command failed with exit code 1 (use -v to see invocation).

[maniscalco, 2015-03-11 16:28:02]

The CQ bit was checked by maniscalco@chromium.org

[maniscalco, 2015-03-11 16:28:05]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/982883002/#ps160001
(title: "Merge with master.")

[commit-bot: I haz the power, 2015-03-11 16:28:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/982883002/160001

[maniscalco, 2015-03-11 17:11:57]

On 2015/03/11 16:28:24, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/982883002/160001

Relanding now that https://codereview.chromium.org/986743004 has landed.

[commit-bot: I haz the power, 2015-03-11 17:36:42]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-03-11 17:37:44]

Patchset 9 (id:??) landed as
https://crrev.com/4c80a31653994ec6f112985b3493b1809e8e3415
Cr-Commit-Position: refs/heads/master@{#320103}