Pass AttachmentIdList instead of AttachmentList to SyncData

components/sync_driver/generic_change_processor.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/sync_driver/generic_change_processor.h"
 
 #include "base/location.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/sync_driver/sync_api_component_factory.h"
@@ skipping 67 matching lines @@
   sync_pb::EntitySpecifics password_holder;
   password_holder.mutable_password()->mutable_client_only_encrypted_data()->
       CopyFrom(read_node.GetPasswordSpecifics());
   return syncer::SyncData::CreateRemoteData(sync_id,
                                             password_holder,
                                             read_node.GetModificationTime(),
                                             attachment_ids,
                                             attachment_service_proxy);
 }
 
-const syncer::AttachmentId& AttachmentToAttachmentId(
-    const syncer::Attachment& attachment) {
-  return attachment.GetId();
-}
-
 }  // namespace
 
 GenericChangeProcessor::GenericChangeProcessor(
     DataTypeErrorHandler* error_handler,
     const base::WeakPtr<syncer::SyncableService>& local_service,
     const base::WeakPtr<syncer::SyncMergeResult>& merge_result,
     syncer::UserShare* user_share,
     SyncApiComponentFactory* sync_factory,
     const scoped_refptr<syncer::AttachmentStore>& attachment_store)
     : ChangeProcessor(error_handler),
@@ skipping 300 matching lines @@
 
 }  // namespace
 
 syncer::SyncError GenericChangeProcessor::ProcessSyncChanges(
     const tracked_objects::Location& from_here,
     const syncer::SyncChangeList& list_of_changes) {
   DCHECK(CalledOnValidThread());
 
   // 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());
 
   syncer::ModelType type = syncer::UNSPECIFIED;
 
   for (syncer::SyncChangeList::const_iterator iter = list_of_changes.begin();
        iter != list_of_changes.end();
        ++iter) {
     const syncer::SyncChange& change = *iter;
     DCHECK_NE(change.sync_data().GetDataType(), syncer::UNSPECIFIED);
@@ skipping 45 matching lines @@
       syncer::SyncError error(
           FROM_HERE,
           syncer::SyncError::DATATYPE_ERROR,
           "Datatype performs attachment operation without initializing "
           "attachment store",
           type);
       error_handler()->OnSingleDataTypeUnrecoverableError(error);
       NOTREACHED();
       return error;
     }
-    StoreAndUploadAttachments(new_attachments);
+    UploadAttachments(new_attachments);
   }
 
   return syncer::SyncError();
 }
 
 // WARNING: this code is sensitive to compiler optimizations. Be careful
 // modifying any code around an OnSingleDataTypeUnrecoverableError call, else
 // the compiler attempts to merge it with other calls, losing useful information
 // in breakpad uploads.
 syncer::SyncError GenericChangeProcessor::HandleActionAdd(
     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) {
+    syncer::AttachmentIdList* new_attachments) {
   // TODO(sync): Handle other types of creation (custom parents, folders,
   // etc.).
   syncer::ReadNode root_node(&trans);
   const syncer::SyncDataLocal sync_data_local(change.sync_data());
   if (root_node.InitTypeRoot(sync_data_local.GetDataType()) !=
       syncer::BaseNode::INIT_OK) {
     syncer::SyncError error(FROM_HERE,
                             syncer::SyncError::DATATYPE_ERROR,
                             "Failed to look up root node for type " + type_str,
                             type);
@@ skipping 47 matching lines @@
       }
     }
   }
   sync_node->SetTitle(change.sync_data().GetTitle());
   SetNodeSpecifics(sync_data_local.GetSpecifics(), 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_added(merge_result_->num_items_added() + 1);
   }
   return syncer::SyncError();
 }
 // WARNING: this code is sensitive to compiler optimizations. Be careful
 // modifying any code around an OnSingleDataTypeUnrecoverableError call, else
 // the compiler attempts to merge it with other calls, losing useful information
 // in breakpad uploads.
 syncer::SyncError GenericChangeProcessor::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) {
+    syncer::AttachmentIdList* new_attachments) {
   // TODO(zea): consider having this logic for all possible changes?
 
   const syncer::SyncDataLocal sync_data_local(change.sync_data());
   syncer::BaseNode::InitByLookupResult result =
       sync_node->InitByClientTagLookup(sync_data_local.GetDataType(),
                                        sync_data_local.GetTag());
   if (result != syncer::BaseNode::INIT_OK) {
     std::string error_prefix = "Failed to load " + type_str + " node. " +
                                change.location().ToString() + ", ";
     if (result == syncer::BaseNode::INIT_FAILED_PRECONDITION) {
@@ skipping 61 matching lines @@
                     type);
         error_handler()->OnSingleDataTypeUnrecoverableError(error);
         LOG(ERROR) << "Update: encr case 4.";
         return error;
       }
     }
   }
 
   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() +
                                           1);
   }
   // TODO(sync): Support updating other parts of the sync node (title,
   // successor, parent, etc.).
   return syncer::SyncError();
 }
 
@@ skipping 31 matching lines @@
 }
 
 void GenericChangeProcessor::StartImpl() {
 }
 
 syncer::UserShare* GenericChangeProcessor::share_handle() const {
   DCHECK(CalledOnValidThread());
   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.

[maniscalco, 2014/09/12 23:23:33]

This comment is now out of date.

[pavely, 2014/09/15 17:41:49]

I removed this comment. It now becomes datatype's responsibility to call
ProcessSyncChanges only after attachment is stored with AttachmentStore.
On 2014/09/12 23:23:33, maniscalco wrote:

-  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