[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

Review URL: https://codereview.chromium.org/982883002

Cr-Commit-Position: refs/heads/master@{#319794}

components/sync_driver/generic_change_processor_unittest.cc

@@ -374,8 +374,8 @@ TEST_F(SyncGenericChangeProcessorTest,
   pref_specifics->set_name("test");
 
   syncer::AttachmentIdList attachment_ids;
-  attachment_ids.push_back(syncer::AttachmentId::Create());
-  attachment_ids.push_back(syncer::AttachmentId::Create());
+  attachment_ids.push_back(syncer::AttachmentId::Create(0, 0));
+  attachment_ids.push_back(syncer::AttachmentId::Create(0, 0));
 
   // Add a SyncData with two attachments.
   syncer::SyncChangeList change_list;
@@ -398,7 +398,7 @@ TEST_F(SyncGenericChangeProcessorTest,
 
   // Update the SyncData, replacing its two attachments with one new attachment.
   syncer::AttachmentIdList new_attachment_ids;
-  new_attachment_ids.push_back(syncer::AttachmentId::Create());
+  new_attachment_ids.push_back(syncer::AttachmentId::Create(0, 0));
   mock_attachment_service()->attachment_id_sets()->clear();
   change_list.clear();
   change_list.push_back(
@@ -428,7 +428,7 @@ TEST_F(SyncGenericChangeProcessorTest, AttachmentUploaded) {
   pref_specifics->set_name("test");
 
   syncer::AttachmentIdList attachment_ids;
-  attachment_ids.push_back(syncer::AttachmentId::Create());
+  attachment_ids.push_back(syncer::AttachmentId::Create(0, 0));
 
   // Add a SyncData with two attachments.
   syncer::SyncChangeList change_list;
@@ -456,8 +456,8 @@ TEST_F(SyncGenericChangeProcessorTest, AttachmentUploaded) {
 // scheduled for upload.
 TEST_F(SyncGenericChangeProcessorTest, UploadAllAttachmentsNotOnServer) {
   // Create two attachment ids.  id2 will be marked as "on server".
-  syncer::AttachmentId id1 = syncer::AttachmentId::Create();
-  syncer::AttachmentId id2 = syncer::AttachmentId::Create();
+  syncer::AttachmentId id1 = syncer::AttachmentId::Create(0, 0);
+  syncer::AttachmentId id2 = syncer::AttachmentId::Create(0, 0);
   {
     // Write an entry containing these two attachment ids.
     syncer::WriteTransaction trans(FROM_HERE, user_share());

sync/api/attachments/attachment.cc

@@ -15,15 +15,14 @@ Attachment::~Attachment() {}
 Attachment Attachment::Create(
     const scoped_refptr<base::RefCountedMemory>& data) {
   uint32_t crc32c = ComputeCrc32c(data);
-  return CreateFromParts(AttachmentId::Create(), data, crc32c);
+  return CreateFromParts(AttachmentId::Create(data->size(), crc32c), data);
 }
 
 // Static.
 Attachment Attachment::CreateFromParts(
     const AttachmentId& id,
-    const scoped_refptr<base::RefCountedMemory>& data,
-    uint32_t crc32c) {
-  return Attachment(id, data, crc32c);
+    const scoped_refptr<base::RefCountedMemory>& data) {
+  return Attachment(id, data);
 }
 
 const AttachmentId& Attachment::GetId() const { return id_; }
@@ -32,12 +31,14 @@ const scoped_refptr<base::RefCountedMemory>& Attachment::GetData() const {
   return data_;
 }
 
-uint32_t Attachment::GetCrc32c() const { return crc32c_; }
+uint32_t Attachment::GetCrc32c() const {
+  return id_.GetCrc32c();
+}
 
 Attachment::Attachment(const AttachmentId& id,
-                       const scoped_refptr<base::RefCountedMemory>& data,
-                       uint32_t crc32c)
-    : id_(id), data_(data), crc32c_(crc32c) {
+                       const scoped_refptr<base::RefCountedMemory>& data)
+    : id_(id), data_(data) {
+  DCHECK_EQ(id.GetSize(), data->size());
   DCHECK(data.get());
 }
 

sync/api/attachments/attachment.h

@@ -34,14 +34,13 @@ class SYNC_EXPORT Attachment {
   // Used when creating a brand new attachment.
   static Attachment Create(const scoped_refptr<base::RefCountedMemory>& data);
 
-  // Creates an attachment with the supplied id, data and crc32c.
+  // Creates an attachment with the supplied id and data.
   //
   // Used when you want to recreate a specific attachment. E.g. creating a local
   // copy of an attachment that already exists on the sync server.
   static Attachment CreateFromParts(
       const AttachmentId& id,
-      const scoped_refptr<base::RefCountedMemory>& data,
-      uint32_t crc32c);
+      const scoped_refptr<base::RefCountedMemory>& data);
 
   // Returns this attachment's id.
   const AttachmentId& GetId() const;
@@ -58,11 +57,9 @@ class SYNC_EXPORT Attachment {
  private:
   AttachmentId id_;
   scoped_refptr<base::RefCountedMemory> data_;
-  uint32_t crc32c_;
 
   Attachment(const AttachmentId& id,
-             const scoped_refptr<base::RefCountedMemory>& data,
-             uint32_t crc32c);
+             const scoped_refptr<base::RefCountedMemory>& data);
 };
 
 typedef std::vector<syncer::Attachment> AttachmentList;

sync/api/attachments/attachment_id.cc

@@ -52,8 +52,8 @@ bool AttachmentId::operator<(const AttachmentId& other) const {
 }
 
 // Static.
-AttachmentId AttachmentId::Create() {
-  sync_pb::AttachmentIdProto proto = CreateAttachmentIdProto();
+AttachmentId AttachmentId::Create(size_t size, uint32_t crc32c) {
+  sync_pb::AttachmentIdProto proto = CreateAttachmentIdProto(size, crc32c);
   return AttachmentId(&proto);
 }
 
@@ -71,4 +71,12 @@ const sync_pb::AttachmentIdProto& AttachmentId::GetProto() const {
 AttachmentId::AttachmentId(sync_pb::AttachmentIdProto* proto)
     : proto_(proto) {}
 
+size_t AttachmentId::GetSize() const {
+  return proto_.Get().size_bytes();
+}
+
+uint32_t AttachmentId::GetCrc32c() const {
+  return proto_.Get().crc32c();
+}
+
 }  // namespace syncer

sync/api/attachments/attachment_id.h

@@ -35,14 +35,24 @@ class SYNC_EXPORT AttachmentId {
   // Needed for using AttachmentId as key in std::map.
   bool operator<(const AttachmentId& other) const;
 
-  // Creates a unique attachment id.
-  static AttachmentId Create();
+  // Creates a unique id for an attachment.
+  //
+  // |size| is the attachment's size in bytes.
+  //
+  // |crc32c| is the attachment's crc32c.
+  static AttachmentId Create(size_t size, uint32_t crc32c);
 
   // Creates an attachment id from an initialized proto.
   static AttachmentId CreateFromProto(const sync_pb::AttachmentIdProto& proto);
 
   const sync_pb::AttachmentIdProto& GetProto() const;
 
+  // Returns the size (in bytes) the attachment.
+  size_t GetSize() const;
+
+  // Returns the crc32c the attachment.
+  uint32_t GetCrc32c() const;
+
  private:
   // Necessary since we forward-declare sync_pb::AttachmentIdProto; see comments
   // in immutable.h.

sync/api/attachments/attachment_id_unittest.cc

@@ -10,29 +10,25 @@
 
 namespace syncer {
 
-namespace {
-
-}  // namespace
-
 class AttachmentIdTest : public testing::Test {};
 
 TEST_F(AttachmentIdTest, Create_IsUnique) {
-  AttachmentId id1 = AttachmentId::Create();
-  AttachmentId id2 = AttachmentId::Create();
+  AttachmentId id1 = AttachmentId::Create(0, 0);
+  AttachmentId id2 = AttachmentId::Create(0, 0);
   EXPECT_NE(id1, id2);
 }
 
 TEST_F(AttachmentIdTest, OperatorEqual) {
-  AttachmentId id1 = AttachmentId::Create();
+  AttachmentId id1 = AttachmentId::Create(0, 0);
   AttachmentId id2(id1);
   EXPECT_EQ(id1, id2);
 }
 
 TEST_F(AttachmentIdTest, OperatorLess) {
-  AttachmentId id1 = AttachmentId::Create();
+  AttachmentId id1 = AttachmentId::Create(0, 0);
   EXPECT_FALSE(id1 < id1);
 
-  AttachmentId id2 = AttachmentId::Create();
+  AttachmentId id2 = AttachmentId::Create(0, 0);
   EXPECT_FALSE(id1 < id1);
 
   EXPECT_NE(id1, id2);

sync/api/attachments/attachment_metadata.h

@@ -30,6 +30,9 @@ class SYNC_EXPORT AttachmentMetadata {
   size_t GetSize() const;
 
  private:
+  // TODO(maniscalco): Reconcile AttachmentMetadata and
+  // AttachmentId. AttachmentId knows the size of the attachment so
+  // AttachmentMetadata may not be necessary (crbug/465375).
   AttachmentId id_;
   size_t size_;
 };

sync/api/attachments/attachment_metadata_unittest.cc

@@ -11,8 +11,9 @@ namespace syncer {
 class AttachmentMetadataTest : public testing::Test {};
 
 TEST_F(AttachmentMetadataTest, Create) {
-  AttachmentId id = AttachmentId::Create();
   size_t size = 42;
+  uint32_t crc32c = 2349829;
+  AttachmentId id = AttachmentId::Create(size, crc32c);
   AttachmentMetadata metadata(id, size);
   EXPECT_EQ(metadata.GetId(), id);
   EXPECT_EQ(metadata.GetSize(), size);

sync/api/attachments/attachment_unittest.cc

@@ -38,11 +38,11 @@ TEST_F(AttachmentTest, Create_WithEmptyData) {
 }
 
 TEST_F(AttachmentTest, CreateFromParts_HappyCase) {
-  AttachmentId id = AttachmentId::Create();
   scoped_refptr<base::RefCountedString> some_data(new base::RefCountedString);
   some_data->data() = kAttachmentData;
   uint32_t crc32c = ComputeCrc32c(some_data);
-  Attachment a = Attachment::CreateFromParts(id, some_data, crc32c);
+  AttachmentId id = AttachmentId::Create(some_data->size(), crc32c);
+  Attachment a = Attachment::CreateFromParts(id, some_data);
   EXPECT_EQ(id, a.GetId());
   EXPECT_EQ(some_data, a.GetData());
 }

sync/api/sync_data_unittest.cc

@@ -72,9 +72,9 @@ TEST_F(SyncDataTest, CreateLocalData) {
 TEST_F(SyncDataTest, CreateLocalDataWithAttachments) {
   specifics.mutable_preference();
   AttachmentIdList attachment_ids;
-  attachment_ids.push_back(AttachmentId::Create());
-  attachment_ids.push_back(AttachmentId::Create());
-  attachment_ids.push_back(AttachmentId::Create());
+  attachment_ids.push_back(AttachmentId::Create(0, 0));
+  attachment_ids.push_back(AttachmentId::Create(0, 0));
+  attachment_ids.push_back(AttachmentId::Create(0, 0));
 
   SyncData data = SyncData::CreateLocalDataWithAttachments(
       kSyncTag, kNonUniqueTitle, specifics, attachment_ids);

sync/engine/directory_commit_contribution_unittest.cc

@@ -350,7 +350,7 @@ TEST_F(DirectoryCommitContributionTest, HierarchySupport_Preferences) {
 
 void AddAttachment(sync_pb::AttachmentMetadata* metadata, bool is_on_server) {
   sync_pb::AttachmentMetadataRecord record;
-  *record.mutable_id() = CreateAttachmentIdProto();
+  *record.mutable_id() = CreateAttachmentIdProto(0, 0);
   record.set_is_on_server(is_on_server);
   *metadata->add_record() = record;
 }

sync/engine/directory_update_handler_unittest.cc

@@ -401,7 +401,7 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest,
   scoped_ptr<sync_pb::SyncEntity> e1 = CreateUpdate(
       SyncableIdToProto(Id::CreateFromServerId("e1")), "", ARTICLES);
   sync_pb::AttachmentIdProto* attachment_id = e1->add_attachment_id();
-  *attachment_id = CreateAttachmentIdProto();
+  *attachment_id = CreateAttachmentIdProto(0, 0);
 
   SyncEntityList updates;
   updates.push_back(e1.get());
@@ -1080,8 +1080,10 @@ TEST_F(DirectoryUpdateHandlerApplyUpdateTest,
   *specifics.mutable_article() = sync_pb::ArticleSpecifics();
   int64 handle = entry_factory()->CreateSyncedItem("art1", ARTICLES, is_folder);
 
-  sync_pb::AttachmentIdProto local_attachment_id = CreateAttachmentIdProto();
-  sync_pb::AttachmentIdProto server_attachment_id = CreateAttachmentIdProto();
+  sync_pb::AttachmentIdProto local_attachment_id =
+      CreateAttachmentIdProto(0, 0);
+  sync_pb::AttachmentIdProto server_attachment_id =
+      CreateAttachmentIdProto(0, 0);
 
   // Add an attachment to the local attachment metadata.
   sync_pb::AttachmentMetadata local_metadata;
@@ -1122,8 +1124,8 @@ TEST_F(DirectoryUpdateHandlerApplyUpdateTest,
   *specifics.mutable_article() = sync_pb::ArticleSpecifics();
   int64 handle = entry_factory()->CreateSyncedItem("art1", ARTICLES, is_folder);
 
-  sync_pb::AttachmentIdProto id1 = CreateAttachmentIdProto();
-  sync_pb::AttachmentIdProto id2 = CreateAttachmentIdProto();
+  sync_pb::AttachmentIdProto id1 = CreateAttachmentIdProto(0, 0);
+  sync_pb::AttachmentIdProto id2 = CreateAttachmentIdProto(0, 0);
 
   // Add id1, then id2 to the local attachment metadata.
   sync_pb::AttachmentMetadata local_metadata;

sync/internal_api/attachments/attachment_downloader_impl.cc

@@ -132,7 +132,7 @@ void AttachmentDownloaderImpl::OnGetTokenFailure(
     DownloadState* download_state = *iter;
     scoped_refptr<base::RefCountedString> null_attachment_data;
     ReportResult(*download_state, DOWNLOAD_TRANSIENT_ERROR,
-                 null_attachment_data, 0);
+                 null_attachment_data);
     DCHECK(state_map_.find(download_state->attachment_url) != state_map_.end());
     state_map_.erase(download_state->attachment_url);
   }
@@ -175,7 +175,13 @@ void AttachmentDownloaderImpl::OnURLFetchComplete(
       // locally calculated one will be stored and used for further checks.
       result = DOWNLOAD_TRANSIENT_ERROR;
     } else {
-      result = DOWNLOAD_SUCCESS;
+      // If the id's crc32c doesn't match that of the downloaded attachment,
+      // then we're stuck and retrying is unlikely to help.
+      if (attachment_crc32c != download_state.attachment_id.GetCrc32c()) {
+        result = DOWNLOAD_UNSPECIFIED_ERROR;
+      } else {
+        result = DOWNLOAD_SUCCESS;
+      }
     }
     UMA_HISTOGRAM_BOOLEAN("Sync.Attachments.DownloadChecksumResult",
                           result == DOWNLOAD_SUCCESS);
@@ -193,7 +199,7 @@ void AttachmentDownloaderImpl::OnURLFetchComplete(
   } else if (response_code == net::URLFetcher::RESPONSE_CODE_INVALID) {
     result = DOWNLOAD_TRANSIENT_ERROR;
   }
-  ReportResult(download_state, result, attachment_data, attachment_crc32c);
+  ReportResult(download_state, result, attachment_data);
   state_map_.erase(iter);
 }
 
@@ -221,16 +227,15 @@ void AttachmentDownloaderImpl::RequestAccessToken(
 void AttachmentDownloaderImpl::ReportResult(
     const DownloadState& download_state,
     const DownloadResult& result,
-    const scoped_refptr<base::RefCountedString>& attachment_data,
-    uint32_t attachment_crc32c) {
+    const scoped_refptr<base::RefCountedString>& attachment_data) {
   std::vector<DownloadCallback>::const_iterator iter;
   for (iter = download_state.user_callbacks.begin();
        iter != download_state.user_callbacks.end();
        ++iter) {
     scoped_ptr<Attachment> attachment;
     if (result == DOWNLOAD_SUCCESS) {
       attachment.reset(new Attachment(Attachment::CreateFromParts(
-          download_state.attachment_id, attachment_data, attachment_crc32c)));
+          download_state.attachment_id, attachment_data)));
     }
 
     base::MessageLoop::current()->PostTask(