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

Issue 950343006: [BackgroundSync] Initial land of the BackgroundSyncManager (Closed)

Created:
5 years, 10 months ago by jkarlin
Modified:
5 years, 9 months ago
Reviewers:
michaeln, davidben
CC:
chromium-reviews, darin-cc_chromium.org, horo+watch_chromium.org, iclelland, jam, jsbell+serviceworker_chromium.org, kinuko+watch, kinuko+serviceworker, maniscalco+watch_chromium.org, maxbogue+watch_chromium.org, nhiroki, pvalenzuela+watch_chromium.org, serviceworker-reviews, tim+watch_chromium.org, tzik, zea+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[BackgroundSync] Initial land of the BackgroundSyncManager The BackgroundSyncManager is the class that handles registration of background syncs. This first CL handles storage and retrieval of registrations and sequential ordering of async operations. Future CLs will add permission checks and incorporate a scheduler. Eventually the BackgroundSyncManager will be created and owned by the BackgroundSyncMessageFilter, which will be owned by the RenderViewHost. BackgroundSync Design Doc: https://docs.google.com/document/d/1MAuNzV0q5FporLZVJMh7CMrwTHwcZsWX6YUwPwKMGco/ BUG=449443 Committed: https://crrev.com/34ee23cfb4147f51b8ca0f728e6354f8f86db8ee Cr-Commit-Position: refs/heads/master@{#322375}

Patch Set 1 #

Patch Set 2 : GetRegistration and some cleanup #

Patch Set 3 : Remove unnecessary friend #

Patch Set 4 : Rebase #

Patch Set 5 : Compiler nit #

Patch Set 6 : fixes #

Patch Set 7 : Cleanup #

Total comments: 27

Patch Set 8 : std::string instead of string16 #

Patch Set 9 : Address comments from PS 7 #

Patch Set 10 : Rebase #

Total comments: 32

Patch Set 11 : Rebase #

Patch Set 12 : Addresses comments from PS 10 #

Total comments: 4

Patch Set 13 : Address comments from PS 12 #

Patch Set 14 : Added PRESUBMIT.py to check for clang formatting #

Total comments: 22

Patch Set 15 : Mapped registrations, more tests, register overwrites existing, unique ids for registrations #

Total comments: 39

Patch Set 16 : Address issues from PS15 #

Total comments: 4

Patch Set 17 : Address comments from PS16 #

Patch Set 18 : Rebase #

