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

Issue 512413003: Replace AttachmentStore's StoreAttachments with UploadAttachments. (Closed)

Created:
6 years, 3 months ago by maniscalco
Modified:
6 years, 3 months ago
Reviewers:
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

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://crrev.com/5dde06853d1d5020796184c5fc749b9730ce8f07 Cr-Commit-Position: refs/heads/master@{#293536}

Patch Set 1 #

Patch Set 2 : Merge with master. #

Patch Set 3 : Fix generic_change_processor_unittest.cc. #

Patch Set 4 : Add RunLoop to GenericChangeProcessor unit test. #

Patch Set 5 : Changes from self-review. #

Total comments: 4

Patch Set 6 : Apply feedback from pavely@ #

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

Messages

Total messages: 12 (2 generated)
maniscalco
Pavel, would you please review this change?
6 years, 3 months ago (2014-09-02 18:56:47 UTC) #2
pavely
https://codereview.chromium.org/512413003/diff/80001/components/sync_driver/generic_change_processor_unittest.cc File components/sync_driver/generic_change_processor_unittest.cc (right): https://codereview.chromium.org/512413003/diff/80001/components/sync_driver/generic_change_processor_unittest.cc#newcode366 components/sync_driver/generic_change_processor_unittest.cc:366: EXPECT_THAT(attachments_added, testing::Contains(attachments[0].GetId())); (not asking to change): I learned today ...
6 years, 3 months ago (2014-09-04 17:44:37 UTC) #3
maniscalco
Thanks for the feedback. All applied. PTAL. https://codereview.chromium.org/512413003/diff/80001/components/sync_driver/generic_change_processor_unittest.cc File components/sync_driver/generic_change_processor_unittest.cc (right): https://codereview.chromium.org/512413003/diff/80001/components/sync_driver/generic_change_processor_unittest.cc#newcode366 components/sync_driver/generic_change_processor_unittest.cc:366: EXPECT_THAT(attachments_added, testing::Contains(attachments[0].GetId())); ...
6 years, 3 months ago (2014-09-04 22:39:38 UTC) #4
pavely
lgtm
6 years, 3 months ago (2014-09-05 01:15:49 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maniscalco@chromium.org/512413003/100001
6 years, 3 months ago (2014-09-05 15:43:21 UTC) #7
commit-bot: I haz the power
Committed patchset #6 (id:100001) as 15e22ff398406c253ca172047e493cadf5252c73
6 years, 3 months ago (2014-09-05 16:46:31 UTC) #8
Noam Samuel
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/538403004/ by noamsml@chromium.org. ...
6 years, 3 months ago (2014-09-05 17:10:07 UTC) #9
maniscalco
On 2014/09/05 17:10:07, Noam Samuel wrote: > A revert of this CL (patchset #6 id:100001) ...
6 years, 3 months ago (2014-09-05 17:42:52 UTC) #10
maniscalco
On 2014/09/05 17:42:52, maniscalco wrote: > On 2014/09/05 17:10:07, Noam Samuel wrote: > > A ...
6 years, 3 months ago (2014-09-08 21:59:08 UTC) #11
commit-bot: I haz the power
6 years, 3 months ago (2014-09-10 03:40:06 UTC) #12
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/5dde06853d1d5020796184c5fc749b9730ce8f07
Cr-Commit-Position: refs/heads/master@{#293536}

Powered by Google App Engine
This is Rietveld 408576698