Pass AttachmentIdList instead of AttachmentList to SyncData

Datatype is responsible for writing attachments to AttachmentStore.
GenericChangeProcessor doesn't need attachment data in SyncData, only
list of attachment ids.

This change removes AttachmentList from SyncData and fixes corresponding
dependencies.

BUG=
R=maniscalco@chromium.org

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

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

3 files changed, 46 insertions, 97 deletions

diff --git a/components/sync_driver/generic_change_processor.cc b/components/sync_driver/generic_change_processor.cc
index 30b215e..9bfd0c2 100644
--- a/components/sync_driver/generic_change_processor.cc
+++ b/components/sync_driver/generic_change_processor.cc
@@ -85,11 +85,6 @@ syncer::SyncData BuildRemoteSyncData(
                                             attachment_service_proxy);
 }
 
-const syncer::AttachmentId& AttachmentToAttachmentId(
-    const syncer::Attachment& attachment) {
-  return attachment.GetId();
-}
-
 }  // namespace
 
 GenericChangeProcessor::GenericChangeProcessor(
@@ -410,7 +405,7 @@ syncer::SyncError GenericChangeProcessor::ProcessSyncChanges(
 
   // Keep track of brand new attachments so we can persist them on this device
   // and upload them to the server.
-  syncer::AttachmentList new_attachments;
+  syncer::AttachmentIdList new_attachments;
 
   syncer::WriteTransaction trans(from_here, share_handle());
 
@@ -476,7 +471,7 @@ syncer::SyncError GenericChangeProcessor::ProcessSyncChanges(
       NOTREACHED();
       return error;
     }
-    StoreAndUploadAttachments(new_attachments);
+    UploadAttachments(new_attachments);
   }
 
   return syncer::SyncError();
@@ -492,7 +487,7 @@ syncer::SyncError GenericChangeProcessor::HandleActionAdd(
     const syncer::ModelType& type,
     const syncer::WriteTransaction& trans,
     syncer::WriteNode* sync_node,
-    syncer::AttachmentList* new_attachments) {
+    syncer::AttachmentIdList* new_attachments) {
   // TODO(sync): Handle other types of creation (custom parents, folders,
   // etc.).
   syncer::ReadNode root_node(&trans);
@@ -560,12 +555,8 @@ syncer::SyncError GenericChangeProcessor::HandleActionAdd(
   SetAttachmentMetadata(attachment_ids, sync_node);
 
   // Return any newly added attachments.
-  const syncer::AttachmentList& local_attachments_for_upload =
-      sync_data_local.GetLocalAttachmentsForUpload();
-  new_attachments->insert(new_attachments->end(),
-                          local_attachments_for_upload.begin(),
-                          local_attachments_for_upload.end());
-
+  new_attachments->insert(
+      new_attachments->end(), attachment_ids.begin(), attachment_ids.end());
   if (merge_result_.get()) {
     merge_result_->set_num_items_added(merge_result_->num_items_added() + 1);
   }
@@ -581,7 +572,7 @@ syncer::SyncError GenericChangeProcessor::HandleActionUpdate(
     const syncer::ModelType& type,
     const syncer::WriteTransaction& trans,
     syncer::WriteNode* sync_node,
-    syncer::AttachmentList* new_attachments) {
+    syncer::AttachmentIdList* new_attachments) {
   // TODO(zea): consider having this logic for all possible changes?
 
   const syncer::SyncDataLocal sync_data_local(change.sync_data());
@@ -663,14 +654,12 @@ syncer::SyncError GenericChangeProcessor::HandleActionUpdate(
 
   sync_node->SetTitle(change.sync_data().GetTitle());
   SetNodeSpecifics(sync_data_local.GetSpecifics(), sync_node);
-  SetAttachmentMetadata(sync_data_local.GetAttachmentIds(), sync_node);
+  syncer::AttachmentIdList attachment_ids = sync_data_local.GetAttachmentIds();
+  SetAttachmentMetadata(attachment_ids, sync_node);
 
   // Return any newly added attachments.
-  const syncer::AttachmentList& local_attachments_for_upload =
-      sync_data_local.GetLocalAttachmentsForUpload();
-  new_attachments->insert(new_attachments->end(),
-                          local_attachments_for_upload.begin(),
-                          local_attachments_for_upload.end());
+  new_attachments->insert(
+      new_attachments->end(), attachment_ids.begin(), attachment_ids.end());
 
   if (merge_result_.get()) {
     merge_result_->set_num_items_modified(merge_result_->num_items_modified() +
@@ -722,42 +711,14 @@ syncer::UserShare* GenericChangeProcessor::share_handle() const {
   return share_handle_;
 }
 
-void GenericChangeProcessor::StoreAndUploadAttachments(
-    const syncer::AttachmentList& attachments) {
+void GenericChangeProcessor::UploadAttachments(
+    const syncer::AttachmentIdList& attachment_ids) {
   DCHECK(CalledOnValidThread());
   DCHECK(attachment_service_.get() != NULL);
-  attachment_service_->GetStore()->Write(
-      attachments,
-      base::Bind(&GenericChangeProcessor::WriteAttachmentsDone,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 attachments));
-}
-
-void GenericChangeProcessor::WriteAttachmentsDone(
-    const syncer::AttachmentList& attachments,
-    const syncer::AttachmentStore::Result& result) {
-  DCHECK(CalledOnValidThread());
-  DCHECK(attachment_service_.get() != NULL);
-  if (result != syncer::AttachmentStore::SUCCESS) {
-    // TODO(maniscalco): Deal with case where an error occurred (bug 361251).
-    return;
-  }
 
-  // TODO(maniscalco): Here is where we're going to update the in-directory
-  // entry to indicate that the attachments have been successfully stored on
-  // disk.  Why do we care?  Because we might crash after persisting the
-  // directory to disk, but before we have persisted its attachments, leaving us
-  // with danging attachment ids.  Having a flag that indicates we've stored the
-  // entry will allow us to detect and filter entries with dangling attachment
-  // ids (bug 368353).
-
-  // Begin uploading the attachments now that they are safe on disk.
-  syncer::AttachmentIdSet attachment_ids;
-  std::transform(attachments.begin(),
-                 attachments.end(),
-                 std::inserter(attachment_ids, attachment_ids.end()),
-                 AttachmentToAttachmentId);
-  attachment_service_->UploadAttachments(attachment_ids);
+  syncer::AttachmentIdSet attachment_id_set;
+  attachment_id_set.insert(attachment_ids.begin(), attachment_ids.end());
+  attachment_service_->UploadAttachments(attachment_id_set);
 }
 
 }  // namespace sync_driver
diff --git a/components/sync_driver/generic_change_processor.h b/components/sync_driver/generic_change_processor.h
index afd1ba3..f76fba5e 100644
--- a/components/sync_driver/generic_change_processor.h
+++ b/components/sync_driver/generic_change_processor.h
@@ -117,30 +117,25 @@ class GenericChangeProcessor : public ChangeProcessor,
                                     const syncer::ModelType& type,
                                     const syncer::WriteTransaction& trans,
                                     syncer::WriteNode* sync_node,
-                                    syncer::AttachmentList* new_attachments);
+                                    syncer::AttachmentIdList* new_attachments);
 
   // Logically part of ProcessSyncChanges.
   //
   // |new_attachments| is an output parameter containing newly added attachments
   // that need to be stored.  This method will append to it.
-  syncer::SyncError HandleActionUpdate(const syncer::SyncChange& change,
-                                       const std::string& type_str,
-                                       const syncer::ModelType& type,
-                                       const syncer::WriteTransaction& trans,
-                                       syncer::WriteNode* sync_node,
-                                       syncer::AttachmentList* new_attachments);
-
-  // Store |attachments| locally then upload them to the sync server.
+  syncer::SyncError HandleActionUpdate(
+      const syncer::SyncChange& change,
+      const std::string& type_str,
+      const syncer::ModelType& type,
+      const syncer::WriteTransaction& trans,
+      syncer::WriteNode* sync_node,
+      syncer::AttachmentIdList* new_attachments);
+
+  // Upload |attachments| to the sync server.
   //
-  // Store and uploading are asynchronous operations.  |WriteAttachmentsDone|
-  // will be invoked once the attachments have been stored on the local device.
-  void StoreAndUploadAttachments(const syncer::AttachmentList& attachments);
-
-  // Invoked once attachments have been stored locally.
-  //
-  // See also AttachmentStore::WriteCallback.
-  void WriteAttachmentsDone(const syncer::AttachmentList& attachments,
-                            const syncer::AttachmentStore::Result& result);
+  // This function assumes that attachments were already stored in
+  // AttachmentStore.
+  void UploadAttachments(const syncer::AttachmentIdList& attachment_ids);
 
   // The SyncableService this change processor will forward changes on to.
   const base::WeakPtr<syncer::SyncableService> local_service_;
diff --git a/components/sync_driver/generic_change_processor_unittest.cc b/components/sync_driver/generic_change_processor_unittest.cc
index 9ea4e43..ce20575 100644
--- a/components/sync_driver/generic_change_processor_unittest.cc
+++ b/components/sync_driver/generic_change_processor_unittest.cc
@@ -33,8 +33,6 @@ namespace sync_driver {
 
 namespace {
 
-const char kTestData[] = "some data";
-
 // A mock that keeps track of attachments passed to UploadAttachments.
 class MockAttachmentService : public syncer::AttachmentServiceImpl {
  public:
@@ -347,12 +345,10 @@ TEST_F(SyncGenericChangeProcessorTest,
   sync_pb::EntitySpecifics specifics;
   sync_pb::PreferenceSpecifics* pref_specifics = specifics.mutable_preference();
   pref_specifics->set_name("test");
-  syncer::AttachmentList attachments;
-  scoped_refptr<base::RefCountedString> attachment_data =
-      new base::RefCountedString;
-  attachment_data->data() = kTestData;
-  attachments.push_back(syncer::Attachment::Create(attachment_data));
-  attachments.push_back(syncer::Attachment::Create(attachment_data));
+
+  syncer::AttachmentIdList attachment_ids;
+  attachment_ids.push_back(syncer::AttachmentId::Create());
+  attachment_ids.push_back(syncer::AttachmentId::Create());
 
   // Add a SyncData with two attachments.
   syncer::SyncChangeList change_list;
@@ -360,7 +356,7 @@ TEST_F(SyncGenericChangeProcessorTest,
       syncer::SyncChange(FROM_HERE,
                          syncer::SyncChange::ACTION_ADD,
                          syncer::SyncData::CreateLocalDataWithAttachments(
-                             tag, title, specifics, attachments)));
+                             tag, title, specifics, attachment_ids)));
   ASSERT_FALSE(
       change_processor()->ProcessSyncChanges(FROM_HERE, change_list).IsSet());
   RunLoop();
@@ -369,20 +365,20 @@ TEST_F(SyncGenericChangeProcessorTest,
   ASSERT_EQ(mock_attachment_service()->attachment_id_sets()->size(), 1U);
   const syncer::AttachmentIdSet& attachments_added =
       mock_attachment_service()->attachment_id_sets()->front();
-  ASSERT_THAT(attachments_added,
-              testing::UnorderedElementsAre(attachments[0].GetId(),
-                                            attachments[1].GetId()));
+  ASSERT_THAT(
+      attachments_added,
+      testing::UnorderedElementsAre(attachment_ids[0], attachment_ids[1]));
 
   // Update the SyncData, replacing its two attachments with one new attachment.
-  syncer::AttachmentList new_attachments;
-  new_attachments.push_back(syncer::Attachment::Create(attachment_data));
+  syncer::AttachmentIdList new_attachment_ids;
+  new_attachment_ids.push_back(syncer::AttachmentId::Create());
   mock_attachment_service()->attachment_id_sets()->clear();
   change_list.clear();
   change_list.push_back(
       syncer::SyncChange(FROM_HERE,
                          syncer::SyncChange::ACTION_UPDATE,
                          syncer::SyncData::CreateLocalDataWithAttachments(
-                             tag, title, specifics, new_attachments)));
+                             tag, title, specifics, new_attachment_ids)));
   ASSERT_FALSE(
       change_processor()->ProcessSyncChanges(FROM_HERE, change_list).IsSet());
   RunLoop();
@@ -392,7 +388,7 @@ TEST_F(SyncGenericChangeProcessorTest,
   const syncer::AttachmentIdSet& new_attachments_added =
       mock_attachment_service()->attachment_id_sets()->front();
   ASSERT_THAT(new_attachments_added,
-              testing::UnorderedElementsAre(new_attachments[0].GetId()));
+              testing::UnorderedElementsAre(new_attachment_ids[0]));
 }
 
 // Verify that after attachment is uploaded GenericChangeProcessor updates
@@ -403,11 +399,9 @@ TEST_F(SyncGenericChangeProcessorTest, AttachmentUploaded) {
   sync_pb::EntitySpecifics specifics;
   sync_pb::PreferenceSpecifics* pref_specifics = specifics.mutable_preference();
   pref_specifics->set_name("test");
-  syncer::AttachmentList attachments;
-  scoped_refptr<base::RefCountedString> attachment_data =
-      new base::RefCountedString;
-  attachment_data->data() = kTestData;
-  attachments.push_back(syncer::Attachment::Create(attachment_data));
+
+  syncer::AttachmentIdList attachment_ids;
+  attachment_ids.push_back(syncer::AttachmentId::Create());
 
   // Add a SyncData with two attachments.
   syncer::SyncChangeList change_list;
@@ -415,12 +409,11 @@ TEST_F(SyncGenericChangeProcessorTest, AttachmentUploaded) {
       syncer::SyncChange(FROM_HERE,
                          syncer::SyncChange::ACTION_ADD,
                          syncer::SyncData::CreateLocalDataWithAttachments(
-                             tag, title, specifics, attachments)));
+                             tag, title, specifics, attachment_ids)));
   ASSERT_FALSE(
       change_processor()->ProcessSyncChanges(FROM_HERE, change_list).IsSet());
 
-  sync_pb::AttachmentIdProto attachment_id_proto =
-      attachments[0].GetId().GetProto();
+  sync_pb::AttachmentIdProto attachment_id_proto = attachment_ids[0].GetProto();
   syncer::AttachmentId attachment_id =
       syncer::AttachmentId::CreateFromProto(attachment_id_proto);
 
@@ -429,7 +422,7 @@ TEST_F(SyncGenericChangeProcessorTest, AttachmentUploaded) {
   syncer::ReadNode node(&read_transaction);
   ASSERT_EQ(node.InitByClientTagLookup(syncer::PREFERENCES, tag),
             syncer::BaseNode::INIT_OK);
-  syncer::AttachmentIdList attachment_ids = node.GetAttachmentIds();
+  attachment_ids = node.GetAttachmentIds();
   EXPECT_EQ(1U, attachment_ids.size());
 }