Store attachment crc in AttachmentStore

Store attachment crc in AttachmentStore

In this change:
- Add crc into Attachment class
- Rename Attachment's Create/CreateWithId to CreateNew/RestoreExisting.
- Refactor how AttachmentDownloader extracts and checks crc. Now it also
    needs to pass crc to Attachment construction
- Store crc in RecordMetadata
- Check crc when reading attachment from AttachmentStore.

BUG=417794
R=maniscalco@chromium.org

Committed: https://crrev.com/0c166910d0276c52650008f0d9464be27d146782
Cr-Commit-Position: refs/heads/master@{#304123}

[pavely, 2014-11-10 23:45:29]

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

https://codereview.chromium.org/710073003/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment.h:42: static Attachment RestoreExisting(
I wanted to generalize name since it is not create with id and crc (essentially
all attachment metadata). One option was Create/CreateWithMetadata, but word
"metadata" is overloaded. I think CreateNew/RestoreExisting reflect intention.

[maniscalco, 2014-11-11 00:44:55]

Nice work!  Here's my feedback.  It's mostly nits.

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

https://codereview.chromium.org/710073003/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment.h:42: static Attachment RestoreExisting(
On 2014/11/10 23:45:29, pavely wrote:
> I wanted to generalize name since it is not create with id and crc
(essentially
> all attachment metadata). One option was Create/CreateWithMetadata, but word
> "metadata" is overloaded. I think CreateNew/RestoreExisting reflect intention.

I see.  WDYT about CreateFromParts?  Restore in the name seems a little odd to
me, but I have a hard time explaining why exactly.

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

https://codereview.chromium.org/710073003/diff/20001/sync/api/attachments/att...
sync/api/attachments/attachment.h:53: uint32_t GetCrc32c() const;
Maybe indicate that this is a cheap operation?

e.g.

// Returns the pre-computed CRC32C for the attachment.

https://codereview.chromium.org/710073003/diff/20001/sync/api/attachments/att...
File sync/api/attachments/attachment_unittest.cc (right):

https://codereview.chromium.org/710073003/diff/20001/sync/api/attachments/att...
sync/api/attachments/attachment_unittest.cc:44: uint32_t crc =
ComputeCrc32c(some_data);
crc -> crc32c

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

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_downloader_impl.cc:155: // Fail
download only if there is useful crc in header and it doesn't
crc -> crc32c (and elsewhere in this comment)

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_downloader_impl.cc:157: // locally
calculated one will be  stored and used for further checks.
extra space between be and stored

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_downloader_impl.cc:211: uint32_t
attachment_crc) {
crc -> crc32c (and elsewhere in this file)

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_downloader_impl_unittest.cc
(right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_downloader_impl_unittest.cc:470:
TEST_F(AttachmentDownloaderImplTest, ExtractCrc32c_TooLong) {
Good one!

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_service_impl_unittest.cc (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_service_impl_unittest.cc:59: uint32_t
crc = ComputeCrc32c(data);
crc -> crc32c (and elsewhere in this file)

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_uploader_impl.cc (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:209: uint32_t crc =
attachment_.GetCrc32c();
Can be made const.  Also, how about renaming to crc32c to remove ambiguity?

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:344: std::string
AttachmentUploaderImpl::FormatCrc32cHash(uint32_t crc) {
crc -> crc32c?

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_uploader_impl_unittest.cc (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl_unittest.cc:631:
TEST_F(AttachmentUploaderImplTest, ComputeCrc32cHash) {
Rename test case to FormatCrc32Hash since that's what it's really testing?

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/fake_attachment_downloader.cc (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/fake_attachment_downloader.cc:28: uint32_t crc =
ComputeCrc32c(data);
Since there are different kinds of CRCs, I suggest renaming the var to crc32c to
remove ambiguity.  Also, it can be made const.

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

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:237: uint32_t crc =
ComputeCrc32c(data);
crc -> crc32c

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/on_disk_attachment_store_unittest.cc (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store_unittest.cc:321: //
base::FilePath db_path =
Left over from a previous revision?

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store_unittest.cc:336: 0);
Maybe put into a var named intentionally_wrong_crc32c or similar?

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/proto/attachment_store.proto (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/proto/attachment_store.proto:27: // Crc of
attachment data.
Crc -> crc32c

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/proto/attachment_store.proto:28: optional fixed32
crc32c = 2;
Why use fixed32?  I would have expected uint32.

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/public...
File sync/internal_api/public/attachments/attachment_downloader_impl.h (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/public...
sync/internal_api/public/attachments/attachment_downloader_impl.h:91: // hash in
it, this hash is correctly base64 encoded 32 but value.
I think this comment may have been mangled: "encoded 32 but value" ?

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/public...
File sync/internal_api/public/attachments/attachment_uploader_impl.h (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/public...
sync/internal_api/public/attachments/attachment_uploader_impl.h:54: // Format
crc32c to pass into X-Goog-Hash header
nit: needs a period at the end.

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/public...
File sync/internal_api/public/attachments/attachment_util.h (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/public...
sync/internal_api/public/attachments/attachment_util.h:19: uint32_t
ComputeCrc32c(const scoped_refptr<base::RefCountedMemory>& data);
WDYT about putting this on Attachment (public, static) ?  IMO it seems like a
good fit.

[pavely, 2014-11-11 22:27:15]

PTAL

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

https://codereview.chromium.org/710073003/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment.h:42: static Attachment RestoreExisting(
On 2014/11/11 00:44:53, maniscalco wrote:
> On 2014/11/10 23:45:29, pavely wrote:
> > I wanted to generalize name since it is not create with id and crc
> (essentially
> > all attachment metadata). One option was Create/CreateWithMetadata, but word
> > "metadata" is overloaded. I think CreateNew/RestoreExisting reflect
intention.
> 
> I see.  WDYT about CreateFromParts?  Restore in the name seems a little odd to
> me, but I have a hard time explaining why exactly.

The reason is Restore means something is broken, deteriorated and now we are
restoring it to glory.
I'm Ok with CreateFromParts.

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

https://codereview.chromium.org/710073003/diff/20001/sync/api/attachments/att...
sync/api/attachments/attachment.h:53: uint32_t GetCrc32c() const;
On 2014/11/11 00:44:53, maniscalco wrote:
> Maybe indicate that this is a cheap operation?
> 
> e.g.
> 
> // Returns the pre-computed CRC32C for the attachment.

Done.

https://codereview.chromium.org/710073003/diff/20001/sync/api/attachments/att...
File sync/api/attachments/attachment_unittest.cc (right):

https://codereview.chromium.org/710073003/diff/20001/sync/api/attachments/att...
sync/api/attachments/attachment_unittest.cc:44: uint32_t crc =
ComputeCrc32c(some_data);
On 2014/11/11 00:44:53, maniscalco wrote:
> crc -> crc32c

Done.

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

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_downloader_impl.cc:155: // Fail
download only if there is useful crc in header and it doesn't
On 2014/11/11 00:44:54, maniscalco wrote:
> crc -> crc32c (and elsewhere in this comment)

Done.

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_downloader_impl.cc:157: // locally
calculated one will be  stored and used for further checks.
On 2014/11/11 00:44:54, maniscalco wrote:
> extra space between be and stored

Done.

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_downloader_impl.cc:211: uint32_t
attachment_crc) {
On 2014/11/11 00:44:54, maniscalco wrote:
> crc -> crc32c (and elsewhere in this file)

Done.

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_service_impl_unittest.cc (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_service_impl_unittest.cc:59: uint32_t
crc = ComputeCrc32c(data);
On 2014/11/11 00:44:54, maniscalco wrote:
> crc -> crc32c (and elsewhere in this file)

Done.

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_uploader_impl.cc (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:209: uint32_t crc =
attachment_.GetCrc32c();
On 2014/11/11 00:44:54, maniscalco wrote:
> Can be made const.  Also, how about renaming to crc32c to remove ambiguity?

Done.

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:344: std::string
AttachmentUploaderImpl::FormatCrc32cHash(uint32_t crc) {
On 2014/11/11 00:44:54, maniscalco wrote:
> crc -> crc32c?

Done.

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_uploader_impl_unittest.cc (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl_unittest.cc:631:
TEST_F(AttachmentUploaderImplTest, ComputeCrc32cHash) {
On 2014/11/11 00:44:54, maniscalco wrote:
> Rename test case to FormatCrc32Hash since that's what it's really testing?

Done.

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/fake_attachment_downloader.cc (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/fake_attachment_downloader.cc:28: uint32_t crc =
ComputeCrc32c(data);
On 2014/11/11 00:44:54, maniscalco wrote:
> Since there are different kinds of CRCs, I suggest renaming the var to crc32c
to
> remove ambiguity.  Also, it can be made const.

Done.

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

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:237: uint32_t crc =
ComputeCrc32c(data);
On 2014/11/11 00:44:54, maniscalco wrote:
> crc -> crc32c

Done.

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/on_disk_attachment_store_unittest.cc (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store_unittest.cc:321: //
base::FilePath db_path =
On 2014/11/11 00:44:54, maniscalco wrote:
> Left over from a previous revision?

Done.

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store_unittest.cc:336: 0);
On 2014/11/11 00:44:54, maniscalco wrote:
> Maybe put into a var named intentionally_wrong_crc32c or similar?

Done.

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/proto/attachment_store.proto (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/proto/attachment_store.proto:27: // Crc of
attachment data.
On 2014/11/11 00:44:54, maniscalco wrote:
> Crc -> crc32c

Done.

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/proto/attachment_store.proto:28: optional fixed32
crc32c = 2;
On 2014/11/11 00:44:54, maniscalco wrote:
> Why use fixed32?  I would have expected uint32.

Protobuf language guide says that fixed32 "Always four bytes. More efficient
than uint32 if values are often greater than 2^28."

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/public...
File sync/internal_api/public/attachments/attachment_downloader_impl.h (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/public...
sync/internal_api/public/attachments/attachment_downloader_impl.h:91: // hash in
it, this hash is correctly base64 encoded 32 but value.
On 2014/11/11 00:44:54, maniscalco wrote:
> I think this comment may have been mangled: "encoded 32 but value" ?

Done.

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/public...
File sync/internal_api/public/attachments/attachment_uploader_impl.h (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/public...
sync/internal_api/public/attachments/attachment_uploader_impl.h:54: // Format
crc32c to pass into X-Goog-Hash header
On 2014/11/11 00:44:54, maniscalco wrote:
> nit: needs a period at the end.

Done.

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/public...
File sync/internal_api/public/attachments/attachment_util.h (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/public...
sync/internal_api/public/attachments/attachment_util.h:19: uint32_t
ComputeCrc32c(const scoped_refptr<base::RefCountedMemory>& data);
On 2014/11/11 00:44:54, maniscalco wrote:
> WDYT about putting this on Attachment (public, static) ?  IMO it seems like a
> good fit.

I'd love to but seems like I can't include third_party from sync/api. The whole
reason for creating this file is to workaround that constraint.

[maniscalco, 2014-11-11 23:05:32]

LGTM.  Happy to chat more about sync/api/attachments/DEPS if you want go that
route.

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/proto/attachment_store.proto (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/proto/attachment_store.proto:28: optional fixed32
crc32c = 2;
On 2014/11/11 22:27:15, pavely wrote:
> On 2014/11/11 00:44:54, maniscalco wrote:
> > Why use fixed32?  I would have expected uint32.
> 
> Protobuf language guide says that fixed32 "Always four bytes. More efficient
> than uint32 if values are often greater than 2^28."

Wow, so many integral types!  Looks like uint32 is more common in chrome protos,
but I'm OK with fixed32 as well.  Up to you.

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/public...
File sync/internal_api/public/attachments/attachment_util.h (right):

https://codereview.chromium.org/710073003/diff/20001/sync/internal_api/public...
sync/internal_api/public/attachments/attachment_util.h:19: uint32_t
ComputeCrc32c(const scoped_refptr<base::RefCountedMemory>& data);
On 2014/11/11 22:27:15, pavely wrote:
> On 2014/11/11 00:44:54, maniscalco wrote:
> > WDYT about putting this on Attachment (public, static) ?  IMO it seems like
a
> > good fit.
> 
> I'd love to but seems like I can't include third_party from sync/api. The
whole
> reason for creating this file is to workaround that constraint.

Ah, right, sync/api/attachments inherits sync/api's DEPS which prohibits
third_party stuff.

Another option is to add a DEPS to sync/api/attachments and allow the inclusion
of leveldb.  I'm OK with either one.

If we think we may run into this with other stuff that fits in
sync/api/attachments, we should probably go the updated DEPS route.  If we think
this is the only one, the util class seems OK.

[pavely, 2014-11-13 22:20:47]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2014-11-13 22:22:14]

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

[commit-bot: I haz the power, 2014-11-14 00:23:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-11-14 00:24:09]

Patchset 5 (id:??) landed as
https://crrev.com/0c166910d0276c52650008f0d9464be27d146782
Cr-Commit-Position: refs/heads/master@{#304123}