Make GenericChangeProcessor upload attachments on startup.

Convert some uses of AttachmentIdList to AttachmentIdSet.

Remove the no longer needed UploadAttachments method from
GenericChangeProcessor.

BUG=372622

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

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

3 files changed, 57 insertions, 23 deletions

diff --git a/components/sync_driver/generic_change_processor.cc b/components/sync_driver/generic_change_processor.cc
index 08238e5..a6614f8 100644
--- a/components/sync_driver/generic_change_processor.cc
+++ b/components/sync_driver/generic_change_processor.cc
@@ -112,6 +112,7 @@ GenericChangeProcessor::GenericChangeProcessor(
     attachment_service_proxy_.reset(new syncer::AttachmentServiceProxy(
         base::MessageLoopProxy::current(),
         attachment_service_weak_ptr_factory_->GetWeakPtr()));
+    UploadAllAttachmentsNotOnServer();
   } else {
     attachment_service_proxy_.reset(new syncer::AttachmentServiceProxy(
         base::MessageLoopProxy::current(),
@@ -408,7 +409,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::AttachmentIdList new_attachments;
+  syncer::AttachmentIdSet new_attachments;
 
   syncer::WriteTransaction trans(from_here, share_handle());
 
@@ -470,7 +471,7 @@ syncer::SyncError GenericChangeProcessor::ProcessSyncChanges(
       NOTREACHED();
       return error;
     }
-    UploadAttachments(new_attachments);
+    attachment_service_->UploadAttachments(new_attachments);
   }
 
   return syncer::SyncError();
@@ -485,7 +486,7 @@ syncer::SyncError GenericChangeProcessor::HandleActionAdd(
     const std::string& type_str,
     const syncer::WriteTransaction& trans,
     syncer::WriteNode* sync_node,
-    syncer::AttachmentIdList* new_attachments) {
+    syncer::AttachmentIdSet* new_attachments) {
   // TODO(sync): Handle other types of creation (custom parents, folders,
   // etc.).
   syncer::ReadNode root_node(&trans);
@@ -553,8 +554,7 @@ syncer::SyncError GenericChangeProcessor::HandleActionAdd(
   SetAttachmentMetadata(attachment_ids, sync_node);
 
   // Return any newly added attachments.
-  new_attachments->insert(
-      new_attachments->end(), attachment_ids.begin(), attachment_ids.end());
+  new_attachments->insert(attachment_ids.begin(), attachment_ids.end());
   if (merge_result_.get()) {
     merge_result_->set_num_items_added(merge_result_->num_items_added() + 1);
   }
@@ -569,7 +569,7 @@ syncer::SyncError GenericChangeProcessor::HandleActionUpdate(
     const std::string& type_str,
     const syncer::WriteTransaction& trans,
     syncer::WriteNode* sync_node,
-    syncer::AttachmentIdList* new_attachments) {
+    syncer::AttachmentIdSet* new_attachments) {
   // TODO(zea): consider having this logic for all possible changes?
 
   const syncer::SyncDataLocal sync_data_local(change.sync_data());
@@ -655,8 +655,7 @@ syncer::SyncError GenericChangeProcessor::HandleActionUpdate(
   SetAttachmentMetadata(attachment_ids, sync_node);
 
   // Return any newly added attachments.
-  new_attachments->insert(
-      new_attachments->end(), attachment_ids.begin(), attachment_ids.end());
+  new_attachments->insert(attachment_ids.begin(), attachment_ids.end());
 
   if (merge_result_.get()) {
     merge_result_->set_num_items_modified(merge_result_->num_items_modified() +
@@ -703,14 +702,17 @@ syncer::UserShare* GenericChangeProcessor::share_handle() const {
   return share_handle_;
 }
 
-void GenericChangeProcessor::UploadAttachments(
-    const syncer::AttachmentIdList& attachment_ids) {
+void GenericChangeProcessor::UploadAllAttachmentsNotOnServer() {
   DCHECK(CalledOnValidThread());
-  DCHECK(attachment_service_.get() != NULL);
-
-  syncer::AttachmentIdSet attachment_id_set;
-  attachment_id_set.insert(attachment_ids.begin(), attachment_ids.end());
-  attachment_service_->UploadAttachments(attachment_id_set);
+  DCHECK(attachment_service_.get());
+  syncer::AttachmentIdSet id_set;
+  {
+    syncer::ReadTransaction trans(FROM_HERE, share_handle());
+    trans.GetAttachmentIdsToUpload(type_, &id_set);
+  }
+  if (!id_set.empty()) {
+    attachment_service_->UploadAttachments(id_set);
+  }
 }
 
 }  // namespace sync_driver
diff --git a/components/sync_driver/generic_change_processor.h b/components/sync_driver/generic_change_processor.h
index 05a7368..c370c3f 100644
--- a/components/sync_driver/generic_change_processor.h
+++ b/components/sync_driver/generic_change_processor.h
@@ -114,7 +114,7 @@ class GenericChangeProcessor : public ChangeProcessor,
                                     const std::string& type_str,
                                     const syncer::WriteTransaction& trans,
                                     syncer::WriteNode* sync_node,
-                                    syncer::AttachmentIdList* new_attachments);
+                                    syncer::AttachmentIdSet* new_attachments);
 
   // Logically part of ProcessSyncChanges.
   //
@@ -125,13 +125,11 @@ class GenericChangeProcessor : public ChangeProcessor,
       const std::string& type_str,
       const syncer::WriteTransaction& trans,
       syncer::WriteNode* sync_node,
-      syncer::AttachmentIdList* new_attachments);
+      syncer::AttachmentIdSet* new_attachments);
 
-  // Upload |attachments| to the sync server.
-  //
-  // This function assumes that attachments were already stored in
-  // AttachmentStore.
-  void UploadAttachments(const syncer::AttachmentIdList& attachment_ids);
+  // Begin uploading attachments that have not yet been uploaded to the sync
+  // server.
+  void UploadAllAttachmentsNotOnServer();
 
   const syncer::ModelType type_;
 
diff --git a/components/sync_driver/generic_change_processor_unittest.cc b/components/sync_driver/generic_change_processor_unittest.cc
index 623a509..f3c580e 100644
--- a/components/sync_driver/generic_change_processor_unittest.cc
+++ b/components/sync_driver/generic_change_processor_unittest.cc
@@ -11,6 +11,7 @@
 #include "base/strings/stringprintf.h"
 #include "components/sync_driver/data_type_error_handler_mock.h"
 #include "components/sync_driver/sync_api_component_factory.h"
+#include "sync/api/attachments/attachment_id.h"
 #include "sync/api/attachments/fake_attachment_store.h"
 #include "sync/api/fake_syncable_service.h"
 #include "sync/api/sync_change.h"
@@ -142,9 +143,12 @@ class SyncGenericChangeProcessorTest : public testing::Test {
                                         test_user_share_->user_share());
     }
     test_user_share_->encryption_handler()->Init();
+    ConstructGenericChangeProcessor(type);
+  }
+
+  void ConstructGenericChangeProcessor(syncer::ModelType type) {
     scoped_refptr<syncer::AttachmentStore> attachment_store(
         new syncer::FakeAttachmentStore(base::MessageLoopProxy::current()));
-
     scoped_ptr<MockAttachmentService> mock_attachment_service(
         new MockAttachmentService(attachment_store));
     // GenericChangeProcessor takes ownership of the AttachmentService, but we
@@ -446,6 +450,36 @@ TEST_F(SyncGenericChangeProcessorTest, AttachmentUploaded) {
   EXPECT_EQ(1U, attachment_ids.size());
 }
 
+// Verify that upon construction, all attachments not yet on the server are
+// 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();
+  {
+    // Write an entry containing these two attachment ids.
+    syncer::WriteTransaction trans(FROM_HERE, user_share());
+    syncer::ReadNode root(&trans);
+    ASSERT_EQ(syncer::BaseNode::INIT_OK, root.InitTypeRoot(kType));
+    syncer::WriteNode node(&trans);
+    node.InitUniqueByCreation(kType, root, "some node");
+    sync_pb::AttachmentMetadata metadata;
+    sync_pb::AttachmentMetadataRecord* record1 = metadata.add_record();
+    *record1->mutable_id() = id1.GetProto();
+    sync_pb::AttachmentMetadataRecord* record2 = metadata.add_record();
+    *record2->mutable_id() = id2.GetProto();
+    record2->set_is_on_server(true);
+    node.SetAttachmentMetadata(metadata);
+  }
+
+  // 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(),
+              testing::UnorderedElementsAre(id1));
+}
+
 }  // namespace
 
 }  // namespace sync_driver