Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(51)

Issue 538403004: Revert of Replace AttachmentStore's StoreAttachments with UploadAttachments. (Closed)

Created:
6 years, 3 months ago by Noam Samuel
Modified:
6 years, 3 months ago
Reviewers:
maniscalco, pavely
CC:
chromium-reviews, tim+watch_chromium.org, haitaol+watch_chromium.org, zea+watch_chromium.org, maniscalco+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Revert of Replace AttachmentStore's StoreAttachments with UploadAttachments. (patchset #6 id:100001 of https://codereview.chromium.org/512413003/) Reason for revert: Chromium GPU build breakage: http://build.chromium.org/p/chromium.gpu/builders/GPU%20Linux%20Builder/builds/17869 Original issue's description: > Replace AttachmentStore's StoreAttachments with UploadAttachments. > > This change is lays groundwork for making attachment upload operations > persistent. > > Make GenericChangeProcessor responsible for writing attachments to the > AttachmentStore and calling UploadAttachments. In a future CL, datatype > code will be responsible for writing attachments to the store. > > Queue up attachments for upload inside of AttachmentService. In a > future CL we'll add rate limiting, retry, and back-off logic. > > Expose AttachmentService's AttachmentStore via GetStore method. > > BUG= > > Committed: https://chromium.googlesource.com/chromium/src/+/15e22ff398406c253ca172047e493cadf5252c73 TBR=pavely@chromium.org,maniscalco@chromium.org NOTREECHECKS=true NOTRY=true BUG=

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+251 lines, -369 lines) Patch
M components/sync_driver/generic_change_processor.h View 3 chunks +0 lines, -15 lines 0 comments Download
M components/sync_driver/generic_change_processor.cc View 5 chunks +21 lines, -44 lines 0 comments Download
M components/sync_driver/generic_change_processor_unittest.cc View 7 chunks +26 lines, -35 lines 0 comments Download
M sync/internal_api/attachments/attachment_service_impl.cc View 5 chunks +31 lines, -63 lines 0 comments Download
M sync/internal_api/attachments/attachment_service_impl_unittest.cc View 5 chunks +103 lines, -139 lines 0 comments Download
M sync/internal_api/attachments/attachment_service_proxy.cc View 5 chunks +22 lines, -15 lines 0 comments Download
M sync/internal_api/attachments/attachment_service_proxy_unittest.cc View 6 chunks +22 lines, -13 lines 0 comments Download
M sync/internal_api/public/attachments/attachment_service.h View 4 chunks +15 lines, -17 lines 0 comments Download
M sync/internal_api/public/attachments/attachment_service_impl.h View 4 chunks +7 lines, -20 lines 0 comments Download
M sync/internal_api/public/attachments/attachment_service_proxy.h View 2 chunks +4 lines, -8 lines 0 comments Download

Messages

Total messages: 4 (1 generated)
Noam Samuel
Created Revert of Replace AttachmentStore's StoreAttachments with UploadAttachments.
6 years, 3 months ago (2014-09-05 17:10:07 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/538403004/1
6 years, 3 months ago (2014-09-05 17:11:31 UTC) #2
commit-bot: I haz the power
6 years, 3 months ago (2014-09-05 17:12:23 UTC) #4
Failed to apply patch for components/sync_driver/generic_change_processor.cc:
While running git apply --index -p1;
  error: patch failed: components/sync_driver/generic_change_processor.cc:85
  error: components/sync_driver/generic_change_processor.cc: patch does not
apply

Patch:       components/sync_driver/generic_change_processor.cc
Index: components/sync_driver/generic_change_processor.cc
diff --git a/components/sync_driver/generic_change_processor.cc
b/components/sync_driver/generic_change_processor.cc
index
ddf6b676eb45a697d3d8b0a12482197bb82ab9ef..c44ce72a1bd23d1fa5a8e377c50903bc9ef83a6a
100644
--- a/components/sync_driver/generic_change_processor.cc
+++ b/components/sync_driver/generic_change_processor.cc
@@ -85,11 +85,6 @@
                                             attachment_service_proxy);
 }
 
-const syncer::AttachmentId& AttachmentToAttachmentId(
-    const syncer::Attachment& attachment) {
-  return attachment.GetId();
-}
-
 }  // namespace
 
 GenericChangeProcessor::GenericChangeProcessor(
@@ -107,8 +102,7 @@
       attachment_service_weak_ptr_factory_(attachment_service_.get()),
       attachment_service_proxy_(
           base::MessageLoopProxy::current(),
-          attachment_service_weak_ptr_factory_.GetWeakPtr()),
-      weak_ptr_factory_(this) {
+          attachment_service_weak_ptr_factory_.GetWeakPtr()) {
   DCHECK(CalledOnValidThread());
   DCHECK(attachment_service_);
 }
@@ -393,6 +387,25 @@
   return syncer::SyncError();
 }
 
+// A callback invoked on completion of AttachmentService::StoreAttachment.
+void IgnoreStoreResult(const syncer::AttachmentService::StoreResult&) {
+  // 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).
+}
+
+void StoreAttachments(syncer::AttachmentService* attachment_service,
+                      const syncer::AttachmentList& attachments) {
+  DCHECK(attachment_service);
+  syncer::AttachmentService::StoreCallback ignore_store_result =
+      base::Bind(&IgnoreStoreResult);
+  attachment_service->StoreAttachments(attachments, ignore_store_result);
+}
+
 }  // namespace
 
 syncer::SyncError GenericChangeProcessor::ProcessSyncChanges(
@@ -452,7 +465,7 @@
   }
 
   if (!new_attachments.empty()) {
-    StoreAndUploadAttachments(new_attachments);
+    StoreAttachments(attachment_service_.get(), new_attachments);
   }
 
   return syncer::SyncError();
@@ -698,40 +711,4 @@
   return share_handle_;
 }
 
-void GenericChangeProcessor::StoreAndUploadAttachments(
-    const syncer::AttachmentList& attachments) {
-  DCHECK(CalledOnValidThread());
-  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());
-  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);
-}
-
 }  // namespace sync_driver

Powered by Google App Engine
This is Rietveld 408576698