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}

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

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_;

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());
 }
 

sync/api/sync_data.cc

@@ -16,17 +16,8 @@
 #include "sync/protocol/proto_value_conversions.h"
 #include "sync/protocol/sync.pb.h"
 
-using syncer::Attachment;
-using syncer::AttachmentIdList;
-using syncer::AttachmentList;
-
 namespace {
 
-sync_pb::AttachmentIdProto AttachmentToProto(
-    const syncer::Attachment& attachment) {
-  return attachment.GetId().GetProto();
-}
-
 sync_pb::AttachmentIdProto IdToProto(
     const syncer::AttachmentId& attachment_id) {
   return attachment_id.GetProto();
@@ -36,19 +27,12 @@ syncer::AttachmentId ProtoToId(const sync_pb::AttachmentIdProto& proto) {
   return syncer::AttachmentId::CreateFromProto(proto);
 }
 
-// Return true iff |attachments| contains one or more elements with the same
-// AttachmentId.
-bool ContainsDuplicateAttachments(const syncer::AttachmentList& attachments) {
-  std::set<syncer::AttachmentId> id_set;
-  AttachmentList::const_iterator iter = attachments.begin();
-  AttachmentList::const_iterator end = attachments.end();
-  for (; iter != end; ++iter) {
-    if (id_set.find(iter->GetId()) != id_set.end()) {
-      return true;
-    }
-    id_set.insert(iter->GetId());
-  }
-  return false;
+// Return true iff |attachment_ids| contains duplicates.
+bool ContainsDuplicateAttachments(
+    const syncer::AttachmentIdList& attachment_ids) {
+  syncer::AttachmentIdSet id_set;
+  id_set.insert(attachment_ids.begin(), attachment_ids.end());
+  return id_set.size() != attachment_ids.size();
 }
 
 }  // namespace
@@ -82,13 +66,11 @@ SyncData::SyncData() : id_(kInvalidId), is_valid_(false) {}
 
 SyncData::SyncData(int64 id,
                    sync_pb::SyncEntity* entity,
-                   AttachmentList* attachments,
                    const base::Time& remote_modification_time,
                    const syncer::AttachmentServiceProxy& attachment_service)
     : id_(id),
       remote_modification_time_(remote_modification_time),
       immutable_entity_(entity),
-      attachments_(attachments),
       attachment_service_(attachment_service),
       is_valid_(true) {}
 
@@ -106,9 +88,9 @@ SyncData SyncData::CreateLocalDelete(const std::string& sync_tag,
 SyncData SyncData::CreateLocalData(const std::string& sync_tag,
                                    const std::string& non_unique_title,
                                    const sync_pb::EntitySpecifics& specifics) {
-  syncer::AttachmentList attachments;
+  syncer::AttachmentIdList attachment_ids;
   return CreateLocalDataWithAttachments(
-      sync_tag, non_unique_title, specifics, attachments);
+      sync_tag, non_unique_title, specifics, attachment_ids);
 }
 
 // Static.
@@ -116,20 +98,18 @@ SyncData SyncData::CreateLocalDataWithAttachments(
     const std::string& sync_tag,
     const std::string& non_unique_title,
     const sync_pb::EntitySpecifics& specifics,
-    const AttachmentList& attachments) {
-  DCHECK(!ContainsDuplicateAttachments(attachments));
+    const AttachmentIdList& attachment_ids) {
+  DCHECK(!ContainsDuplicateAttachments(attachment_ids));
   sync_pb::SyncEntity entity;
   entity.set_client_defined_unique_tag(sync_tag);
   entity.set_non_unique_name(non_unique_title);
   entity.mutable_specifics()->CopyFrom(specifics);
-  std::transform(attachments.begin(),
-                 attachments.end(),
+  std::transform(attachment_ids.begin(),
+                 attachment_ids.end(),
                  RepeatedFieldBackInserter(entity.mutable_attachment_id()),
-                 AttachmentToProto);
-  AttachmentList copy_of_attachments(attachments);
+                 IdToProto);
   return SyncData(kInvalidId,
                   &entity,
-                  &copy_of_attachments,
                   base::Time(),
                   AttachmentServiceProxy());
 }
@@ -148,9 +128,7 @@ SyncData SyncData::CreateRemoteData(
                  attachment_ids.end(),
                  RepeatedFieldBackInserter(entity.mutable_attachment_id()),
                  IdToProto);
-  AttachmentList attachments;
-  return SyncData(
-      id, &entity, &attachments, modification_time, attachment_service);
+  return SyncData(id, &entity, modification_time, attachment_service);
 }
 
 bool SyncData::IsValid() const { return is_valid_; }
@@ -215,10 +193,6 @@ SyncDataLocal::SyncDataLocal(const SyncData& sync_data) : SyncData(sync_data) {
 
 SyncDataLocal::~SyncDataLocal() {}
 
-const AttachmentList& SyncDataLocal::GetLocalAttachmentsForUpload() const {
-  return attachments_.Get();
-}
-
 const std::string& SyncDataLocal::GetTag() const {
   return immutable_entity_.Get().client_defined_unique_tag();
 }

sync/api/sync_data.h

@@ -14,7 +14,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/stl_util.h"
 #include "base/time/time.h"
-#include "sync/api/attachments/attachment.h"
+#include "sync/api/attachments/attachment_id.h"
 #include "sync/base/sync_export.h"
 #include "sync/internal_api/public/attachments/attachment_service_proxy.h"
 #include "sync/internal_api/public/base/model_type.h"
@@ -55,7 +55,7 @@ class SYNC_EXPORT SyncData {
   // primarily for debug purposes, and will be overwritten if the datatype is
   // encrypted.
   //
-  // For data with attachments: |attachments| must not contain duplicates.
+  // For data with attachments: |attachment_ids| must not contain duplicates.
   static SyncData CreateLocalDelete(
       const std::string& sync_tag,
       ModelType datatype);
@@ -67,7 +67,7 @@ class SYNC_EXPORT SyncData {
       const std::string& sync_tag,
       const std::string& non_unique_title,
       const sync_pb::EntitySpecifics& specifics,
-      const AttachmentList& attachments);
+      const AttachmentIdList& attachment_ids);
 
   // Helper method for creating SyncData objects originating from the syncer.
   static SyncData CreateRemoteData(
@@ -136,8 +136,6 @@ class SYNC_EXPORT SyncData {
   // The actual shared sync entity being held.
   ImmutableSyncEntity immutable_entity_;
 
-  Immutable<AttachmentList> attachments_;
-
   AttachmentServiceProxy attachment_service_;
 
  private:
@@ -147,7 +145,6 @@ class SYNC_EXPORT SyncData {
   // Clears |entity| and |attachments|.
   SyncData(int64 id,
            sync_pb::SyncEntity* entity,
-           AttachmentList* attachments,
            const base::Time& remote_modification_time,
            const syncer::AttachmentServiceProxy& attachment_service);
 };
@@ -161,9 +158,6 @@ class SYNC_EXPORT SyncDataLocal : public SyncData {
   explicit SyncDataLocal(const SyncData& sync_data);
   ~SyncDataLocal();
 
-  // Return a list of this SyncData's attachments.
-  const AttachmentList& GetLocalAttachmentsForUpload() const;
-
   // Return the value of the unique client tag. This is only set for data going
   // TO the syncer, not coming from.
   const std::string& GetTag() const;

sync/api/sync_data_unittest.cc

@@ -71,30 +71,28 @@ TEST_F(SyncDataTest, CreateLocalData) {
 
 TEST_F(SyncDataTest, CreateLocalDataWithAttachments) {
   specifics.mutable_preference();
-  scoped_refptr<base::RefCountedMemory> bytes(new base::RefCountedString);
-  AttachmentList attachments;
-  attachments.push_back(Attachment::Create(bytes));
-  attachments.push_back(Attachment::Create(bytes));
-  attachments.push_back(Attachment::Create(bytes));
+  AttachmentIdList attachment_ids;
+  attachment_ids.push_back(AttachmentId::Create());
+  attachment_ids.push_back(AttachmentId::Create());
+  attachment_ids.push_back(AttachmentId::Create());
 
   SyncData data = SyncData::CreateLocalDataWithAttachments(
-      kSyncTag, kNonUniqueTitle, specifics, attachments);
+      kSyncTag, kNonUniqueTitle, specifics, attachment_ids);
   EXPECT_TRUE(data.IsValid());
   EXPECT_TRUE(data.IsLocal());
   EXPECT_EQ(kSyncTag, SyncDataLocal(data).GetTag());
   EXPECT_EQ(kDatatype, data.GetDataType());
   EXPECT_EQ(kNonUniqueTitle, data.GetTitle());
   EXPECT_TRUE(data.GetSpecifics().has_preference());
-  AttachmentIdList attachment_ids = data.GetAttachmentIds();
+  attachment_ids = data.GetAttachmentIds();
   EXPECT_EQ(3U, attachment_ids.size());
-  EXPECT_EQ(3U, SyncDataLocal(data).GetLocalAttachmentsForUpload().size());
 }
 
 TEST_F(SyncDataTest, CreateLocalDataWithAttachments_EmptyListOfAttachments) {
   specifics.mutable_preference();
-  AttachmentList attachments;
+  AttachmentIdList attachment_ids;
   SyncData data = SyncData::CreateLocalDataWithAttachments(
-      kSyncTag, kNonUniqueTitle, specifics, attachments);
+      kSyncTag, kNonUniqueTitle, specifics, attachment_ids);
   EXPECT_TRUE(data.IsValid());
   EXPECT_TRUE(data.IsLocal());
   EXPECT_EQ(kSyncTag, SyncDataLocal(data).GetTag());
@@ -102,7 +100,6 @@ TEST_F(SyncDataTest, CreateLocalDataWithAttachments_EmptyListOfAttachments) {
   EXPECT_EQ(kNonUniqueTitle, data.GetTitle());
   EXPECT_TRUE(data.GetSpecifics().has_preference());
   EXPECT_TRUE(data.GetAttachmentIds().empty());
-  EXPECT_TRUE(SyncDataLocal(data).GetLocalAttachmentsForUpload().empty());
 }
 
 TEST_F(SyncDataTest, CreateRemoteData) {