[Sync] Replace AttachmentIdSet with AttachmentIdList in interfaces.

Today both types are used on AttachmentService and other interfaces.
In this change I make all interfaces use AttachmentIdList.
AttachmentIdSet is only used in implementations and unittests.

R=maniscalco@chromium.org
BUG=
TEST=No observable behavior change.

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

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

components/sync_driver/generic_change_processor.cc

@@ -471,7 +471,11 @@ syncer::SyncError GenericChangeProcessor::ProcessSyncChanges(
       NOTREACHED();
       return error;
     }
-    attachment_service_->UploadAttachments(new_attachments);
+    syncer::AttachmentIdList ids_to_upload;
+    ids_to_upload.reserve(new_attachments.size());
+    std::copy(new_attachments.begin(), new_attachments.end(),
+              std::back_inserter(ids_to_upload));
+    attachment_service_->UploadAttachments(ids_to_upload);
   }
 
   return syncer::SyncError();
@@ -705,13 +709,13 @@ syncer::UserShare* GenericChangeProcessor::share_handle() const {
 void GenericChangeProcessor::UploadAllAttachmentsNotOnServer() {
   DCHECK(CalledOnValidThread());
   DCHECK(attachment_service_.get());
-  syncer::AttachmentIdSet id_set;
+  syncer::AttachmentIdList ids;
   {
     syncer::ReadTransaction trans(FROM_HERE, share_handle());
-    trans.GetAttachmentIdsToUpload(type_, &id_set);
+    trans.GetAttachmentIdsToUpload(type_, &ids);
   }
-  if (!id_set.empty()) {
-    attachment_service_->UploadAttachments(id_set);
+  if (!ids.empty()) {
+    attachment_service_->UploadAttachments(ids);
   }
 }
 

components/sync_driver/generic_change_processor_unittest.cc

@@ -40,11 +40,11 @@ class MockAttachmentService : public syncer::AttachmentServiceImpl {
   MockAttachmentService(scoped_ptr<syncer::AttachmentStore> attachment_store);
   ~MockAttachmentService() override;
   void UploadAttachments(
-      const syncer::AttachmentIdSet& attachment_ids) override;
-  std::vector<syncer::AttachmentIdSet>* attachment_id_sets();
+      const syncer::AttachmentIdList& attachment_ids) override;
+  std::vector<syncer::AttachmentIdList>* attachment_id_lists();
 
  private:
-  std::vector<syncer::AttachmentIdSet> attachment_id_sets_;
+  std::vector<syncer::AttachmentIdList> attachment_id_lists_;
 };
 
 MockAttachmentService::MockAttachmentService(
@@ -63,14 +63,14 @@ MockAttachmentService::~MockAttachmentService() {
 }
 
 void MockAttachmentService::UploadAttachments(
-    const syncer::AttachmentIdSet& attachment_ids) {
-  attachment_id_sets_.push_back(attachment_ids);
+    const syncer::AttachmentIdList& attachment_ids) {
+  attachment_id_lists_.push_back(attachment_ids);
   AttachmentServiceImpl::UploadAttachments(attachment_ids);
 }
 
-std::vector<syncer::AttachmentIdSet>*
-MockAttachmentService::attachment_id_sets() {
-  return &attachment_id_sets_;
+std::vector<syncer::AttachmentIdList>*
+MockAttachmentService::attachment_id_lists() {
+  return &attachment_id_lists_;
 }
 
 // MockSyncApiComponentFactory needed to initialize GenericChangeProcessor and
@@ -385,9 +385,9 @@ TEST_F(SyncGenericChangeProcessorTest,
   RunLoop();
 
   // Check that the AttachmentService received the new attachments.
-  ASSERT_EQ(mock_attachment_service()->attachment_id_sets()->size(), 1U);
-  const syncer::AttachmentIdSet& attachments_added =
-      mock_attachment_service()->attachment_id_sets()->front();
+  ASSERT_EQ(mock_attachment_service()->attachment_id_lists()->size(), 1U);
+  const syncer::AttachmentIdList& attachments_added =
+      mock_attachment_service()->attachment_id_lists()->front();
   ASSERT_THAT(
       attachments_added,
       testing::UnorderedElementsAre(attachment_ids[0], attachment_ids[1]));
@@ -395,7 +395,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(0, 0));
-  mock_attachment_service()->attachment_id_sets()->clear();
+  mock_attachment_service()->attachment_id_lists()->clear();
   change_list.clear();
   change_list.push_back(
       syncer::SyncChange(FROM_HERE,
@@ -407,9 +407,9 @@ TEST_F(SyncGenericChangeProcessorTest,
   RunLoop();
 
   // Check that the AttachmentService received it.
-  ASSERT_EQ(mock_attachment_service()->attachment_id_sets()->size(), 1U);
-  const syncer::AttachmentIdSet& new_attachments_added =
-      mock_attachment_service()->attachment_id_sets()->front();
+  ASSERT_EQ(mock_attachment_service()->attachment_id_lists()->size(), 1U);
+  const syncer::AttachmentIdList& new_attachments_added =
+      mock_attachment_service()->attachment_id_lists()->front();
   ASSERT_THAT(new_attachments_added,
               testing::UnorderedElementsAre(new_attachment_ids[0]));
 }
@@ -473,8 +473,8 @@ TEST_F(SyncGenericChangeProcessorTest, UploadAllAttachmentsNotOnServer) {
   // Construct the GenericChangeProcessor and see that it asks the
   // AttachmentService to upload id1 only.
   ConstructGenericChangeProcessor(kType);
-  ASSERT_EQ(1U, mock_attachment_service()->attachment_id_sets()->size());
-  ASSERT_THAT(mock_attachment_service()->attachment_id_sets()->front(),
+  ASSERT_EQ(1U, mock_attachment_service()->attachment_id_lists()->size());
+  ASSERT_THAT(mock_attachment_service()->attachment_id_lists()->front(),
               testing::UnorderedElementsAre(id1));
 }
 

sync/api/attachments/attachment_id.h

@@ -75,6 +75,8 @@ class SYNC_EXPORT AttachmentId {
   AttachmentId(sync_pb::AttachmentIdProto* proto);
 };
 
+// All public interfaces use AttachmentIdList. AttachmentIdSet is used in
+// implementations of algorithms where set properties are needed.
 typedef std::vector<AttachmentId> AttachmentIdList;
 typedef std::set<AttachmentId> AttachmentIdSet;
 

sync/internal_api/attachments/attachment_service_impl.cc

@@ -273,14 +273,13 @@ void AttachmentServiceImpl::BeginUpload(const AttachmentId& attachment_id) {
 }
 
 void AttachmentServiceImpl::UploadAttachments(
-    const AttachmentIdSet& attachment_ids) {
+    const AttachmentIdList& attachment_ids) {
   DCHECK(CalledOnValidThread());
   if (!attachment_uploader_.get()) {
     return;
   }
-  AttachmentIdSet::const_iterator iter = attachment_ids.begin();
-  AttachmentIdSet::const_iterator end = attachment_ids.end();
-  for (; iter != end; ++iter) {
+  for (auto iter = attachment_ids.begin(); iter != attachment_ids.end();
+       ++iter) {
     upload_task_queue_->AddToQueue(*iter);
   }
 }

sync/internal_api/attachments/attachment_service_impl_unittest.cc

@@ -260,6 +260,14 @@ class AttachmentServiceImplTest : public testing::Test,
     }
   }
 
+  static AttachmentIdSet AttachmentIdSetFromList(
+      const AttachmentIdList& id_list) {
+    AttachmentIdSet id_set;
+    std::copy(id_list.begin(), id_list.end(),
+              std::inserter(id_set, id_set.end()));
+    return id_set;
+  }
+
   const std::vector<AttachmentService::GetOrDownloadResult>&
   download_results() const {
     return download_results_;
@@ -420,10 +428,10 @@ TEST_F(AttachmentServiceImplTest, GetOrDownload_NoDownloader) {
 }
 
 TEST_F(AttachmentServiceImplTest, UploadAttachments_Success) {
-  AttachmentIdSet attachment_ids;
+  AttachmentIdList attachment_ids;
   const unsigned num_attachments = 3;
   for (unsigned i = 0; i < num_attachments; ++i) {
-    attachment_ids.insert(AttachmentId::Create(0, 0));
+    attachment_ids.push_back(AttachmentId::Create(0, 0));
   }
   attachment_service()->UploadAttachments(attachment_ids);
 
@@ -432,7 +440,7 @@ TEST_F(AttachmentServiceImplTest, UploadAttachments_Success) {
     // See that the service has issued a read for at least one of the
     // attachments.
     ASSERT_GE(store()->read_ids.size(), 1U);
-    store()->RespondToRead(attachment_ids);
+    store()->RespondToRead(AttachmentIdSetFromList(attachment_ids));
     RunLoop();
     ASSERT_GE(uploader()->upload_requests.size(), 1U);
     uploader()->RespondToUpload(uploader()->upload_requests.begin()->first,
@@ -444,9 +452,8 @@ TEST_F(AttachmentServiceImplTest, UploadAttachments_Success) {
 
   // See that all the attachments were uploaded.
   ASSERT_EQ(attachment_ids.size(), on_attachment_uploaded_list().size());
-  AttachmentIdSet::const_iterator iter = attachment_ids.begin();
-  const AttachmentIdSet::const_iterator end = attachment_ids.end();
-  for (iter = attachment_ids.begin(); iter != end; ++iter) {
+  for (auto iter = attachment_ids.begin(); iter != attachment_ids.end();
+       ++iter) {
     EXPECT_THAT(on_attachment_uploaded_list(), testing::Contains(*iter));
   }
 }
@@ -456,13 +463,13 @@ TEST_F(AttachmentServiceImplTest, UploadAttachments_Success_NoDelegate) {
                               make_scoped_ptr(new MockAttachmentDownloader()),
                               NULL);  // No delegate.
 
-  AttachmentIdSet attachment_ids;
-  attachment_ids.insert(AttachmentId::Create(0, 0));
+  AttachmentIdList attachment_ids;
+  attachment_ids.push_back(AttachmentId::Create(0, 0));
   attachment_service()->UploadAttachments(attachment_ids);
   RunLoopAndFireTimer();
   ASSERT_EQ(1U, store()->read_ids.size());
   ASSERT_EQ(0U, uploader()->upload_requests.size());
-  store()->RespondToRead(attachment_ids);
+  store()->RespondToRead(AttachmentIdSetFromList(attachment_ids));
   RunLoop();
   ASSERT_EQ(0U, store()->read_ids.size());
   ASSERT_EQ(1U, uploader()->upload_requests.size());
@@ -473,15 +480,15 @@ TEST_F(AttachmentServiceImplTest, UploadAttachments_Success_NoDelegate) {
 }
 
 TEST_F(AttachmentServiceImplTest, UploadAttachments_SomeMissingFromStore) {
-  AttachmentIdSet attachment_ids;
-  attachment_ids.insert(AttachmentId::Create(0, 0));
-  attachment_ids.insert(AttachmentId::Create(0, 0));
+  AttachmentIdList attachment_ids;
+  attachment_ids.push_back(AttachmentId::Create(0, 0));
+  attachment_ids.push_back(AttachmentId::Create(0, 0));
   attachment_service()->UploadAttachments(attachment_ids);
   RunLoopAndFireTimer();
   ASSERT_GE(store()->read_ids.size(), 1U);
 
   ASSERT_EQ(0U, uploader()->upload_requests.size());
-  store()->RespondToRead(attachment_ids);
+  store()->RespondToRead(AttachmentIdSetFromList(attachment_ids));
   RunLoop();
   ASSERT_EQ(1U, uploader()->upload_requests.size());
 
@@ -498,10 +505,10 @@ TEST_F(AttachmentServiceImplTest, UploadAttachments_SomeMissingFromStore) {
 }
 
 TEST_F(AttachmentServiceImplTest, UploadAttachments_AllMissingFromStore) {
-  AttachmentIdSet attachment_ids;
+  AttachmentIdList attachment_ids;
   const unsigned num_attachments = 2;
   for (unsigned i = 0; i < num_attachments; ++i) {
-    attachment_ids.insert(AttachmentId::Create(0, 0));
+    attachment_ids.push_back(AttachmentId::Create(0, 0));
   }
   attachment_service()->UploadAttachments(attachment_ids);
 
@@ -524,8 +531,8 @@ TEST_F(AttachmentServiceImplTest, UploadAttachments_NoUploader) {
                               make_scoped_ptr(new MockAttachmentDownloader()),
                               this);
 
-  AttachmentIdSet attachment_ids;
-  attachment_ids.insert(AttachmentId::Create(0, 0));
+  AttachmentIdList attachment_ids;
+  attachment_ids.push_back(AttachmentId::Create(0, 0));
   attachment_service()->UploadAttachments(attachment_ids);
   RunLoop();
   EXPECT_EQ(0U, store()->read_ids.size());
@@ -534,17 +541,17 @@ TEST_F(AttachmentServiceImplTest, UploadAttachments_NoUploader) {
 
 // Upload three attachments.  For one of them, server responds with error.
 TEST_F(AttachmentServiceImplTest, UploadAttachments_OneUploadFails) {
-  AttachmentIdSet attachment_ids;
+  AttachmentIdList attachment_ids;
   const unsigned num_attachments = 3;
   for (unsigned i = 0; i < num_attachments; ++i) {
-    attachment_ids.insert(AttachmentId::Create(0, 0));
+    attachment_ids.push_back(AttachmentId::Create(0, 0));
   }
   attachment_service()->UploadAttachments(attachment_ids);
 
   for (unsigned i = 0; i < 3; ++i) {
     RunLoopAndFireTimer();
     ASSERT_GE(store()->read_ids.size(), 1U);
-    store()->RespondToRead(attachment_ids);
+    store()->RespondToRead(AttachmentIdSetFromList(attachment_ids));
     RunLoop();
     ASSERT_EQ(1U, uploader()->upload_requests.size());
     AttachmentUploader::UploadResult result =
@@ -566,13 +573,13 @@ TEST_F(AttachmentServiceImplTest, UploadAttachments_OneUploadFails) {
 // network disconnect/connect events and see that backoff is cleared.
 TEST_F(AttachmentServiceImplTest,
        UploadAttachments_ResetBackoffAfterNetworkChange) {
-  AttachmentIdSet attachment_ids;
-  attachment_ids.insert(AttachmentId::Create(0, 0));
+  AttachmentIdList attachment_ids;
+  attachment_ids.push_back(AttachmentId::Create(0, 0));
   attachment_service()->UploadAttachments(attachment_ids);
 
   RunLoopAndFireTimer();
   ASSERT_EQ(1U, store()->read_ids.size());
-  store()->RespondToRead(attachment_ids);
+  store()->RespondToRead(AttachmentIdSetFromList(attachment_ids));
   RunLoop();
   ASSERT_EQ(1U, uploader()->upload_requests.size());
 

sync/internal_api/attachments/attachment_service_proxy.cc

@@ -66,7 +66,7 @@ void AttachmentServiceProxy::GetOrDownloadAttachments(
 }
 
 void AttachmentServiceProxy::UploadAttachments(
-    const AttachmentIdSet& attachment_ids) {
+    const AttachmentIdList& attachment_ids) {
   DCHECK(wrapped_task_runner_.get());
   wrapped_task_runner_->PostTask(
       FROM_HERE,
@@ -91,7 +91,7 @@ void AttachmentServiceProxy::Core::GetOrDownloadAttachments(
 }
 
 void AttachmentServiceProxy::Core::UploadAttachments(
-    const AttachmentIdSet& attachment_ids) {
+    const AttachmentIdList& attachment_ids) {
   if (!wrapped_) {
     return;
   }

sync/internal_api/attachments/attachment_service_proxy_unittest.cc

@@ -46,7 +46,7 @@ class StubAttachmentService : public AttachmentService,
                    base::Passed(&attachments)));
   }
 
-  void UploadAttachments(const AttachmentIdSet& attachments_ids) override {
+  void UploadAttachments(const AttachmentIdList& attachments_ids) override {
     CalledOnValidThread();
     Increment();
   }
@@ -135,7 +135,7 @@ class AttachmentServiceProxyTest : public testing::Test,
 // thread.
 TEST_F(AttachmentServiceProxyTest, MethodsAreProxied) {
   proxy->GetOrDownloadAttachments(AttachmentIdList(), callback_get_or_download);
-  proxy->UploadAttachments(AttachmentIdSet());
+  proxy->UploadAttachments(AttachmentIdList());
   // Wait for the posted calls to execute in the stub thread.
   WaitForStubThread();
   EXPECT_EQ(2, stub->GetCallCount());

sync/internal_api/public/attachments/attachment_service.h

@@ -68,7 +68,7 @@ class SYNC_EXPORT AttachmentService {
   // A request to upload attachments does not persist across restarts of Chrome.
   //
   // Invokes OnAttachmentUploaded on the Delegate (if provided).
-  virtual void UploadAttachments(const AttachmentIdSet& attachment_ids) = 0;
+  virtual void UploadAttachments(const AttachmentIdList& attachment_ids) = 0;
 };
 
 }  // namespace syncer

sync/internal_api/public/attachments/attachment_service_impl.h

@@ -64,7 +64,7 @@ class SYNC_EXPORT AttachmentServiceImpl
   // AttachmentService implementation.
   void GetOrDownloadAttachments(const AttachmentIdList& attachment_ids,
                                 const GetOrDownloadCallback& callback) override;
-  void UploadAttachments(const AttachmentIdSet& attachment_ids) override;
+  void UploadAttachments(const AttachmentIdList& attachment_ids) override;
 
   // NetworkChangeObserver implementation.
   void OnNetworkChanged(

sync/internal_api/public/attachments/attachment_service_proxy.h

@@ -52,7 +52,7 @@ class SYNC_EXPORT AttachmentServiceProxy : public AttachmentService {
 
   void GetOrDownloadAttachments(const AttachmentIdList& attachment_ids,
                                 const GetOrDownloadCallback& callback) override;
-  void UploadAttachments(const AttachmentIdSet& attachment_ids) override;
+  void UploadAttachments(const AttachmentIdList& attachment_ids) override;
 
  protected:
   // Core does the work of proxying calls to AttachmentService methods from one
@@ -78,7 +78,7 @@ class SYNC_EXPORT AttachmentServiceProxy : public AttachmentService {
     void GetOrDownloadAttachments(
         const AttachmentIdList& attachment_ids,
         const GetOrDownloadCallback& callback) override;
-    void UploadAttachments(const AttachmentIdSet& attachment_ids) override;
+    void UploadAttachments(const AttachmentIdList& attachment_ids) override;
 
    protected:
     friend class base::RefCountedThreadSafe<Core>;

sync/internal_api/public/read_transaction.h

@@ -46,9 +46,9 @@ class SYNC_EXPORT ReadTransaction : public BaseTransaction {
   void GetDataTypeContext(ModelType type,
                           sync_pb::DataTypeContext* context) const;
 
-  // Clear |id_set| and fill it with the ids of attachments that need to be
+  // Clear |ids| and fill it with the ids of attachments that need to be
   // uploaded to the sync server.
-  void GetAttachmentIdsToUpload(ModelType type, AttachmentIdSet* id_set) const;
+  void GetAttachmentIdsToUpload(ModelType type, AttachmentIdList* ids) const;
 
   // Return the current (opaque) store birthday.
   std::string GetStoreBirthday() const;

sync/internal_api/read_transaction.cc

@@ -48,10 +48,9 @@ void ReadTransaction::GetDataTypeContext(
 }
 
 void ReadTransaction::GetAttachmentIdsToUpload(ModelType type,
-                                               AttachmentIdSet* id_set) const {
-  DCHECK(id_set);
-  transaction_->directory()->GetAttachmentIdsToUpload(
-      transaction_, type, id_set);
+                                               AttachmentIdList* ids) const {
+  DCHECK(ids);
+  transaction_->directory()->GetAttachmentIdsToUpload(transaction_, type, ids);
 }
 
 std::string ReadTransaction::GetStoreBirthday() const {

sync/syncable/directory.cc

@@ -1498,13 +1498,13 @@ void Directory::UnmarkDirtyEntry(WriteTransaction* trans, Entry* entry) {
 
 void Directory::GetAttachmentIdsToUpload(BaseTransaction* trans,
                                          ModelType type,
-                                         AttachmentIdSet* id_set) {
+                                         AttachmentIdList* ids) {
   // TODO(maniscalco): Maintain an index by ModelType and rewrite this method to
   // use it.  The approach below is likely very expensive because it iterates
   // all entries (bug 415199).
   DCHECK(trans);
-  DCHECK(id_set);
-  id_set->clear();
+  DCHECK(ids);
+  ids->clear();
   AttachmentIdSet on_server_id_set;
   AttachmentIdSet not_on_server_id_set;
   std::vector<int64> metahandles;
@@ -1543,11 +1543,9 @@ void Directory::GetAttachmentIdsToUpload(BaseTransaction* trans,
   // return.
   //
   // TODO(maniscalco): Eliminate redundant metadata storage (bug 415203).
-  std::set_difference(not_on_server_id_set.begin(),
-                      not_on_server_id_set.end(),
-                      on_server_id_set.begin(),
-                      on_server_id_set.end(),
-                      std::inserter(*id_set, id_set->end()));
+  std::set_difference(not_on_server_id_set.begin(), not_on_server_id_set.end(),
+                      on_server_id_set.begin(), on_server_id_set.end(),
+                      std::back_inserter(*ids));
 }
 
 }  // namespace syncable

sync/syncable/directory.h

@@ -417,11 +417,11 @@ class SYNC_EXPORT Directory {
   // preserve sync preferences in DB on disk.
   void UnmarkDirtyEntry(WriteTransaction* trans, Entry* entry);
 
-  // Clears |id_set| and fills it with the ids of attachments that need to be
+  // Clears |ids| and fills it with the ids of attachments that need to be
   // uploaded to the sync server.
   void GetAttachmentIdsToUpload(BaseTransaction* trans,
                                 ModelType type,
-                                AttachmentIdSet* id_set);
+                                AttachmentIdList* ids);
 
  private:
   struct Kernel {

sync/syncable/directory_unittest.cc

@@ -1865,21 +1865,21 @@ TEST_F(SyncableDirectoryTest, Directory_GetAttachmentIdsToUpload) {
       PREFERENCES, "some other entry", id2, attachment_metadata);
 
   // See that Directory reports that this attachment is not on the server.
-  AttachmentIdSet id_set;
+  AttachmentIdList ids;
   {
     ReadTransaction trans(FROM_HERE, dir().get());
-    dir()->GetAttachmentIdsToUpload(&trans, PREFERENCES, &id_set);
+    dir()->GetAttachmentIdsToUpload(&trans, PREFERENCES, &ids);
   }
-  ASSERT_EQ(1U, id_set.size());
-  ASSERT_EQ(attachment_id, *id_set.begin());
+  ASSERT_EQ(1U, ids.size());
+  ASSERT_EQ(attachment_id, *ids.begin());
 
   // Call again, but this time with a ModelType for which there are no entries.
   // See that Directory correctly reports that there are none.
   {
     ReadTransaction trans(FROM_HERE, dir().get());
-    dir()->GetAttachmentIdsToUpload(&trans, PASSWORDS, &id_set);
+    dir()->GetAttachmentIdsToUpload(&trans, PASSWORDS, &ids);
   }
-  ASSERT_TRUE(id_set.empty());
+  ASSERT_TRUE(ids.empty());
 
   // Now, mark the attachment as "on the server" via entry_1.
   {
@@ -1892,9 +1892,9 @@ TEST_F(SyncableDirectoryTest, Directory_GetAttachmentIdsToUpload) {
   // server.
   {
     ReadTransaction trans(FROM_HERE, dir().get());
-    dir()->GetAttachmentIdsToUpload(&trans, PREFERENCES, &id_set);
+    dir()->GetAttachmentIdsToUpload(&trans, PREFERENCES, &ids);
   }
-  ASSERT_TRUE(id_set.empty());
+  ASSERT_TRUE(ids.empty());
 }
 
 // Verify that the directory accepts entries with unset parent ID.