|
|
Created:
5 years, 10 months ago by jkarlin Modified:
5 years, 9 months ago 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 #Messages
Total messages: 37 (10 generated)
jkarlin@chromium.org changed reviewers: + michaeln@chromium.org
Thanks! And sorry that this is so big.
Nice, I've been reading up on the github, designdoc, and bug. I'll take a look later this afternoon at the CL.
All this look reasonable to me, I am curious about hooking this up with mojo, is that a good idea or not? I don't see that we're vending mojo services on the IO thread yet, and i also don't see any blink::ExecutionContext specific mojo service registries either. (the op_scheduler_ usage looks real familiar :) https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:169: for (const BackgroundSyncRegistration& reg : it->second) { thnx for not using const auto everywhere (or anywhere for that matter) https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:186: // TODO(jkarlin): This UTF conversion is incompatible with DOMString, we would it be reasonable for the spec to constrain ids to something that can be represented in utf8 and survive roundtrip conversions? https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:227: reg_iter != registrations->end(); ++reg_iter) { does a fancy for (const BackgroundSyncRegistration& reg : it->second) loop work here or maybe std::find(b, e, sync_registion); https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:33: // background sync. In the meantime, --enable-service-worker-sync is required to Any reason to not use switches::kEnableExperimentalWebPlatformFeatures for this? https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:41: class CONTENT_EXPORT BackgroundSyncManager { I wonder if this is a good candidate for using mojo? I see a few services using that now on the UI thread, see RenderProcessHostImpl::RegisterMojoServices(). https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:55: base::string16 id; Maybe use a utf8 std::string here, do the utf8/16 conversions over on the renderer side nearer the webkit API. https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:67: // queue. Will request permission from the user if necessary. Returns Having this method be responsible for making the ui prompt happen seems odd to me? Take a look at how other perms are requested/checked.
Thanks Michael! https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:169: for (const BackgroundSyncRegistration& reg : it->second) { On 2015/02/27 02:56:30, michaeln wrote: > thnx for not using const auto everywhere (or anywhere for that matter) I'm so not a fan of auto.. https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:186: // TODO(jkarlin): This UTF conversion is incompatible with DOMString, we On 2015/02/27 02:56:30, michaeln wrote: > would it be reasonable for the spec to constrain ids to something that can be > represented in utf8 and survive roundtrip conversions? I've brought this up before. I'd much prefer utf8. Will try again. https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:227: reg_iter != registrations->end(); ++reg_iter) { On 2015/02/27 02:56:30, michaeln wrote: > does a fancy for (const BackgroundSyncRegistration& reg : it->second) loop work > here or maybe std::find(b, e, sync_registion); The foreach loop doesn't work because I need an iterator for erase. std::find works but it doesn't look any better: std::list<BackgroundSyncRegistration>::iterator found = std::find_if(registrations->begin(), registrations->end(), [](const BackgroundSyncRegistration& cur_registration) { return cur_registration.id == sync_registration.id; }); if (found == registrations->end()) return false; registrations->erase(found); return true; The simplest is: registrations.remove_if([](const BackgroundSyncRegistration& reg) { return reg.id == sync_registration.id; }); which is nice with a lambda but iterates over the entire list without an early return. I prefer what's in place as it is easy to understand, returns early, and allows for either a list or vector underneath. https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:33: // background sync. In the meantime, --enable-service-worker-sync is required to On 2015/02/27 02:56:30, michaeln wrote: > Any reason to not use switches::kEnableExperimentalWebPlatformFeatures for this? We have that as well. Is kEnableExperimentalWebPlatformFeatures protection enough? It seems like until we at least have a permission it should have its own flag as kEnableExperimentalWebPlatformFeatures may have been enabled for another feature. https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:41: class CONTENT_EXPORT BackgroundSyncManager { On 2015/02/27 02:56:30, michaeln wrote: > I wonder if this is a good candidate for using mojo? I see a few services using > that now on the UI thread, see RenderProcessHostImpl::RegisterMojoServices(). I think Ian (iclelland@) has already written a fair bit of the IPC code using standard IPC. Since we're hoping to get this out sooner rather than later I'd rather start with the tried and true path and convert to mojo as an optimization later. https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:55: base::string16 id; On 2015/02/27 02:56:30, michaeln wrote: > Maybe use a utf8 std::string here, do the utf8/16 conversions over on the > renderer side nearer the webkit API. If the plan is to ultimately support UTF16 without conversion then I want to do it as late as possible since one day we won't do it at all. https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:67: // queue. Will request permission from the user if necessary. Returns On 2015/02/27 02:56:30, michaeln wrote: > Having this method be responsible for making the ui prompt happen seems odd to > me? Take a look at how other perms are requested/checked. Registration is the event at which we prompt. Same for Push. So, I can make a new function to do the actual check, but it will ultimately be invoked from Register().
lgtm https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:227: reg_iter != registrations->end(); ++reg_iter) { On 2015/02/27 16:30:38, jkarlin wrote: > On 2015/02/27 02:56:30, michaeln wrote: > > does a fancy for (const BackgroundSyncRegistration& reg : it->second) loop > work > > here or maybe std::find(b, e, sync_registion); > > The foreach loop doesn't work because I need an iterator for erase. > > std::find works but it doesn't look any better: > std::list<BackgroundSyncRegistration>::iterator found = > std::find_if(registrations->begin(), registrations->end(), > [](const BackgroundSyncRegistration& cur_registration) { > return cur_registration.id == sync_registration.id; > }); > if (found == registrations->end()) > return false; > registrations->erase(found); > return true; > > The simplest is: > registrations.remove_if([](const BackgroundSyncRegistration& reg) { return > reg.id == sync_registration.id; }); > which is nice with a lambda but iterates over the entire list without an early > return. > > I prefer what's in place as it is easy to understand, returns early, and allows > for either a list or vector underneath. awwww, no love for lambdas :) https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:33: // background sync. In the meantime, --enable-service-worker-sync is required to On 2015/02/27 16:30:38, jkarlin wrote: > On 2015/02/27 02:56:30, michaeln wrote: > > Any reason to not use switches::kEnableExperimentalWebPlatformFeatures for > this? > > We have that as well. Is kEnableExperimentalWebPlatformFeatures protection > enough? It seems like until we at least have a permission it should have its own > flag as kEnableExperimentalWebPlatformFeatures may have been enabled for another > feature. I think it's ok to put this behind the big flag. It only kicks in when invoked so it shouldn't interfere with experimenters of other experiments. https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:41: class CONTENT_EXPORT BackgroundSyncManager { On 2015/02/27 16:30:38, jkarlin wrote: > On 2015/02/27 02:56:30, michaeln wrote: > > I wonder if this is a good candidate for using mojo? I see a few services > using > > that now on the UI thread, see RenderProcessHostImpl::RegisterMojoServices(). > > I think Ian (iclelland@) has already written a fair bit of the IPC code using > standard IPC. Since we're hoping to get this out sooner rather than later I'd > rather start with the tried and true path and convert to mojo as an optimization > later. that makes sense https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:67: // queue. Will request permission from the user if necessary. Returns On 2015/02/27 16:30:38, jkarlin wrote: > On 2015/02/27 02:56:30, michaeln wrote: > > Having this method be responsible for making the ui prompt happen seems odd to > > me? Take a look at how other perms are requested/checked. > > Registration is the event at which we prompt. Same for Push. So, I can make a > new function to do the actual check, but it will ultimately be invoked from > Register(). when you get to that part in the impl, you'll see how little context (like which tab or which renderer process) you have in the bowels of this method call. https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... File content/browser/background_sync/background_sync_manager_unittest.cc (right): https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:20: const int64 kRegistrationId1 = 0; nit: might help to call this kServiceWorkerId1 because two different registration types are one too many? https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:34: sync_reg_1_.id.push_back('1'); nit: makes me think id i a non-string like collection, UTF8ToUTF16("1")
jkarlin@chromium.org changed reviewers: + jochen@chromium.org
jochen@chromium.org: Please review for content/ OWNERS. Thanks! https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:186: // TODO(jkarlin): This UTF conversion is incompatible with DOMString, we On 2015/02/27 16:30:38, jkarlin wrote: > On 2015/02/27 02:56:30, michaeln wrote: > > would it be reasonable for the spec to constrain ids to something that can be > > represented in utf8 and survive roundtrip conversions? > > I've brought this up before. I'd much prefer utf8. Will try again. Sticking with DOMString but WebString::utf8() will do the lenient conversion to utf8 that I need. https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:33: // background sync. In the meantime, --enable-service-worker-sync is required to On 2015/02/28 00:34:23, michaeln wrote: > On 2015/02/27 16:30:38, jkarlin wrote: > > On 2015/02/27 02:56:30, michaeln wrote: > > > Any reason to not use switches::kEnableExperimentalWebPlatformFeatures for > > this? > > > > We have that as well. Is kEnableExperimentalWebPlatformFeatures protection > > enough? It seems like until we at least have a permission it should have its > own > > flag as kEnableExperimentalWebPlatformFeatures may have been enabled for > another > > feature. > > I think it's ok to put this behind the big flag. It only kicks in when invoked > so it shouldn't interfere with experimenters of other experiments. Acknowledged. https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:41: class CONTENT_EXPORT BackgroundSyncManager { On 2015/02/28 00:34:23, michaeln wrote: > On 2015/02/27 16:30:38, jkarlin wrote: > > On 2015/02/27 02:56:30, michaeln wrote: > > > I wonder if this is a good candidate for using mojo? I see a few services > > using > > > that now on the UI thread, see > RenderProcessHostImpl::RegisterMojoServices(). > > > > I think Ian (iclelland@) has already written a fair bit of the IPC code using > > standard IPC. Since we're hoping to get this out sooner rather than later I'd > > rather start with the tried and true path and convert to mojo as an > optimization > > later. > > that makes sense Acknowledged. https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:55: base::string16 id; On 2015/02/27 16:30:38, jkarlin wrote: > On 2015/02/27 02:56:30, michaeln wrote: > > Maybe use a utf8 std::string here, do the utf8/16 conversions over on the > > renderer side nearer the webkit API. > > If the plan is to ultimately support UTF16 without conversion then I want to do > it as late as possible since one day we won't do it at all. I'm going with your suggested plan of lenient conversion to utf8 closer to the webkit api. Changed this class to use std::string. https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:67: // queue. Will request permission from the user if necessary. Returns On 2015/02/28 00:34:23, michaeln wrote: > On 2015/02/27 16:30:38, jkarlin wrote: > > On 2015/02/27 02:56:30, michaeln wrote: > > > Having this method be responsible for making the ui prompt happen seems odd > to > > > me? Take a look at how other perms are requested/checked. > > > > Registration is the event at which we prompt. Same for Push. So, I can make a > > new function to do the actual check, but it will ultimately be invoked from > > Register(). > > when you get to that part in the impl, you'll see how little context (like which > tab or which renderer process) you have in the bowels of this method call. True. Some of this code may need to be broken out into a storage class. https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... File content/browser/background_sync/background_sync_manager_unittest.cc (right): https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:20: const int64 kRegistrationId1 = 0; On 2015/02/28 00:34:23, michaeln wrote: > nit: might help to call this kServiceWorkerId1 because two different > registration types are one too many? Done. https://codereview.chromium.org/950343006/diff/120001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:34: sync_reg_1_.id.push_back('1'); On 2015/02/28 00:34:23, michaeln wrote: > nit: makes me think id i a non-string like collection, UTF8ToUTF16("1") Replaced with std::string
jkarlin@chromium.org changed reviewers: - jochen@chromium.org
jkarlin@chromium.org changed reviewers: + davidben@chromium.org
-r jochen@ since he's traveling +r davidben@chromium.org: Please review changes for content/ OWNERS. Thanks!
On 2015/03/02 16:17:18, jkarlin wrote: > -r jochen@ since he's traveling > > +r mailto:davidben@chromium.org: Please review changes for content/ OWNERS. Thanks! Ping davidben! Thanks for looking.
jkarlin@chromium.org changed reviewers: + jochen@chromium.org
On 2015/03/13 12:36:01, jkarlin wrote: > On 2015/03/02 16:17:18, jkarlin wrote: > > -r jochen@ since he's traveling > > > > +r mailto:davidben@chromium.org: Please review changes for content/ OWNERS. > Thanks! > > Ping davidben! Thanks for looking. Jochen or davidben, PTAL for content/ OWNERS. Thanks!
Sorry about the delay. This slipped by while I was at the security summit. https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:145: // TODO(jkarlin): Check for permission. (Seems an important thing to do. :-P) https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:148: callback.Run(ErrorTypeExists, BackgroundSyncRegistration("")); Between this and everyone futher up the stack, it looks like all this makes Register possible to run its callback reentrantly. It might be better to pop them to the message loop one iteration. At a glance, ServiceWorkerStorage has a RunSoon, so there's some precedence in SW code to do this. (Otherwise you end up potentially deeply nesting functions on the stack. //net functions can return synchronously to avoid this. When they can't, they pop to the message loop.) (Ditto for the other callback.Runs, although probably not all make the public APIs reentrant.) https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:185: } Writing N registrations takes O(N^2). Is this a problem for what you're planning on doing? https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:292: reg_iter != registrations->end(); ++reg_iter) { for-each loop? https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:306: base::WeakPtr<BackgroundSyncManager> manager = weak_ptr_factory_.GetWeakPtr(); Worth a comment explaining what's going on here? (That you're worried about the callback destroying the BackgroundSyncManager.) Actually, I could believe that whatever the solution for the reentrancy problem above turns out to be would make some of this moot. https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:11: #include "base/callback.h" I think you only need callback_forward.h in this file. https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:68: // ErrorTypeOK and the registration on success. "Returns" -> "Calls |callback| with" https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:71: const BackgroundSyncRegistration& sync_registratin, sync_registratin -> sync_registration I don't really understand what a consumer of this API is supposed to do with this. On success, they get back a new BackgroundSyncRegistration (which is just a string id). But it wasn't actually new because they passed one in and the implementation seems to echo the same one back. Did you intend for the class to generate them internally? https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:75: // scheduling queue. Returns ErrorTypeNotFound if not registered. Returns Returns -> Calls |callback with https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:84: // Returns ErrorTypeOK on success. Returns -> Calls |callback| with https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:88: const StatusAndRegistrationCallback& callback); I'm assuming the BackgroundSyncRegistration will get other fields later? Right now it just returns the same thing back. https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:157: bool initializing_; These two bits of state are redundant, right? You can probably just have a single initialized_ since initializing_ is always the opposite. Or perhaps just drop both; it's not clear the DCHECKs are actually asserting on anything since they always assert on the pair. (Perhaps add some DCHECKs on just initialized_ for the private functions called by the scheduler? Otherwise I'd remove both.) https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:159: scoped_ptr<IdToRegistrationMap> registration_map_; Why put this in a scoped_ptr? It doesn't look like you ever release it early or detach it from the object or anything. https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:160: scoped_ptr<ServiceWorkerCacheScheduler> op_scheduler_; (Does this need to be a scoped_ptr? I guess other users do it, but it seems this object may as well be created inline too.) https://codereview.chromium.org/950343006/diff/180001/content/browser/service... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/950343006/diff/180001/content/browser/service... content/browser/service_worker/service_worker_storage.cc:776: if (!LazyInitialize( (Having never seen this class before, it sure would be nice if LazyInitialize had a comment describing what it did. :-P)
Thanks and PTAL davidben. https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:145: // TODO(jkarlin): Check for permission. On 2015/03/13 22:05:43, David Benjamin wrote: > (Seems an important thing to do. :-P) Yes, though we're protected by (two) command line flags. https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:148: callback.Run(ErrorTypeExists, BackgroundSyncRegistration("")); On 2015/03/13 22:05:43, David Benjamin wrote: > Between this and everyone futher up the stack, it looks like all this makes > Register possible to run its callback reentrantly. It might be better to pop > them to the message loop one iteration. At a glance, ServiceWorkerStorage has a > RunSoon, so there's some precedence in SW code to do this. (Otherwise you end up > potentially deeply nesting functions on the stack. //net functions can return > synchronously to avoid this. When they can't, they pop to the message loop.) > > (Ditto for the other callback.Runs, although probably not all make the public > APIs reentrant.) RunSoon is a good idea, done. https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:185: } On 2015/03/13 22:05:43, David Benjamin wrote: > Writing N registrations takes O(N^2). Is this a problem for what you're planning > on doing? The thought is that the lists will be small. In which case a list makes more sense than a map or hash table. But one never knows, I'm sure there will be strange pages out there. I'll stick with a list for now but consider a map later. https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:292: reg_iter != registrations->end(); ++reg_iter) { On 2015/03/13 22:05:43, David Benjamin wrote: > for-each loop? Done. https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:306: base::WeakPtr<BackgroundSyncManager> manager = weak_ptr_factory_.GetWeakPtr(); On 2015/03/13 22:05:43, David Benjamin wrote: > Worth a comment explaining what's going on here? (That you're worried about the > callback destroying the BackgroundSyncManager.) > > Actually, I could believe that whatever the solution for the reentrancy problem > above turns out to be would make some of this moot. Added comments. The callback is from outside of this object, and it may still delete this object. So it's a good idea to keep the check. https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:11: #include "base/callback.h" On 2015/03/13 22:05:44, David Benjamin wrote: > I think you only need callback_forward.h in this file. Done. https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:68: // ErrorTypeOK and the registration on success. On 2015/03/13 22:05:44, David Benjamin wrote: > "Returns" -> "Calls |callback| with" Done. https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:71: const BackgroundSyncRegistration& sync_registratin, On 2015/03/13 22:05:44, David Benjamin wrote: > sync_registratin -> sync_registration > > I don't really understand what a consumer of this API is supposed to do with > this. On success, they get back a new BackgroundSyncRegistration (which is just > a string id). But it wasn't actually new because they passed one in and the > implementation seems to echo the same one back. Did you intend for the class to > generate them internally? In the future there is a chance that the Register() function will alter the registration with user-provided guidance. E.g., "Sure, I'll allow this registration but no more than once per day". https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:75: // scheduling queue. Returns ErrorTypeNotFound if not registered. Returns On 2015/03/13 22:05:44, David Benjamin wrote: > Returns -> Calls |callback with Done. https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:84: // Returns ErrorTypeOK on success. On 2015/03/13 22:05:44, David Benjamin wrote: > Returns -> Calls |callback| with Done. https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:88: const StatusAndRegistrationCallback& callback); On 2015/03/13 22:05:43, David Benjamin wrote: > I'm assuming the BackgroundSyncRegistration will get other fields later? Right > now it just returns the same thing back. Yep! https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:157: bool initializing_; On 2015/03/13 22:05:44, David Benjamin wrote: > These two bits of state are redundant, right? You can probably just have a > single initialized_ since initializing_ is always the opposite. Or perhaps just > drop both; it's not clear the DCHECKs are actually asserting on anything since > they always assert on the pair. > > (Perhaps add some DCHECKs on just initialized_ for the private functions called > by the scheduler? Otherwise I'd remove both.) Since init() is now called by ::Create() (it wasn't when I added those booleans) you're right, they're no longer necessary. Good catch, thanks! https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:159: scoped_ptr<IdToRegistrationMap> registration_map_; On 2015/03/13 22:05:44, David Benjamin wrote: > Why put this in a scoped_ptr? It doesn't look like you ever release it early or > detach it from the object or anything. Done. https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:160: scoped_ptr<ServiceWorkerCacheScheduler> op_scheduler_; On 2015/03/13 22:05:44, David Benjamin wrote: > (Does this need to be a scoped_ptr? I guess other users do it, but it seems this > object may as well be created inline too.) Done. https://codereview.chromium.org/950343006/diff/180001/content/browser/service... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/950343006/diff/180001/content/browser/service... content/browser/service_worker/service_worker_storage.cc:776: if (!LazyInitialize( On 2015/03/13 22:05:44, David Benjamin wrote: > (Having never seen this class before, it sure would be nice if LazyInitialize > had a comment describing what it did. :-P) Done.
https://codereview.chromium.org/950343006/diff/220001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/220001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:17: void RunSoon(const tracked_objects::Location& from_here, why not just post the task everywhere instead of this helper functino? https://codereview.chromium.org/950343006/diff/220001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/950343006/diff/220001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:43: ErrorTypeOK = 0, enums in chromium should be MACRO_STYLE (OK, EXISTS, STORAGE, NOT_FOUND)
jochen@chromium.org changed reviewers: - jochen@chromium.org
deferring to David
PTAL David, thanks! https://codereview.chromium.org/950343006/diff/220001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/220001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:17: void RunSoon(const tracked_objects::Location& from_here, On 2015/03/16 12:04:21, jochen (slow) wrote: > why not just post the task everywhere instead of this helper functino? Done. https://codereview.chromium.org/950343006/diff/220001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/950343006/diff/220001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:43: ErrorTypeOK = 0, On 2015/03/16 12:04:21, jochen (slow) wrote: > enums in chromium should be MACRO_STYLE (OK, EXISTS, STORAGE, NOT_FOUND) Done.
Patchset #14 (id:260001) has been deleted
On 2015/03/16 13:12:19, jkarlin wrote: > PTAL David, thanks! > > https://codereview.chromium.org/950343006/diff/220001/content/browser/backgro... > File content/browser/background_sync/background_sync_manager.cc (right): > > https://codereview.chromium.org/950343006/diff/220001/content/browser/backgro... > content/browser/background_sync/background_sync_manager.cc:17: void > RunSoon(const tracked_objects::Location& from_here, > On 2015/03/16 12:04:21, jochen (slow) wrote: > > why not just post the task everywhere instead of this helper functino? > > Done. > > https://codereview.chromium.org/950343006/diff/220001/content/browser/backgro... > File content/browser/background_sync/background_sync_manager.h (right): > > https://codereview.chromium.org/950343006/diff/220001/content/browser/backgro... > content/browser/background_sync/background_sync_manager.h:43: ErrorTypeOK = 0, > On 2015/03/16 12:04:21, jochen (slow) wrote: > > enums in chromium should be MACRO_STYLE (OK, EXISTS, STORAGE, NOT_FOUND) > > Done. Ping davidben@, thanks!
On 2015/03/16 21:07:56, jkarlin wrote: > Ping davidben@, thanks! Might not be able to get to this until tomorrow. Today has been pretty swamped with meetings and then I have to writeup something I'm long overdue on. :-(
A few more comments on what seem to be problems with unregister (sorry I didn't notice those the first iteration) and comments on the test. NOTE: One of the comments below is on a previous patch set (since those often get lost). https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:71: const BackgroundSyncRegistration& sync_registratin, On 2015/03/14 01:17:27, jkarlin wrote: > On 2015/03/13 22:05:44, David Benjamin wrote: > > sync_registratin -> sync_registration > > > > I don't really understand what a consumer of this API is supposed to do with > > this. On success, they get back a new BackgroundSyncRegistration (which is > just > > a string id). But it wasn't actually new because they passed one in and the > > implementation seems to echo the same one back. Did you intend for the class > to > > generate them internally? > > In the future there is a chance that the Register() function will alter the > registration with user-provided guidance. E.g., "Sure, I'll allow this > registration but no more than once per day". Ah, okay. Perhaps add a comment to this effect or leave it as just a StatusCallback for now? It's somewhat confusing reading the header without that context. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:208: std::list<BackgroundSyncRegistration>* registrations = &(it->second); Nit: No need for parens here. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:256: AddRegistrationToMap(sw_registration_id, sync_registration); There's no guarantee this is the same as the original registration. Perhaps RemoveRegistrationFromMap should return the thing that was removed and you use that. And have it only take the ID as input. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:68: // |callback| with ErrorTypeOK and the registration on success. There's no way to change an existing registration, right? I vaguely worry that people will just defensibly unregister first when using the API. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:79: const BackgroundSyncRegistration& sync_registration, Should this pass in a sync_registration_id rather than the full registration? It's weird that you *almost* ignore the (currently non-existent) remaining parameters. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:157: ServiceWorkerCacheScheduler op_scheduler_; It's a little odd that operations from one sw_registration_id lock out those from another. But probably fine? https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... File content/browser/background_sync/background_sync_manager_unittest.cc (right): https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:18: const char* kOriginUrl = "https://example.com"; Nit: const char* foo -> const char foo[] https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:35: sync_reg_2_.id = "2"; You can just initialize these in the initializer list, right? https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:50: BackgroundSyncManager::Create(service_worker_context_); It'd be nice for this to test that operations just get scheduled if you're waiting on Init to complete. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:74: base::RunLoop().RunUntilIdle(); Likewise, having the RunUntilIdles here means you never test what happens if many Register/Unregister/etc calls happen in parallel. (You could maybe have some tests that call into the background_sync_manager_ directly and pass in their own callbacks?) Schedule a bunch, wait for the last one to fire, and assert that each callback as called as expected. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:196: EXPECT_TRUE(GetRegistration(sync_reg_1_.id)); (It's fine now, but I assume later you'll want these to assert on the properties of the registration it returns.) https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:202: EXPECT_TRUE(Unregister(sync_reg_1_)); I'd add an EXPECT_FALSE(GetRegistration) to assert that that it did something.
Patchset #15 (id:300001) has been deleted
Thanks David, great comments. I wound up doing a bunch of things that I had intended for followup CLs but made sense to do now given your comments. This is a large change, my apologies. 1. Each registration now has a unique id. The previous string field "id" is renamed to "name" and "id" is now a unique int64 identifier. The id is required as a reference for javascript Registration objects. When such objects call "unregister()" the id will need to be passed. 2. Register() can now overwrite existing entries. 3. Registrations per ServiceWorker are now stored in a map instead of a list. 4. New testing framework allows for testing a failing backend and sequential scheduling. Lots more tests! https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/950343006/diff/180001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:71: const BackgroundSyncRegistration& sync_registratin, On 2015/03/17 18:47:14, David Benjamin wrote: > On 2015/03/14 01:17:27, jkarlin wrote: > > On 2015/03/13 22:05:44, David Benjamin wrote: > > > sync_registratin -> sync_registration > > > > > > I don't really understand what a consumer of this API is supposed to do with > > > this. On success, they get back a new BackgroundSyncRegistration (which is > > just > > > a string id). But it wasn't actually new because they passed one in and the > > > implementation seems to echo the same one back. Did you intend for the class > > to > > > generate them internally? > > > > In the future there is a chance that the Register() function will alter the > > registration with user-provided guidance. E.g., "Sure, I'll allow this > > registration but no more than once per day". > > Ah, okay. Perhaps add a comment to this effect or leave it as just a > StatusCallback for now? It's somewhat confusing reading the header without that > context. Done. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:208: std::list<BackgroundSyncRegistration>* registrations = &(it->second); On 2015/03/17 18:47:15, David Benjamin wrote: > Nit: No need for parens here. Done. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:256: AddRegistrationToMap(sw_registration_id, sync_registration); On 2015/03/17 18:47:15, David Benjamin wrote: > There's no guarantee this is the same as the original registration. > > Perhaps RemoveRegistrationFromMap should return the thing that was removed and > you use that. And have it only take the ID as input. Thanks! Done. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:68: // |callback| with ErrorTypeOK and the registration on success. On 2015/03/17 18:47:15, David Benjamin wrote: > There's no way to change an existing registration, right? I vaguely worry that > people will just defensibly unregister first when using the API. The plan was to change this to overwrite existing registrations in a future CL. But I did it now instead. Done. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:79: const BackgroundSyncRegistration& sync_registration, On 2015/03/17 18:47:15, David Benjamin wrote: > Should this pass in a sync_registration_id rather than the full registration? > It's weird that you *almost* ignore the (currently non-existent) remaining > parameters. Done. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:157: ServiceWorkerCacheScheduler op_scheduler_; On 2015/03/17 18:47:15, David Benjamin wrote: > It's a little odd that operations from one sw_registration_id lock out those > from another. But probably fine? I intend to add such parallelism in the future. See the TODO in ServiceWorkerCacheScheduler. I think it's fine for now but should be addressed eventually. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... File content/browser/background_sync/background_sync_manager_unittest.cc (right): https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:18: const char* kOriginUrl = "https://example.com"; On 2015/03/17 18:47:15, David Benjamin wrote: > Nit: const char* foo -> const char foo[] Done. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:35: sync_reg_2_.id = "2"; On 2015/03/17 18:47:15, David Benjamin wrote: > You can just initialize these in the initializer list, right? Done. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:50: BackgroundSyncManager::Create(service_worker_context_); On 2015/03/17 18:47:15, David Benjamin wrote: > It'd be nice for this to test that operations just get scheduled if you're > waiting on Init to complete. Done. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:74: base::RunLoop().RunUntilIdle(); On 2015/03/17 18:47:15, David Benjamin wrote: > Likewise, having the RunUntilIdles here means you never test what happens if > many Register/Unregister/etc calls happen in parallel. (You could maybe have > some tests that call into the background_sync_manager_ directly and pass in > their own callbacks?) Schedule a bunch, wait for the last one to fire, and > assert that each callback as called as expected. Done. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:196: EXPECT_TRUE(GetRegistration(sync_reg_1_.id)); On 2015/03/17 18:47:15, David Benjamin wrote: > (It's fine now, but I assume later you'll want these to assert on the properties > of the registration it returns.) Done. https://codereview.chromium.org/950343006/diff/280001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:202: EXPECT_TRUE(Unregister(sync_reg_1_)); On 2015/03/17 18:47:15, David Benjamin wrote: > I'd add an EXPECT_FALSE(GetRegistration) to assert that that it did something. Done.
PTAL David, thanks!
On 2015/03/23 20:09:59, jkarlin wrote: > PTAL David, thanks! A friendly ping. Sorry that this is such a large CL.
Thanks! One more round-trip and that should be it. (Sorry my latency has been so high. :-/) https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... File content/browser/background_sync/background_sync.proto (right): https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync.proto:14: optional int64 min_period = 3; Nit: Since you're storing it as 0 in-memory and having all these has_min_period checks, maybe just make it required and call 0 the minimum period? https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync.proto:19: required int64 registration_id_counter = 2; Nit: next_registration_id might be clearer. (I feel I've seen more next_foo_id type names. Also makes it clear whether that was the last ID or the next one.) https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:50: DCHECK(sync_registration.id == Nit: DCHECK -> DCHECK_EQ https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:135: registration_proto.name()); Optional: since this data comes from disk, is it worth checking consistency between registration_proto.id() and registration_id_counter() here? Not sure what you'd do in that case though... drop it on the floor? I guess there's also other invariants like "no repeated SW ids" and "no repeated registration names" and such. How much it's worth bothering with, I dunno. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:157: if (existing_registration.Equals(sync_registration)) { This check gives an interesting interaction with Unregister. Say you call Unregister(old_registration) and race it with Register(new_registration) under the same name. Normally, because Unregister checks an id, you know it will win. But because of this Equals check, if the old and the new happen to have the same contents, it depends on which hits the op_scheduler_ first. (One hopes that no one would do that, and if a SW does weird things like this to its background registration state with two independent pieces of code fighting over a name, it's going to be confused regardless. But it is a little odd of behavior for this component.) https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:189: const BackgroundSyncRegistrations::NameToRegistrationMap::const_iterator Nit/optional: This type name is probably a prime candidate to use 'auto' with. :-) https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:232: const BackgroundSyncRegistration& sync_registration, Nit: new_registration, to match the rename in the caller? https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:259: BackgroundSyncRegistration* old_registration) { This never writes the old entry to old_registration. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:270: DCHECK(sw_registration_id != Nit: DCHECK_NE https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:347: const StatusAndRegistrationCallback& callback) { I think this function can just call HasRegistration now. EDIT: Possibly disregard this if you end up removing the RegisterImpl special-case and this no longer needs an output parameter. Though maybe it'd be cleaner anyway? I dunno. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:359: const BackgroundSyncRegistrations::NameToRegistrationMap::const_iterator Optional/nit: ditto about maybe using auto here. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:105: // registered. Calls |callback| with ErrorTypeOK on success. Could you document the relation between |sync_registration_name| and |sync_registration_id|? https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:146: bool HasRegistration(int64 sw_registration_id, Nit: LookupRegistration might be a better name for this now that it has an output. EDIT: Possibly disregard this if you end up removing the RegisterImpl special-case and this no longer needs an output parameter. Though maybe it'd be cleaner anyway? I dunno. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... File content/browser/background_sync/background_sync_manager_unittest.cc (right): https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:73: callback.Run(SERVICE_WORKER_ERROR_FAILED); Nit: The real StoreDataInBackend never calls the callback reentrantly. Probably worth a PostTask here. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:92: callback.Run(std::vector<std::pair<int64, std::string>>(), Nit: Ditto about it being reentrant. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:275: EXPECT_NE(sync_reg_1_.id, callback_registration_.id); Nit: I'd replace sync_reg_1_.id with BackgroundSyncRegistration::kInvalidRegistrationId here. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:355: callback_registration_.id = 10012310; Nit: That's a very weird number. :-) Maybe just callback_registration_.id++? https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:384: EXPECT_TRUE(GetRegistration(sync_reg_1_.name)); Probably should add some assertions that callback_registration_ is exactly what you expected. (Would have caught the bug in RemoveRegistrationFromMap.) https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:481: EXPECT_FALSE(register_called || unregister_called || get_registration_called); Nit: I'd expand these out individually (and ditto below). This way if one triggers we know which it was.
Thanks for the bug catch! PTAL. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... File content/browser/background_sync/background_sync.proto (right): https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync.proto:14: optional int64 min_period = 3; On 2015/03/25 16:01:09, David Benjamin wrote: > Nit: Since you're storing it as 0 in-memory and having all these has_min_period > checks, maybe just make it required and call 0 the minimum period? Acknowledged. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync.proto:19: required int64 registration_id_counter = 2; On 2015/03/25 16:01:09, David Benjamin wrote: > Nit: next_registration_id might be clearer. (I feel I've seen more next_foo_id > type names. Also makes it clear whether that was the last ID or the next one.) Done. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:50: DCHECK(sync_registration.id == On 2015/03/25 16:01:09, David Benjamin wrote: > Nit: DCHECK -> DCHECK_EQ Done. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:135: registration_proto.name()); On 2015/03/25 16:01:09, David Benjamin wrote: > Optional: since this data comes from disk, is it worth checking consistency > between registration_proto.id() and registration_id_counter() here? Not sure > what you'd do in that case though... drop it on the floor? > > I guess there's also other invariants like "no repeated SW ids" and "no repeated > registration names" and such. How much it's worth bothering with, I dunno. Done. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:157: if (existing_registration.Equals(sync_registration)) { On 2015/03/25 16:01:10, David Benjamin wrote: > This check gives an interesting interaction with Unregister. Say you call > Unregister(old_registration) and race it with Register(new_registration) under > the same name. > > Normally, because Unregister checks an id, you know it will win. > > But because of this Equals check, if the old and the new happen to have the same > contents, it depends on which hits the op_scheduler_ first. > > (One hopes that no one would do that, and if a SW does weird things like this to > its background registration state with two independent pieces of code fighting > over a name, it's going to be confused regardless. But it is a little odd of > behavior for this component.) Interesting. The race potential is there, sync functions can be called from the document or the SW. However, that's getting into pretty wonky territory and I'm inclined to leave this as is as it saves writing back to storage each time the page is loaded (I assume people will register for sync each time they load the page). https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:189: const BackgroundSyncRegistrations::NameToRegistrationMap::const_iterator On 2015/03/25 16:01:09, David Benjamin wrote: > Nit/optional: This type name is probably a prime candidate to use 'auto' with. > :-) Done. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:232: const BackgroundSyncRegistration& sync_registration, On 2015/03/25 16:01:09, David Benjamin wrote: > Nit: new_registration, to match the rename in the caller? Done. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:259: BackgroundSyncRegistration* old_registration) { On 2015/03/25 16:01:09, David Benjamin wrote: > This never writes the old entry to old_registration. !!!! Done. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:270: DCHECK(sw_registration_id != On 2015/03/25 16:01:09, David Benjamin wrote: > Nit: DCHECK_NE Done. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:347: const StatusAndRegistrationCallback& callback) { On 2015/03/25 16:01:09, David Benjamin wrote: > I think this function can just call HasRegistration now. > > EDIT: Possibly disregard this if you end up removing the RegisterImpl > special-case and this no longer needs an output parameter. Though maybe it'd be > cleaner anyway? I dunno. Done. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:359: const BackgroundSyncRegistrations::NameToRegistrationMap::const_iterator On 2015/03/25 16:01:10, David Benjamin wrote: > Optional/nit: ditto about maybe using auto here. no longer needed https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:105: // registered. Calls |callback| with ErrorTypeOK on success. On 2015/03/25 16:01:10, David Benjamin wrote: > Could you document the relation between |sync_registration_name| and > |sync_registration_id|? Done. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:146: bool HasRegistration(int64 sw_registration_id, On 2015/03/25 16:01:10, David Benjamin wrote: > Nit: LookupRegistration might be a better name for this now that it has an > output. > > EDIT: Possibly disregard this if you end up removing the RegisterImpl > special-case and this no longer needs an output parameter. Though maybe it'd be > cleaner anyway? I dunno. Done. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... File content/browser/background_sync/background_sync_manager_unittest.cc (right): https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:73: callback.Run(SERVICE_WORKER_ERROR_FAILED); On 2015/03/25 16:01:10, David Benjamin wrote: > Nit: The real StoreDataInBackend never calls the callback reentrantly. Probably > worth a PostTask here. Done. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:92: callback.Run(std::vector<std::pair<int64, std::string>>(), On 2015/03/25 16:01:10, David Benjamin wrote: > Nit: Ditto about it being reentrant. Done. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:275: EXPECT_NE(sync_reg_1_.id, callback_registration_.id); On 2015/03/25 16:01:10, David Benjamin wrote: > Nit: I'd replace sync_reg_1_.id with > BackgroundSyncRegistration::kInvalidRegistrationId here. Done. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:355: callback_registration_.id = 10012310; On 2015/03/25 16:01:10, David Benjamin wrote: > Nit: That's a very weird number. :-) Maybe just callback_registration_.id++? Done. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:384: EXPECT_TRUE(GetRegistration(sync_reg_1_.name)); On 2015/03/25 16:01:10, David Benjamin wrote: > Probably should add some assertions that callback_registration_ is exactly what > you expected. (Would have caught the bug in RemoveRegistrationFromMap.) Yep, done. Verified that updated test fails without the fix. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager_unittest.cc:481: EXPECT_FALSE(register_called || unregister_called || get_registration_called); On 2015/03/25 16:01:10, David Benjamin wrote: > Nit: I'd expand these out individually (and ditto below). This way if one > triggers we know which it was. Done.
lgtm, modulo try bots not being green (looks like you need to rebase), and some questions about InitDidGetDataFromBackend. https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/320001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:157: if (existing_registration.Equals(sync_registration)) { On 2015/03/25 19:24:18, jkarlin wrote: > On 2015/03/25 16:01:10, David Benjamin wrote: > > This check gives an interesting interaction with Unregister. Say you call > > Unregister(old_registration) and race it with Register(new_registration) under > > the same name. > > > > Normally, because Unregister checks an id, you know it will win. > > > > But because of this Equals check, if the old and the new happen to have the > same > > contents, it depends on which hits the op_scheduler_ first. > > > > (One hopes that no one would do that, and if a SW does weird things like this > to > > its background registration state with two independent pieces of code fighting > > over a name, it's going to be confused regardless. But it is a little odd of > > behavior for this component.) > > Interesting. The race potential is there, sync functions can be called from the > document or the SW. However, that's getting into pretty wonky territory and I'm > inclined to leave this as is as it saves writing back to storage each time the > page is loaded (I assume people will register for sync each time they load the > page). Sounds good. https://codereview.chromium.org/950343006/diff/340001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/340001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:128: ServiceWorkerStatusCode status) { Should this check status? Looks like this'll just silently drop everything if it can't read things in. Probably should at least LOG(ERROR)? I suppose the corruption_detected line below also just silently drops everything. Dunno what we usually do in that case, disable the subsystem or silently try to recover with some data loss. https://codereview.chromium.org/950343006/diff/340001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:161: if (corruption_detected) Nit: Maybe spew to LOG(ERROR) or something? Though I'm not really sure what we do about on-disk stuff in general.
https://codereview.chromium.org/950343006/diff/340001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/950343006/diff/340001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:128: ServiceWorkerStatusCode status) { On 2015/03/25 19:42:03, David Benjamin wrote: > Should this check status? Looks like this'll just silently drop everything if it > can't read things in. Probably should at least LOG(ERROR)? I suppose the > corruption_detected line below also just silently drops everything. > > Dunno what we usually do in that case, disable the subsystem or silently try to > recover with some data loss. Added a LOG error and a TODO to the header file that I need to look into error handling from the backend. https://codereview.chromium.org/950343006/diff/340001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:161: if (corruption_detected) On 2015/03/25 19:42:03, David Benjamin wrote: > Nit: Maybe spew to LOG(ERROR) or something? Though I'm not really sure what we > do about on-disk stuff in general. Done.
The CQ bit was checked by jkarlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/950343006/#ps390001 (title: "CONTENT_EXPORT for nested classes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950343006/390001
Message was sent while issue was closed.
Committed patchset #19 (id:390001)
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/34ee23cfb4147f51b8ca0f728e6354f8f86db8ee Cr-Commit-Position: refs/heads/master@{#322375} |