Patch Set 19 : CONTENT_EXPORT for nested classes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1210 lines, -8 lines) Patch
M content/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
A + content/browser/background_sync/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +2 lines, -2 lines 0 comments Download
A content/browser/background_sync/OWNERS View 1 chunk +1 line, -0 lines 0 comments Download
A + content/browser/background_sync/PRESUBMIT.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
A content/browser/background_sync/background_sync.proto View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +20 lines, -0 lines 0 comments Download
A content/browser/background_sync/background_sync_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +221 lines, -0 lines 0 comments Download
A content/browser/background_sync/background_sync_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +414 lines, -0 lines 0 comments Download
A content/browser/background_sync/background_sync_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +515 lines, -0 lines 0 comments Download
A content/browser/background_sync/background_sync_proto.gyp View 1 chunk +17 lines, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_context_wrapper.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_storage.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_storage.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +9 lines, -5 lines 0 comments Download
M content/content_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +3 lines, -0 lines 0 comments Download
M content/content_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +2 lines, -0 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 37 (10 generated)
jkarlin
Thanks! And sorry that this is so big.
5 years, 10 months ago (2015-02-26 17:21:20 UTC) #2
michaeln
Nice, I've been reading up on the github, designdoc, and bug. I'll take a look ...
5 years, 10 months ago (2015-02-26 22:21:48 UTC) #3
michaeln
All this look reasonable to me, I am curious about hooking this up with mojo, ...
5 years, 9 months ago (2015-02-27 02:56:30 UTC) #4
jkarlin
Thanks Michael! https://codereview.chromium.org/950343006/diff/120001/content/browser/background_sync/background_sync_manager.cc File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/120001/content/browser/background_sync/background_sync_manager.cc#newcode169 content/browser/background_sync/background_sync_manager.cc:169: for (const BackgroundSyncRegistration& reg : it->second) { ...
5 years, 9 months ago (2015-02-27 16:30:38 UTC) #5
michaeln
lgtm https://codereview.chromium.org/950343006/diff/120001/content/browser/background_sync/background_sync_manager.cc File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/120001/content/browser/background_sync/background_sync_manager.cc#newcode227 content/browser/background_sync/background_sync_manager.cc:227: reg_iter != registrations->end(); ++reg_iter) { On 2015/02/27 16:30:38, ...
5 years, 9 months ago (2015-02-28 00:34:24 UTC) #6
jkarlin
jochen@chromium.org: Please review for content/ OWNERS. Thanks! https://codereview.chromium.org/950343006/diff/120001/content/browser/background_sync/background_sync_manager.cc File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/120001/content/browser/background_sync/background_sync_manager.cc#newcode186 content/browser/background_sync/background_sync_manager.cc:186: // TODO(jkarlin): ...
5 years, 9 months ago (2015-03-02 14:41:04 UTC) #8
jkarlin
-r jochen@ since he's traveling +r davidben@chromium.org: Please review changes for content/ OWNERS. Thanks!
5 years, 9 months ago (2015-03-02 16:17:18 UTC) #11
jkarlin
On 2015/03/02 16:17:18, jkarlin wrote: > -r jochen@ since he's traveling > > +r mailto:davidben@chromium.org: ...
5 years, 9 months ago (2015-03-13 12:36:01 UTC) #12
jkarlin
On 2015/03/13 12:36:01, jkarlin wrote: > On 2015/03/02 16:17:18, jkarlin wrote: > > -r jochen@ ...
5 years, 9 months ago (2015-03-13 18:32:19 UTC) #14
davidben
Sorry about the delay. This slipped by while I was at the security summit. https://codereview.chromium.org/950343006/diff/180001/content/browser/background_sync/background_sync_manager.cc ...
5 years, 9 months ago (2015-03-13 22:05:44 UTC) #15
jkarlin
Thanks and PTAL davidben. https://codereview.chromium.org/950343006/diff/180001/content/browser/background_sync/background_sync_manager.cc File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/180001/content/browser/background_sync/background_sync_manager.cc#newcode145 content/browser/background_sync/background_sync_manager.cc:145: // TODO(jkarlin): Check for permission. ...
5 years, 9 months ago (2015-03-14 01:17:28 UTC) #16
jochen (gone - plz use gerrit)
https://codereview.chromium.org/950343006/diff/220001/content/browser/background_sync/background_sync_manager.cc File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/220001/content/browser/background_sync/background_sync_manager.cc#newcode17 content/browser/background_sync/background_sync_manager.cc:17: void RunSoon(const tracked_objects::Location& from_here, why not just post the ...
5 years, 9 months ago (2015-03-16 12:04:21 UTC) #17
jochen (gone - plz use gerrit)
deferring to David
5 years, 9 months ago (2015-03-16 12:04:52 UTC) #19
jkarlin
PTAL David, thanks! https://codereview.chromium.org/950343006/diff/220001/content/browser/background_sync/background_sync_manager.cc File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/220001/content/browser/background_sync/background_sync_manager.cc#newcode17 content/browser/background_sync/background_sync_manager.cc:17: void RunSoon(const tracked_objects::Location& from_here, On 2015/03/16 ...
5 years, 9 months ago (2015-03-16 13:12:19 UTC) #20
jkarlin
On 2015/03/16 13:12:19, jkarlin wrote: > PTAL David, thanks! > > https://codereview.chromium.org/950343006/diff/220001/content/browser/background_sync/background_sync_manager.cc > File content/browser/background_sync/background_sync_manager.cc ...
5 years, 9 months ago (2015-03-16 21:07:56 UTC) #22
davidben
On 2015/03/16 21:07:56, jkarlin wrote: > Ping davidben@, thanks! Might not be able to get ...
5 years, 9 months ago (2015-03-16 21:38:39 UTC) #23
davidben
A few more comments on what seem to be problems with unregister (sorry I didn't ...
5 years, 9 months ago (2015-03-17 18:47:15 UTC) #24
jkarlin
Thanks David, great comments. I wound up doing a bunch of things that I had ...
5 years, 9 months ago (2015-03-20 13:50:50 UTC) #26
jkarlin
PTAL David, thanks!
5 years, 9 months ago (2015-03-23 20:09:59 UTC) #27
jkarlin
On 2015/03/23 20:09:59, jkarlin wrote: > PTAL David, thanks! A friendly ping. Sorry that this ...
5 years, 9 months ago (2015-03-24 18:00:59 UTC) #28
davidben
Thanks! One more round-trip and that should be it. (Sorry my latency has been so ...
5 years, 9 months ago (2015-03-25 16:01:10 UTC) #29
jkarlin
Thanks for the bug catch! PTAL. https://codereview.chromium.org/950343006/diff/320001/content/browser/background_sync/background_sync.proto File content/browser/background_sync/background_sync.proto (right): https://codereview.chromium.org/950343006/diff/320001/content/browser/background_sync/background_sync.proto#newcode14 content/browser/background_sync/background_sync.proto:14: optional int64 min_period ...
5 years, 9 months ago (2015-03-25 19:24:19 UTC) #30
davidben
lgtm, modulo try bots not being green (looks like you need to rebase), and some ...
5 years, 9 months ago (2015-03-25 19:42:03 UTC) #31
jkarlin
https://codereview.chromium.org/950343006/diff/340001/content/browser/background_sync/background_sync_manager.cc File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/340001/content/browser/background_sync/background_sync_manager.cc#newcode128 content/browser/background_sync/background_sync_manager.cc:128: ServiceWorkerStatusCode status) { On 2015/03/25 19:42:03, David Benjamin wrote: ...
5 years, 9 months ago (2015-03-25 20:45:45 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950343006/390001
5 years, 9 months ago (2015-03-26 12:47:13 UTC) #35
commit-bot: I haz the power
Committed patchset #19 (id:390001)
5 years, 9 months ago (2015-03-26 13:59:01 UTC) #36
commit-bot: I haz the power
5 years, 9 months ago (2015-03-26 13:59:41 UTC) #37
Message was sent while issue was closed.
Patchset 19 (id:??) landed as
https://crrev.com/34ee23cfb4147f51b8ca0f728e6354f8f86db8ee
Cr-Commit-Position: refs/heads/master@{#322375}

Powered by Google App Engine
This is Rietveld 408576698