Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
62203007:
Implement memory-persistent registration (Closed)
|
Created:
7 years, 1 month ago by alecflett Modified:
6 years, 5 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement memory-persistent registration
This simply provides an in-memory registration in ServiceWorkerContext. It provides basic registration/unregistration,
adding stub ServiceWorkerRegistry and ServiceWorkerVersion classes.
BUG=285976
TBR=jam@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239380
Patch Set 1 #Patch Set 2 : #
Total comments: 22
Patch Set 3 : Address kinuko's comments, add more tests #Patch Set 4 : Remove some stray #includes #
Total comments: 1
Patch Set 5 : Address review comments before splitting #Patch Set 6 : Rip out registration/version #Patch Set 7 : Now with storage and jobs! #Patch Set 8 : Working version #
Total comments: 24
Patch Set 9 : Also pass status around #Patch Set 10 : Clean up enums, comments, etc #Patch Set 11 : More comments #
Total comments: 42
Patch Set 12 : Address kinuko's comments #
Total comments: 29
Patch Set 13 : Address most comments #
Total comments: 6
Patch Set 14 : spacing and whatnot #Patch Set 15 : Fix const_iterator bustage #Patch Set 16 : Fix variable that we don't need anymore #Messages
Total messages: 35 (0 generated)
michaeln@ / kinuko@ - PTAL? I've explicitly made the register/unregister asynchronous, requiring the event loop to roll, and enforced it via tests. I had told michael about this earlier: the reason I did it this way is that I want to make sure we don't have some codepaths with some synchronous callbacks and some with asynchronous callbacks - this has been a notorious source of bugs on other projects.
High-level comment: how much are we going to make this version tentative/prototype-ish or not? The current in-memory map code looks to be somewhat work but we'll need to revise a lot later again. Alternatively we can take intermediate approaches like: - define a storage class interface - define a URL pattern matcher interface - write them in a lightweight way first, e.g. using in-memory hash and simple ad-hoc matching, and write unittests for them - StorageWorkerContext basically connects those interfaces so that switching from in-memory hash to leveldb doesn't cause significant change https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... File content/browser/service_worker/service_worker_context.cc (right): https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_context.cc:20: static int64 NextRegistrationId() { nit: no need of static in anonymous namespace https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_context.cc:45: Please match the method order same as in header. https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_context.cc:66: if (previous_registration) { We first unregister in the RegisterServiceWorker code path below so we shouldn't come this path? https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_context.cc:86: ServiceWorkerRegistration* registration = Register(pattern, script_url); Do we always unregister and register a new one? https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... File content/browser/service_worker/service_worker_context.h (right): https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_context.h:47: typedef base::Callback<void(int64)> ResponseCallback; nit: please move this typedef before all other methods/ctor, right after public: https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_context.h:71: void Unregister(const GURL& pattern); We do seem to have lots of register/registration methods, can some of them be private? I want to make it clear that what public methods each class will expose. https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_context.h:82: typedef std::map<GURL, scoped_refptr<ServiceWorkerRegistration> > I think this has been discussed once, but do we continue to use GURL for URL pattern? I'm feeling that's weird. https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... File content/browser/service_worker/service_worker_dispatcher_host.h (right): https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.h:43: int32 worker_id); Not implemented? https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... File content/browser/service_worker/service_worker_registration.h (right): https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_registration.h:28: int32 id() const { return registration_id_; } We're using int64 in other files
On 2013/11/08 09:08:20, kinuko wrote: > High-level comment: how much are we going to make this version > tentative/prototype-ish or not? I'm taking the approach of doing the minimal amount of work to get us to our first milestone - where registration is persistent in RAM. I personally don't mind revising this again later.. I think all the approaches you bring up are good, but I'd like to address them in future (smaller) CL's. I've thought a bit about some of these individual issues: > The current in-memory map code looks to be > somewhat work but we'll need to revise a lot later again. Alternatively we can > take intermediate approaches like: > > - define a storage class interface Agreed - this is why the TODO mentions the RegistrationForDocument() being asynchronous.. > - define a URL pattern matcher interface So I'd like to look into borrowing the extensions pattern matcher here but it's currently in chrome/ not content. It's well tested, modular, the patterns are well defined, and are to serve a similar purpose: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... As a part of that we'd need to formalize this pattern matching in the spec, and get buy-in from mozilla. In any case, that's why I'm doing a very hacky approach right now - I know that no matter what I write will be wrong, so I might as well write the minimum for now. > - write them in a lightweight way first, e.g. using in-memory hash and simple > ad-hoc matching, and write unittests for them Well I have really basic unit tests for the matches that are implemented here (see service_worker_context_unittest.cc) - so it's not modular (yet) but we can refactor them when we need to. > - StorageWorkerContext basically connects those interfaces so that switching > from in-memory hash to leveldb doesn't cause significant change
https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... File content/browser/service_worker/service_worker_context.cc (right): https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_context.cc:20: static int64 NextRegistrationId() { On 2013/11/08 09:08:21, kinuko wrote: > nit: no need of static in anonymous namespace Done. https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_context.cc:45: On 2013/11/08 09:08:21, kinuko wrote: > Please match the method order same as in header. Done. https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_context.cc:66: if (previous_registration) { On 2013/11/08 09:08:21, kinuko wrote: > We first unregister in the RegisterServiceWorker code path below so we shouldn't > come this path? Done. (oops, leftover from early versions of this) https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_context.cc:86: ServiceWorkerRegistration* registration = Register(pattern, script_url); On 2013/11/08 09:08:21, kinuko wrote: > Do we always unregister and register a new one? Yes. unregister is a no-op if there was no previous script. This ensures that if there *was* a previous registration, that it is shutdown. And actually, if the script_url is unchanged, this should be a no-op. I'll add a test. https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... File content/browser/service_worker/service_worker_context.h (right): https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_context.h:47: typedef base::Callback<void(int64)> ResponseCallback; On 2013/11/08 09:08:21, kinuko wrote: > nit: please move this typedef before all other methods/ctor, right after public: Done. https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_context.h:71: void Unregister(const GURL& pattern); On 2013/11/08 09:08:21, kinuko wrote: > We do seem to have lots of register/registration methods, can some of them be > private? I want to make it clear that what public methods each class will > expose. Done. They were there for testing, I found a better way (FRIEND_TEST*()) https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_context.h:82: typedef std::map<GURL, scoped_refptr<ServiceWorkerRegistration> > On 2013/11/08 09:08:21, kinuko wrote: > I think this has been discussed once, but do we continue to use GURL for URL > pattern? I'm feeling that's weird. I'm fine changing it. Can I do that in a separate CL? https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... File content/browser/service_worker/service_worker_dispatcher_host.h (right): https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_dispatcher_host.h:43: int32 worker_id); On 2013/11/08 09:08:21, kinuko wrote: > Not implemented? oops. Done. https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... File content/browser/service_worker/service_worker_registration.h (right): https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_registration.h:28: int32 id() const { return registration_id_; } On 2013/11/08 09:08:21, kinuko wrote: > We're using int64 in other files Done.
On 2013/11/08 22:50:54, alecflett wrote: > https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... > File content/browser/service_worker/service_worker_context.cc (right): > > https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... > content/browser/service_worker/service_worker_context.cc:20: static int64 > NextRegistrationId() { > On 2013/11/08 09:08:21, kinuko wrote: > > nit: no need of static in anonymous namespace > > Done. > > https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... > content/browser/service_worker/service_worker_context.cc:45: > On 2013/11/08 09:08:21, kinuko wrote: > > Please match the method order same as in header. > > Done. > > https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... > content/browser/service_worker/service_worker_context.cc:66: if > (previous_registration) { > On 2013/11/08 09:08:21, kinuko wrote: > > We first unregister in the RegisterServiceWorker code path below so we > shouldn't > > come this path? > > Done. (oops, leftover from early versions of this) > > https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... > content/browser/service_worker/service_worker_context.cc:86: > ServiceWorkerRegistration* registration = Register(pattern, script_url); > On 2013/11/08 09:08:21, kinuko wrote: > > Do we always unregister and register a new one? > > Yes. unregister is a no-op if there was no previous script. > > This ensures that if there *was* a previous registration, that it is shutdown. > And actually, if the script_url is unchanged, this should be a no-op. > > I'll add a test. > > https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... > File content/browser/service_worker/service_worker_context.h (right): > > https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... > content/browser/service_worker/service_worker_context.h:47: typedef > base::Callback<void(int64)> ResponseCallback; > On 2013/11/08 09:08:21, kinuko wrote: > > nit: please move this typedef before all other methods/ctor, right after > public: > > Done. > > https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... > content/browser/service_worker/service_worker_context.h:71: void > Unregister(const GURL& pattern); > On 2013/11/08 09:08:21, kinuko wrote: > > We do seem to have lots of register/registration methods, can some of them be > > private? I want to make it clear that what public methods each class will > > expose. > > Done. > They were there for testing, I found a better way (FRIEND_TEST*()) > > https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... > content/browser/service_worker/service_worker_context.h:82: typedef > std::map<GURL, scoped_refptr<ServiceWorkerRegistration> > > On 2013/11/08 09:08:21, kinuko wrote: > > I think this has been discussed once, but do we continue to use GURL for URL > > pattern? I'm feeling that's weird. > > I'm fine changing it. Can I do that in a separate CL? > I realized why I found this useful - we can do same-origin checks if this is a url. (We can't just do a pattern check because you can register scripts outside of the pattern)
> High-level comment: how much are we going to make this version > tentative/prototype-ish or not? The current in-memory map code looks to be > somewhat work but we'll need to revise a lot later again. Alternatively we can > take intermediate approaches like: > > - define a storage class interface > - define a URL pattern matcher interface > - write them in a lightweight way first, e.g. using in-memory hash and simple > ad-hoc matching, and write unittests for them > - StorageWorkerContext basically connects those interfaces so that switching > from in-memory hash to leveldb doesn't cause significant change I think I have the same concern as kinuko. I'd like to spend more cycles early on to get our interfaces closer to what we think is correct, so then we can work more in parallel on different sides of those interfaces. Maybe we carve this CL up into smaller parts? Defining SWRegistration and SWVersion classes and coding up ownership relationships that work would be great to dig into. Prescribe who owns what and code these classes up accordingly. The appcache has a take at that, AppCacheGroup and AppCache have a similar sort of relationship as these two SW classes, we could recycle that pattern or come up with something else that works. Defining interfaces for the Register function would be great to dig into. Seems like Register() is a long'ish running task, that multiple documents can be party to and receive completion notification of. Also seems like if all the party goers leave the party before completion... the Register() function should probably abort() instead of continuing. A stab at classes that model/support that would be good. https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... File content/browser/service_worker/service_worker_registration.h (right): https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_registration.h:22: : public base::RefCountedThreadSafe<ServiceWorkerRegistration> { Is there a reason to make this a thread-safe? I think we should strive to have this class be a IO thread only sort of class, so no need for threadsafe refcounting. https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_registration.h:24: ServiceWorkerRegistration(const GURL& pattern, other places we use the term 'scope', here it's 'pattern', either is fine... i have a preference for 'pattern'. https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_registration.h:26: int32 service_worker_id); registration_id this arg s/b called registration_id https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... content/browser/service_worker/service_worker_version.h:26: scoped_refptr<ServiceWorkerRegistration> registration_; Hmmm... does this cause a reference cycle? Reg owns a Ver which refs the owning Reg. https://codereview.chromium.org/62203007/diff/170001/content/browser/service_... File content/browser/service_worker/service_worker_context.cc (right): https://codereview.chromium.org/62203007/diff/170001/content/browser/service_... content/browser/service_worker/service_worker_context.cc:118: bool ServiceWorkerContext::PatternMatches(const GURL& pattern, Maybe we can get some use out of the MatchPattern function in base for this, it does multiple glob matching... here's how appcache::Namespace::IsMatch uses that function... // We have to escape '?' characters since MatchPattern also treats those // as wildcards which we don't want here, we only do '*'s. std::string pattern = namespace_url.spec(); if (namespace_url.has_query()) ReplaceSubstringsAfterOffset(&pattern, 0, "?", "\\?"); return MatchPattern(url.spec(), pattern);
Updated patch and new CL upcoming. All the rest of the review comments will be addressed: On 2013/11/11 23:48:24, michaeln wrote: > I think I have the same concern as kinuko. I'd like to spend more cycles early > on to get our interfaces closer to what we think is correct, so then we can work > more in parallel on different sides of those interfaces. > > Maybe we carve this CL up into smaller parts? > As discussed, I'm going to break out ServiceWorkerRegistration and ServiceWorkerVersion stubs into a separate CL. Both will be non-threadsafe refcounted and have explicit shutdown methods (specific naming to be discussed/decided in that patch) > Defining SWRegistration and SWVersion classes and coding up ownership > relationships that work would be great to dig into. Prescribe who owns what and > code these classes up accordingly. The appcache has a take at that, > AppCacheGroup and AppCache have a similar sort of relationship as these two SW > classes, we could recycle that pattern or come up with something else that > works. > > Defining interfaces for the Register function would be great to dig into. Seems > like Register() is a long'ish running task, that multiple documents can be party > to and receive completion notification of. Also seems like if all the party > goers leave the party before completion... the Register() function should > probably abort() instead of continuing. A stab at classes that model/support > that would be good. > > https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... > File content/browser/service_worker/service_worker_registration.h (right): > > https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... > content/browser/service_worker/service_worker_registration.h:22: : public > base::RefCountedThreadSafe<ServiceWorkerRegistration> { > Is there a reason to make this a thread-safe? I think we should strive to have > this class be a IO thread only sort of class, so no need for threadsafe > refcounting. nope, overzealous cut 'n paste. Done. > > https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... > content/browser/service_worker/service_worker_registration.h:24: > ServiceWorkerRegistration(const GURL& pattern, > other places we use the term 'scope', here it's 'pattern', either is fine... i > have a preference for 'pattern'. > Done. (pattern) > https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... > content/browser/service_worker/service_worker_registration.h:26: int32 > service_worker_id); > registration_id this arg s/b called registration_id Done. > > https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... > File content/browser/service_worker/service_worker_version.h (right): > > https://codereview.chromium.org/62203007/diff/20001/content/browser/service_w... > content/browser/service_worker/service_worker_version.h:26: > scoped_refptr<ServiceWorkerRegistration> registration_; > Hmmm... does this cause a reference cycle? Reg owns a Ver which refs the owning > Reg. Explicit Shutdown() avoids the leak. Expectation is that ServiceWorkerContext shuts down all open registrations. > > https://codereview.chromium.org/62203007/diff/170001/content/browser/service_... > File content/browser/service_worker/service_worker_context.cc (right): > > https://codereview.chromium.org/62203007/diff/170001/content/browser/service_... > content/browser/service_worker/service_worker_context.cc:118: bool > ServiceWorkerContext::PatternMatches(const GURL& pattern, > Maybe we can get some use out of the MatchPattern function in base for this, it > does multiple glob matching... here's how appcache::Namespace::IsMatch uses that > function... oops, didn't even know that existed. Done.
Ok, this patch has now been updated to reflect moving the registration/version stuff into a separate smaller patch.. it isn't quite ready yet, as I'm going to try breaking out the "storage" interface.
Ok, this is *almost* ready for a full review but michaeln/kinuko feel free to take a look, but I'm not expecting an LGTM quite yet. I hoped to be done with this already but I was thwarted by memory management issues. (Yay ASAN!) the overall architecture is there: 1) The ServiceWorkerStorage class "owns" registration/unregistration 2) The ServiceWorkerRegisterJob class is a per-registration-process class that manages the multi-step registration/unregistration process. It takes into account the fact that looking up, unregistering, and registering are all potentially asynchronous operations. The final RegisterInternal/UnregisterInternal happens to be sychronous, but that's something hidden between ServiceWorkerStorage and ServiceWorkerRegisterJob. 3) Shutdown is a little more explicit (As you'll see I had to tweak the dispatcher a bit more to make sure the shutdown happened cleanly) Specifically what needs work before I can land this: 1) Breaking out service_worker_storage* unittests from service_worker_context_unittests 2) deleting jobs when they're complete - right now the finished jobs just hang out. 3) general cleanup of unittests - there are some inconsistencies in the way I'm dealing with callbacks, depending on the test. 4) general cleanup of the way callback.Run() is called - I need to consistently use PostTask where appropriate. And then in future patches we can deal with: 1) cancelling jobs 2) managing simultaenous/overlapping registrations 3) actual "startup" of a version so that kinuko can hook up an embedded worker host to it.
ok, a little late, but this is now ready to go. michaeln/kinuko PTAL?
Haven't read the tests yet, but sending some comments. https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:27: void ServiceWorkerContextCore::Shutdown() { When this is expected to be called? If this is always called right before context_core_.reset() we can simply do these in dtor (until we find we want to call Shutdown() earlier than it's actually destructed) https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:38: void ServiceWorkerContextCore::RegistrationComplete( nit: method order. (I'm so accustomed to the style where we have public methods and then private methods in .cc too..) https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:41: callback.Run(registration->id()); nit: until we need registration reference in this class Storage class can directly return the new registered id rather than returning a ref? https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_context_core.h:60: const base::Closure& callback); Do these methods never fail? (Maybe no while we're dealing in-memory map, but probably they do with real storage) If they could fail I expect callback signatures to look like: base::Callback<void(ServiceWorkeStatusCode result, int64 registration_id)> RegisterCallback; base::Callback<void(ServiceWorkeStatusCode result)> StatusCallback; And use StatusCallback whenever the caller doesn't need any particular return value but status. Or are you planning to use something different to propagate the error/status code? https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_context_unittest.cc:23: make_scoped_ptr(new ServiceWorkerContextCore(base::FilePath(), NULL)); nit: context_.reset(new SWCC(...)); https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_register_job.cc:33: weak_factory_.GetWeakPtr())))); In this way of chaining when something very bad happens in an early stage we still need to continue to call every callback in the chain? https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_register_job.cc:59: if (storage_) { storage_ owns this job, so technically this shouldn't be null? https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... File content/browser/service_worker/service_worker_register_job.h (right): https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_register_job.h:13: class ServiceWorkerRegisterJob { It feels this job's basically providing implementation details of Storage class, am I correct? Does this need to have its own .h/.cc file? While our version just operates on in-memory map I'd consider this impl detail is not very important while storage class's interface will be more important. If we share this assumption I feel this class's impl could be slightly more simplified while we're sticking to in-memory thing. We'll need to add some complexities when we really need it. https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:45: // loaded from disk). For now it is always pessimistically async. I think it's ok to always call back asynchronously, usually it makes things much simpler. https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:91: return worker_id++; While we're working on in-memory version this could be embedded in ServiceWorkerStorage class, couldn't it? https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... File content/browser/service_worker/service_worker_storage.h (right): https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_storage.h:66: // asynchronous. Not seem to be really used by tests? (Instead Job seems to be calling these)
I don't have a patch up in response to these, but here's what I'm working on: https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:27: void ServiceWorkerContextCore::Shutdown() { On 2013/11/25 08:17:10, kinuko wrote: > When this is expected to be called? If this is always called right before > context_core_.reset() we can simply do these in dtor (until we find we want to > call Shutdown() earlier than it's actually destructed) So here the pattern is that the refcounted objects (registration, version) can exist in a "shutdown" state - separating out the memory allocation from the "liveness" of the object. In general I'm hoping to propagate shutdown to various objects which eventually result in the release of those objects, which delete them entirely. In this case, ServiceWorkerContextCore is not refcounted, so perhaps shutdown isn't necessary here. This is a bit of a collision with the separation of ServiceWorkerContext (which was refcounted) into *Core/*Wrapper. Perhaps it isn't necessary now. I'll revisit it. In particular, I wanted to be able to handle the case where events are finishing inside ServiceWorkerVersions https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:38: void ServiceWorkerContextCore::RegistrationComplete( On 2013/11/25 08:17:10, kinuko wrote: > nit: method order. (I'm so accustomed to the style where we have public methods > and then private methods in .cc too..) Sorry, I'll try to be consistent there. https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:41: callback.Run(registration->id()); On 2013/11/25 08:17:10, kinuko wrote: > nit: until we need registration reference in this class Storage class can > directly return the new registered id rather than returning a ref? Well I'm trying to shield the Storage objects from knowing much about the registration itself (i.e. there's no reason for it to know that the id is special, nor should it have to maintain a map of id to object, or anything like that). I'll see how this looks in the Storage class and possibly change it. https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_context_core.h:60: const base::Closure& callback); On 2013/11/25 08:17:10, kinuko wrote: > Do these methods never fail? (Maybe no while we're dealing in-memory map, but > probably they do with real storage) > > If they could fail I expect callback signatures to look like: > > base::Callback<void(ServiceWorkeStatusCode result, > int64 registration_id)> RegisterCallback; > base::Callback<void(ServiceWorkeStatusCode result)> StatusCallback; > > And use StatusCallback whenever the caller doesn't need any particular return > value but status. Or are you planning to use something different to propagate > the error/status code? Yeah, I figured I could add that later, but I'm happy to do so here to make it clear. https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_context_unittest.cc:23: make_scoped_ptr(new ServiceWorkerContextCore(base::FilePath(), NULL)); On 2013/11/25 08:17:10, kinuko wrote: > nit: context_.reset(new SWCC(...)); Done. https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_register_job.cc:33: weak_factory_.GetWeakPtr())))); On 2013/11/25 08:17:10, kinuko wrote: > In this way of chaining when something very bad happens in an early stage we > still need to continue to call every callback in the chain? yes - though if we add the status field like you suggested, then any callback along the chain can pretty easily forward its error status and cause each link to short-circuit. An alternative to this would be something Promise-like where we have a success chain and a failure chain... https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_register_job.cc:59: if (storage_) { On 2013/11/25 08:17:10, kinuko wrote: > storage_ owns this job, so technically this shouldn't be null? Well this allows storage_ to go away in the middle of a registration job - think of a browser or profile shutdown (or chrome app uninstall) that happens while the registration job is running. https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... File content/browser/service_worker/service_worker_register_job.h (right): https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_register_job.h:13: class ServiceWorkerRegisterJob { On 2013/11/25 08:17:10, kinuko wrote: > It feels this job's basically providing implementation details of Storage class, > am I correct? Not exactly... There may be one or more ServiceWorkerRegisterJob operations in flight, (thinks mulitple registration/unregistrations happening at the same time) and a Job consists of a chain of asynchronous events. The goal of this class is to abstract out that chain of events, so you can imagine multiple Jobs running from a single Storage instance, and thus being able (ultimately) to cancel, abort, or complete a job. > Does this need to have its own .h/.cc file? While our version > just operates on in-memory map I'd consider this impl detail is not very > important while storage class's interface will be more important. If we share > this assumption I feel this class's impl could be slightly more simplified while > we're sticking to in-memory thing. We'll need to add some complexities when we > really need it. Well I made this in response to folks wanting a slightly less-temporary solution. As I broke out the impl. into the storage class, I realized I had this chain of async events (find, unregister, register, install, upgrade, activate, etc) and needed a place to contain that process. michaeln suggested the notion of a "job" and I think it fits the pattern well.
https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_register_job.cc:33: weak_factory_.GetWeakPtr())))); On 2013/11/26 00:19:14, alecflett wrote: > On 2013/11/25 08:17:10, kinuko wrote: > > In this way of chaining when something very bad happens in an early stage we > > still need to continue to call every callback in the chain? > > yes - though if we add the status field like you suggested, then any callback > along the chain can pretty easily forward its error status and cause each link > to short-circuit. > > An alternative to this would be something Promise-like where we have a success > chain and a failure chain... Ok, with status code we can short-cut, which may not be that bad. (I might say something different after I take a look at the new code, but anyway :)) https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_register_job.cc:59: if (storage_) { On 2013/11/26 00:19:14, alecflett wrote: > On 2013/11/25 08:17:10, kinuko wrote: > > storage_ owns this job, so technically this shouldn't be null? > > Well this allows storage_ to go away in the middle of a registration job - think > of a browser or profile shutdown (or chrome app uninstall) that happens while > the registration job is running. But storage class does own this Job instance via ScopedVector, doesn't it? https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... File content/browser/service_worker/service_worker_register_job.h (right): https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_register_job.h:13: class ServiceWorkerRegisterJob { On 2013/11/26 00:19:14, alecflett wrote: > On 2013/11/25 08:17:10, kinuko wrote: > > It feels this job's basically providing implementation details of Storage > class, > > am I correct? > > Not exactly... There may be one or more ServiceWorkerRegisterJob operations in > flight, (thinks mulitple registration/unregistrations happening at the same > time) and a Job consists of a chain of asynchronous events. The goal of this > class is to abstract out that chain of events, so you can imagine multiple Jobs > running from a single Storage instance, and thus being able (ultimately) to > cancel, abort, or complete a job. > > > Does this need to have its own .h/.cc file? While our version > > just operates on in-memory map I'd consider this impl detail is not very > > important while storage class's interface will be more important. If we share > > this assumption I feel this class's impl could be slightly more simplified > while > > we're sticking to in-memory thing. We'll need to add some complexities when we > > really need it. > > Well I made this in response to folks wanting a slightly less-temporary > solution. As I broke out the impl. into the storage class, I realized I had this > chain of async events (find, unregister, register, install, upgrade, activate, > etc) and needed a place to contain that process. michaeln suggested the notion > of a "job" and I think it fits the pattern well. Ok, the class structure (storage class starts this and owns jobs, and the existence of job is not exposed outside the storage class) made me think like this one's implementing internal details of storage. I can imagine some benefits having a separate job class for registration, but they're not too obvious to me (yet) in the current code structure. Do you have some thoughts on: what interfaces we'll want to have for start/abort/wait a job, who will call those interfaces, and who should own/manage the job's lifetime?
On 2013/11/26 08:02:00, kinuko wrote: > https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... > File content/browser/service_worker/service_worker_register_job.cc (right): > > https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... > content/browser/service_worker/service_worker_register_job.cc:33: > weak_factory_.GetWeakPtr())))); > On 2013/11/26 00:19:14, alecflett wrote: > > On 2013/11/25 08:17:10, kinuko wrote: > > > In this way of chaining when something very bad happens in an early stage we > > > still need to continue to call every callback in the chain? > > > > yes - though if we add the status field like you suggested, then any callback > > along the chain can pretty easily forward its error status and cause each link > > to short-circuit. > > > > An alternative to this would be something Promise-like where we have a success > > chain and a failure chain... > > Ok, with status code we can short-cut, which may not be that bad. (I might say > something different after I take a look at the new code, but anyway :)) > > https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... > content/browser/service_worker/service_worker_register_job.cc:59: if (storage_) > { > On 2013/11/26 00:19:14, alecflett wrote: > > On 2013/11/25 08:17:10, kinuko wrote: > > > storage_ owns this job, so technically this shouldn't be null? > > > > Well this allows storage_ to go away in the middle of a registration job - > think > > of a browser or profile shutdown (or chrome app uninstall) that happens while > > the registration job is running. > > But storage class does own this Job instance via ScopedVector, doesn't it? > > https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... > File content/browser/service_worker/service_worker_register_job.h (right): > > https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... > content/browser/service_worker/service_worker_register_job.h:13: class > ServiceWorkerRegisterJob { > On 2013/11/26 00:19:14, alecflett wrote: > > On 2013/11/25 08:17:10, kinuko wrote: > > > It feels this job's basically providing implementation details of Storage > > class, > > > am I correct? > > > > Not exactly... There may be one or more ServiceWorkerRegisterJob operations in > > flight, (thinks mulitple registration/unregistrations happening at the same > > time) and a Job consists of a chain of asynchronous events. The goal of this > > class is to abstract out that chain of events, so you can imagine multiple > Jobs > > running from a single Storage instance, and thus being able (ultimately) to > > cancel, abort, or complete a job. > > > > > Does this need to have its own .h/.cc file? While our version > > > just operates on in-memory map I'd consider this impl detail is not very > > > important while storage class's interface will be more important. If we > share > > > this assumption I feel this class's impl could be slightly more simplified > > while > > > we're sticking to in-memory thing. We'll need to add some complexities when > we > > > really need it. > > > > Well I made this in response to folks wanting a slightly less-temporary > > solution. As I broke out the impl. into the storage class, I realized I had > this > > chain of async events (find, unregister, register, install, upgrade, activate, > > etc) and needed a place to contain that process. michaeln suggested the notion > > of a "job" and I think it fits the pattern well. > > Ok, the class structure (storage class starts this and owns jobs, and the > existence of job is not exposed outside the storage class) made me think like > this one's implementing internal details of storage. > > I can imagine some benefits having a separate job class for registration, but > they're not too obvious to me (yet) in the current code structure. Do you have > some thoughts on: what interfaces we'll want to have for start/abort/wait a job, > who will call those interfaces, and who should own/manage the job's lifetime? An example for "wait": if two pages in the same domain both call navigator.registerServiceWorker("/*", "script.js"); during pageload, and these pages are loaded around the same time, then the "first" one will start the registration, (i.e. just the first one to get to ServiceWorkerStorage::Register()) and create the ServiceWorkerRegisterJob... then the "second" one will come in and ServiceWorkerStorage should recognize that there is already a job running for ("/*", "script.js") and hook up to the existing job to call that. I think external abort/cancel happens more when the page is torn down before the registration is complete. Also, a job may be effectively "paused" while waiting for activation/replacement: https://github.com/slightlyoff/ServiceWorker/blob/master/explainer.md#replace... so there may be needs for cancelation. Finally, I think a job may effectively cancel/abort itself if for example, the install event fails for some reason. I actually don't know what happens in this case.
Ok new patch is up. I'm not totally happy with the enums as they stand and will try to rev them one more today..
ok, I'm happy (enough) with this now. There's obviously a lot more that needs to go in here, and the use of the various enums *seems* superfluous right now.. but I'd like to land this and keep iterating so that we can do things like add an active ServiceWorkerVersion and whatnot. https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:45: // loaded from disk). For now it is always pessimistically async. On 2013/11/25 08:17:10, kinuko wrote: > I think it's ok to always call back asynchronously, usually it makes things much > simpler. I completely agree. Its mainly that this is in the critical path for URL resolution - though I guess its really FindRegistrationForDocument that is in the critical path. I'll remove this particular comment. https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:91: return worker_id++; On 2013/11/25 08:17:10, kinuko wrote: > While we're working on in-memory version this could be embedded in > ServiceWorkerStorage class, couldn't it? I'm not sure of the value of that? This local function prevents anyone from making any explicit dependencies on the choice of id.
On 2013/11/26 20:23:47, alecflett wrote: > On 2013/11/26 08:02:00, kinuko wrote: > > > https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... > > File content/browser/service_worker/service_worker_register_job.cc (right): > > > > > https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... > > content/browser/service_worker/service_worker_register_job.cc:33: > > weak_factory_.GetWeakPtr())))); > > On 2013/11/26 00:19:14, alecflett wrote: > > > On 2013/11/25 08:17:10, kinuko wrote: > > > > In this way of chaining when something very bad happens in an early stage > we > > > > still need to continue to call every callback in the chain? > > > > > > yes - though if we add the status field like you suggested, then any > callback > > > along the chain can pretty easily forward its error status and cause each > link > > > to short-circuit. > > > > > > An alternative to this would be something Promise-like where we have a > success > > > chain and a failure chain... > > > > Ok, with status code we can short-cut, which may not be that bad. (I might say > > something different after I take a look at the new code, but anyway :)) > > > > > https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... > > content/browser/service_worker/service_worker_register_job.cc:59: if > (storage_) > > { > > On 2013/11/26 00:19:14, alecflett wrote: > > > On 2013/11/25 08:17:10, kinuko wrote: > > > > storage_ owns this job, so technically this shouldn't be null? > > > > > > Well this allows storage_ to go away in the middle of a registration job - > > think > > > of a browser or profile shutdown (or chrome app uninstall) that happens > while > > > the registration job is running. > > > > But storage class does own this Job instance via ScopedVector, doesn't it? > > > > > https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... > > File content/browser/service_worker/service_worker_register_job.h (right): > > > > > https://codereview.chromium.org/62203007/diff/320001/content/browser/service_... > > content/browser/service_worker/service_worker_register_job.h:13: class > > ServiceWorkerRegisterJob { > > On 2013/11/26 00:19:14, alecflett wrote: > > > On 2013/11/25 08:17:10, kinuko wrote: > > > > It feels this job's basically providing implementation details of Storage > > > class, > > > > am I correct? > > > > > > Not exactly... There may be one or more ServiceWorkerRegisterJob operations > in > > > flight, (thinks mulitple registration/unregistrations happening at the same > > > time) and a Job consists of a chain of asynchronous events. The goal of this > > > class is to abstract out that chain of events, so you can imagine multiple > > Jobs > > > running from a single Storage instance, and thus being able (ultimately) to > > > cancel, abort, or complete a job. > > > > > > > Does this need to have its own .h/.cc file? While our version > > > > just operates on in-memory map I'd consider this impl detail is not very > > > > important while storage class's interface will be more important. If we > > share > > > > this assumption I feel this class's impl could be slightly more simplified > > > while > > > > we're sticking to in-memory thing. We'll need to add some complexities > when > > we > > > > really need it. > > > > > > Well I made this in response to folks wanting a slightly less-temporary > > > solution. As I broke out the impl. into the storage class, I realized I had > > this > > > chain of async events (find, unregister, register, install, upgrade, > activate, > > > etc) and needed a place to contain that process. michaeln suggested the > notion > > > of a "job" and I think it fits the pattern well. > > > > Ok, the class structure (storage class starts this and owns jobs, and the > > existence of job is not exposed outside the storage class) made me think like > > this one's implementing internal details of storage. > > > > I can imagine some benefits having a separate job class for registration, but > > they're not too obvious to me (yet) in the current code structure. Do you have > > some thoughts on: what interfaces we'll want to have for start/abort/wait a > job, > > who will call those interfaces, and who should own/manage the job's lifetime? > > An example for "wait": > > if two pages in the same domain both call > > navigator.registerServiceWorker("/*", "script.js"); > > during pageload, and these pages are loaded around the same time, then the > "first" one will start the registration, (i.e. just the first one to get to > ServiceWorkerStorage::Register()) and create the ServiceWorkerRegisterJob... > then the "second" one will come in and ServiceWorkerStorage should recognize > that there is already a job running for ("/*", "script.js") and hook up to the > existing job to call that. > > I think external abort/cancel happens more when the page is torn down before the > registration is complete. Also, a job may be effectively "paused" while waiting > for activation/replacement: > https://github.com/slightlyoff/ServiceWorker/blob/master/explainer.md#replace... > so there may be needs for cancelation. > > Finally, I think a job may effectively cancel/abort itself if for example, the > install event fails for some reason. I actually don't know what happens in this > case. Where do we want to expose these Cancel or Pause interfaces? I have a feeling that all these logic could be hidden behind a set of public interfaces like Register() (this can record in-flight and pending registrations for the same pattern internally), Cancel() and Abort(), and usually that makes things simpler outside the storage class. (Cancelling itself is yet another internal topic which can be handled internally) Having this in a separate .h/.cc class is still fine (actually it might be preferable), I think my question was if Storage is going to be the central place to have these public interfaces to interact with ongoing registration process or not. If yes the model is simple, and other classes than Storage do not need to know details about the Job. If no that sounds we're going to have more complex object model, and I wanted to know if that's the case. For now I'm assuming the answer is yes.
https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:92: void ServiceWorkerContextCore::RegisterServiceWorker( nit: method order. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_context_core.h:54: }; Assume that these enums are kind of tentative probably a single enum would be fine for both operations for now? https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_context_core.h:55: typedef base::Callback<void(RegistrationStatus, int64)> RegistrationCallback; nit: we can write param names in Callback typedefs, that'd be probably helpful for readers to decode what 'int64' means e.g. typedef baes::Callback<void(RegistrationStatus status, int64 registration_id)> RegistrationCallback; https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_register_job.cc:23: // callbacks may be called asynchronously by the previous callback. Are we going to create all these callbacks in reverse order at the beginning? This task's going to do a lot of jobs, I wonder just chaining next callback in each call might be readable. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... File content/browser/service_worker/service_worker_register_job.h (right): https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_register_job.h:44: void StartUnregister(const GURL& pattern); It's one off operation class, I think ideally having separate classes for registration and unregistration would be clearer. Can you clearly comment that either StartRegister and StartUnregister should be called exactly once for each instance? https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:52: static_cast<scoped_refptr<ServiceWorkerRegistration> >(NULL))); Just passing scoped_refptr<ServiceWorkerRegistration>() would work https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:77: it->second))); No need of static_cast https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:86: static_cast<scoped_refptr<ServiceWorkerRegistration> >(NULL))); ditto https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:93: } I suggested having this method as a member method because I thought we're going to generate a new ID based a persisted value on storage, and if so we'll eventually have this method in Storage. Am I right? (Otherwise could you please move this at the top, so that namespace doesn't interleave much?) https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:170: break; We could return here, and https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:173: DCHECK(found) << "Deleting non-existent job. "; put NOTREACHED() << "..." instead of checking found https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... File content/browser/service_worker/service_worker_storage.h (right): https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage.h:42: }; If this (or similar) enum is needed by multiple classes I'd name it ServiceWorkerRegistrationStatus and put it in its own .h file so that we can consistently use same enums in our codebase. Could we do so for this? https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage.h:90: // This is the in-memory registration. Eventually the registration will be persisted to disk. nit: over 80 cols https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:27: ServiceWorkerStorage* storage_; scoped_ptr ? Looks like it's leaking https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:33: TEST_F(ServiceWorkerStorageTest, RegisterMatch) { RegisterMatch -> PatternMatches to make it clearer what method it's testing? https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:52: namespace { Can we move this anon namespace to the top? (e.g. right after 'namespace content {') I'd like to have lower routines/details in another namespace together and separated from the main test code https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:104: message_loop_.RunUntilIdle(); It's deprecated, I think we prefer base::RunLoop().RunUntilIdle() now https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:121: EXPECT_FALSE(called); If we assume FindRegistrationForDocument could return either synchronously or asynchronously I think we should drop these pre-operation expectations here. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:223: ASSERT_NE(scoped_refptr<ServiceWorkerRegistration>(NULL), registration); Can we test FindRegistrationForPattern returns OK here too? https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:315: ServiceWorkerStorage::REGISTRATION_OK, &called, &new_registration)); new_registration -> new_registration_by_pattern ? https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:380: ASSERT_EQ(new_registration, old_registration); new_registration -> new_registration_by_pattern ?
[new patch will be up later today, that includes all the 'Done' parts here] https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:92: void ServiceWorkerContextCore::RegisterServiceWorker( On 2013/11/27 03:45:23, kinuko wrote: > nit: method order. Done. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_context_core.h:54: }; On 2013/11/27 03:45:23, kinuko wrote: > Assume that these enums are kind of tentative probably a single enum would be > fine for both operations for now? Done. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_context_core.h:55: typedef base::Callback<void(RegistrationStatus, int64)> RegistrationCallback; On 2013/11/27 03:45:23, kinuko wrote: > nit: we can write param names in Callback typedefs, that'd be probably helpful > for readers to decode what 'int64' means > > e.g. > > typedef baes::Callback<void(RegistrationStatus status, > int64 registration_id)> RegistrationCallback; Done. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_register_job.cc:23: // callbacks may be called asynchronously by the previous callback. On 2013/11/27 03:45:23, kinuko wrote: > Are we going to create all these callbacks in reverse order at the beginning? > This task's going to do a lot of jobs, I wonder just chaining next callback in > each call might be readable. Yeah, I started this way (see previous iterations of the patch) and got into a fairly confusing nesting chain, which is unfortunately bound to get deeper as we add install and activate steps... this is where I really wanted Promises. Here I'd like to leave it as is (flat and ordered, though backwards) and try to see if I can figure out a nice abstraction in the meantime. Would that be ok? I can put in a TODO if you'd like. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... File content/browser/service_worker/service_worker_register_job.h (right): https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_register_job.h:44: void StartUnregister(const GURL& pattern); On 2013/11/27 03:45:23, kinuko wrote: > It's one off operation class, I think ideally having separate classes for > registration and unregistration would be clearer. I'd like to avoid doing that just yet. They share some code, and will likely share more. Also, the ServiceWorkerStorage class is going to have to keep track of multiple jobs, inspecting which jobs overlap, which means they're going to have to share a base class in order to be treated uniformly. This means introducing a lot (IMO) of abstraction - ServiceWorkerRegistrationJobBase, ServiceWorkerRegisterJob, ServiceWorkerUnregisterJob, etc as well as virtual Start() methods, etc - without an big gain. I'm happy to refactor if I'm shown to be wrong, but can we start with a single class now? (there's nothing specified yet about overlapping registration vs unregistration, but when it does, I'm fairly certain the Storage class is going to have to inspect the jobs' patterns independent of type) > > Can you clearly comment that either StartRegister and StartUnregister should be > called exactly once for each instance? Done. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:52: static_cast<scoped_refptr<ServiceWorkerRegistration> >(NULL))); On 2013/11/27 03:45:23, kinuko wrote: > Just passing scoped_refptr<ServiceWorkerRegistration>() would work Done. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:77: it->second))); On 2013/11/27 03:45:23, kinuko wrote: > No need of static_cast Done. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:86: static_cast<scoped_refptr<ServiceWorkerRegistration> >(NULL))); On 2013/11/27 03:45:23, kinuko wrote: > ditto Done. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:93: } On 2013/11/27 03:45:23, kinuko wrote: > I suggested having this method as a member method because I thought we're going > to generate a new ID based a persisted value on storage, and if so we'll > eventually have this method in Storage. Am I right? I think we'll have some variation on it, but it itself may end up being an asynchronous operation (i.e. if it involves hitting the DB because it's the first registration call, etc) so I'd prefer to just leave this near where it will be used. > > (Otherwise could you please move this at the top, so that namespace doesn't > interleave much?) Done. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:170: break; On 2013/11/27 03:45:23, kinuko wrote: > We could return here, and Done. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:173: DCHECK(found) << "Deleting non-existent job. "; On 2013/11/27 03:45:23, kinuko wrote: > put NOTREACHED() << "..." instead of checking found Done. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... File content/browser/service_worker/service_worker_storage.h (right): https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage.h:42: }; On 2013/11/27 03:45:23, kinuko wrote: > If this (or similar) enum is needed by multiple classes I'd name it > ServiceWorkerRegistrationStatus and put it in its own .h file so that we can > consistently use same enums in our codebase. Could we do so for this? Sure. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:27: ServiceWorkerStorage* storage_; On 2013/11/27 03:45:23, kinuko wrote: > scoped_ptr ? Looks like it's leaking Done. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:33: TEST_F(ServiceWorkerStorageTest, RegisterMatch) { On 2013/11/27 03:45:23, kinuko wrote: > RegisterMatch -> PatternMatches to make it clearer what method it's testing? Done. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:52: namespace { On 2013/11/27 03:45:23, kinuko wrote: > Can we move this anon namespace to the top? (e.g. right after 'namespace content > {') > > I'd like to have lower routines/details in another namespace together and > separated from the main test code Done. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:104: message_loop_.RunUntilIdle(); On 2013/11/27 03:45:23, kinuko wrote: > It's deprecated, I think we prefer base::RunLoop().RunUntilIdle() now I'm not sure how to hook up base::RunLoop() to the IO thread then. What this test does is create an artificial runloop that acts like it's the IO thread. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:121: EXPECT_FALSE(called); On 2013/11/27 03:45:23, kinuko wrote: > If we assume FindRegistrationForDocument could return either synchronously or > asynchronously I think we should drop these pre-operation expectations here. Well mostly I want to track that behavior - I don't want that to change arbitrarily without us aware of that possibility. Like you pointed out, we *might* just want to stick with asynchronicity, but I want this to be a consious choice (i.e. one such that the developer is aware they've changed the behavior because they have to change tests) https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:223: ASSERT_NE(scoped_refptr<ServiceWorkerRegistration>(NULL), registration); On 2013/11/27 03:45:23, kinuko wrote: > Can we test FindRegistrationForPattern returns OK here too? oops, I think that was there before I shuffled this code around. Done. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:315: ServiceWorkerStorage::REGISTRATION_OK, &called, &new_registration)); On 2013/11/27 03:45:23, kinuko wrote: > new_registration -> new_registration_by_pattern ? oops! typo. Done. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:380: ASSERT_EQ(new_registration, old_registration); On 2013/11/27 03:45:23, kinuko wrote: > new_registration -> new_registration_by_pattern ? Done.
Ok, outstanding issues have been addressed. I need to convert ServiceWorkerContextTest to use TestBrowserThreadBundle, but I'll do that separately. https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/62203007/diff/470001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:104: message_loop_.RunUntilIdle(); On 2013/12/02 16:11:35, alecflett wrote: > On 2013/11/27 03:45:23, kinuko wrote: > > It's deprecated, I think we prefer base::RunLoop().RunUntilIdle() now > > I'm not sure how to hook up base::RunLoop() to the IO thread then. What this > test does is create an artificial runloop that acts like it's the IO thread. Ah! I figured it out... TestBrowserThreadBundle
Sorry if I'm blocking you, I think I have a few more comments. Many of them are for style-related, + some design comment. (If this iteration takes too long we can have short VC or something if it helps, I can try wake up early) https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:31: is_shutdown_ = true; I still don't really see how having this separately could help things (after we have Wrapper). If we're not yet sure we can just add this later? If we have situations where we want to do 2-phase termination, e.g. call Shutdown path first and then delete things, then this is totally reasonable, but otherwise always require calling additional method before shutdown usually just increases coding burden-- that's my concern. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:113: callback.Run(status); You meant callback.Run(REGISTRATION_OK) ? Otherwise we can remove this if/else and just call Callback.run(status); https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_context_unittest.cc:39: } Do we need these static methods? Just calling base::Bind(&Callback, &called) looks short & clear enough to me. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_context_unittest.cc:55: } All these static methods can be placed outside this class in an anonymous namespace, so that it becomes clearer that they have no relationships to this test class fixture. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_context_unittest.cc:65: } And this could be in the anonymous namespace too. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_register_job.cc:73: scoped_refptr<ServiceWorkerRegistration>(NULL))); nit: no need of NULL https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_register_job.cc:77: // operation. Pass its resulting status though 'callback'. though -> through ? https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... File content/browser/service_worker/service_worker_register_job.h (right): https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_register_job.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. Some files have (c) while others do not. Afaik our current agreement in the community is we do not need this (c), so maybe we should just drop all (c) from new files. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_register_job.h:40: // This method should be called once and only once per job. To perform all these operations would Storage be the right place to own this job class? It looks it needs to do a bit of storage-related operation but also needs to interact with a lot of other classes. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_register_job.h:64: // These methods convert incomplete sentence? https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... File content/browser/service_worker/service_worker_registration_status.h (right): https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_registration_status.h:16: REGISTER_FAILED, REGISTRATION_FAILED to be consistent? https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:105: registration_jobs_.push_back(job.release()); We use multiple async calls to FindRegistration and Unregistration, or Find + Unregister + Register. How do you plan to make sure multiple registration and unregistration requests don't interleave each other? I imagine we'll probably need some serialization, and for storage-related atomic operations we may want to just perform all in a single task. For the latter reason I feel storage methods for register & unregister could be a single async method (it could internally call common methods)... wdyt? https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... File content/browser/service_worker/service_worker_storage.h (right): https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_storage.h:87: typedef ScopedVector<ServiceWorkerRegisterJob> RegistrationJobList; Can you put typedef's at the beginning of private: section per our coding style? http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:60: base::RunLoop().RunUntilIdle(); As I commented on another CL, just calling base::RunLoop().RunUntilIdle() should be fine as far as we're using a single message loop.
Ok, I've addressed *most* comments. There are a few things that I'd like to leave as-is for now (mostly in the interest of getting this landed) - I'm happy to continue refactoring in future patches as it makes sense. In particular, moving the ownership of ServiceWorkerRegistrationJob to ServiceWorkerContextCore turned out to be somewhat disruptive, and ended up binding up the ServiceWorkerContextCore, ServiceWorkerStorage, and ServiceWorkerRegistrationJob classes a little more tightly than I liked.. the situation didn't seem any better and it keeps the unittests simpler. (i.e. the storage unittests don't require a context at all right now) Down the road, perhaps ServiceWorkerStorage can be split into two classes, but I'd like to make that split later rather than complicate this patch further. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:31: is_shutdown_ = true; On 2013/12/04 13:04:29, kinuko wrote: > I still don't really see how having this separately could help things (after we > have Wrapper). If we're not yet sure we can just add this later? > > If we have situations where we want to do 2-phase termination, e.g. call > Shutdown path first and then delete things, then this is totally reasonable, but > otherwise always require calling additional method before shutdown usually just > increases coding burden-- that's my concern. Yeah, perhaps I've taken this pattern too far. I'll remove this and the storage one. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:113: callback.Run(status); On 2013/12/04 13:04:29, kinuko wrote: > You meant callback.Run(REGISTRATION_OK) ? > > Otherwise we can remove this if/else and just call Callback.run(status); oops, yes! Done. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_context_unittest.cc:39: } On 2013/12/04 13:04:29, kinuko wrote: > Do we need these static methods? Just calling base::Bind(&Callback, &called) > looks short & clear enough to me. I found these helpful both for readability but also because while iterating on the patch I could explicitly type the helper with ServiceWorkerContextCore::[Un]registrationCallback - otherwise I had to comb through heaps of template expansions when the signature to base::Bind() doesn't match the callback. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_context_unittest.cc:55: } On 2013/12/04 13:04:29, kinuko wrote: > All these static methods can be placed outside this class in an anonymous > namespace, so that it becomes clearer that they have no relationships to this > test class fixture. oops, I thought I had done that.. apparently not! Anyway, done. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_context_unittest.cc:65: } On 2013/12/04 13:04:29, kinuko wrote: > And this could be in the anonymous namespace too. Done. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_register_job.cc:73: scoped_refptr<ServiceWorkerRegistration>(NULL))); On 2013/12/04 13:04:29, kinuko wrote: > nit: no need of NULL Done. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_register_job.cc:77: // operation. Pass its resulting status though 'callback'. On 2013/12/04 13:04:29, kinuko wrote: > though -> through ? Done. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... File content/browser/service_worker/service_worker_register_job.h (right): https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_register_job.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/12/04 13:04:29, kinuko wrote: > Some files have (c) while others do not. Afaik our current agreement in the > community is we do not need this (c), so maybe we should just drop all (c) from > new files. Whatever I check in, there's some bot that always fixes them. I can never remember which we need, but I'll make 'em consistent with the existing files. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_register_job.h:40: // This method should be called once and only once per job. On 2013/12/04 13:04:29, kinuko wrote: > To perform all these operations would Storage be the right place to own this job > class? It looks it needs to do a bit of storage-related operation but also > needs to interact with a lot of other classes. I think as far as owning the Job class instance, its a reasonable place for the near term - my hope is to shield ServiceWorkerStorage from the implementation details of the job, so it kind of doesn't matter who owns it. I guess the ServiceWorkerContext could also own it, but it has a 1:1 relationship with ServiceWorkerStorage and I'm trying not to clutter up the main context class. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_register_job.h:64: // These methods convert On 2013/12/04 13:04:29, kinuko wrote: > incomplete sentence? Done. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... File content/browser/service_worker/service_worker_registration_status.h (right): https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_registration_status.h:16: REGISTER_FAILED, On 2013/12/04 13:04:29, kinuko wrote: > REGISTRATION_FAILED to be consistent? Done. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:105: registration_jobs_.push_back(job.release()); On 2013/12/04 13:04:29, kinuko wrote: > We use multiple async calls to FindRegistration and Unregistration, or Find + > Unregister + Register. > How do you plan to make sure multiple registration and unregistration requests > don't interleave each other? ultimately registration_jobs_ will evolve to handle this - I'm thinking it will become a map of pattern -> list of ServiceWorkerRegisterJob instances that are in process. This will also allow things like the ability for an unregister() to cancel an in-process register(). > I imagine we'll probably need some serialization, > and for storage-related atomic operations we may want to just perform all in a > single task. For the latter reason I feel storage methods for register & > unregister could be a single async method (it could internally call common > methods)... wdyt? Well the approach I'm taking is to let them share a common class but let the Start* be the point where we diverge. In its current form this scaffolding may seem excessive, but I think down the road it will make these "Register" and "Unregister" methods simpler, (because I think StartRegister and StartUnregister are going to diverge further) and the Start* is what sets up the chain of async operations. If it turns out later to be a lot of duplicated code between Register and Unregister, I'm happy to merge them. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... File content/browser/service_worker/service_worker_storage.h (right): https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_storage.h:87: typedef ScopedVector<ServiceWorkerRegisterJob> RegistrationJobList; On 2013/12/04 13:04:29, kinuko wrote: > Can you put typedef's at the beginning of private: section per our coding style? > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... Done. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:60: base::RunLoop().RunUntilIdle(); On 2013/12/04 13:04:29, kinuko wrote: > As I commented on another CL, just calling base::RunLoop().RunUntilIdle() should > be fine as far as we're using a single message loop. I feel like I added this because otherwise the threads don't realize they're in the IO loop.
> I feel like I added this because otherwise the threads don't realize they're in > the IO loop. oops, stale comment: I did fix this as you suggested
I'm giving lgtm to unblock things, though I still have some concern about Storage's method organization. I hope we can iterate on this to make things clearer & more robust as we go on. https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/62203007/diff/490001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:105: registration_jobs_.push_back(job.release()); On 2013/12/06 05:43:33, alecflett wrote: > On 2013/12/04 13:04:29, kinuko wrote: > > We use multiple async calls to FindRegistration and Unregistration, or Find + > > Unregister + Register. > > How do you plan to make sure multiple registration and unregistration requests > > don't interleave each other? > > ultimately registration_jobs_ will evolve to handle this - I'm thinking it will > become a map of pattern -> list of ServiceWorkerRegisterJob instances that are > in process. This will also allow things like the ability for an unregister() to > cancel an in-process register(). > > > I imagine we'll probably need some serialization, > > and for storage-related atomic operations we may want to just perform all in a > > single task. For the latter reason I feel storage methods for register & > > unregister could be a single async method (it could internally call common > > methods)... wdyt? > > Well the approach I'm taking is to let them share a common class but let the > Start* be the point where we diverge. In its current form this scaffolding may > seem excessive, but I think down the road it will make these "Register" and > "Unregister" methods simpler, (because I think StartRegister and StartUnregister > are going to diverge further) and the Start* is what sets up the chain of async > operations. If it turns out later to be a lot of duplicated code between > Register and Unregister, I'm happy to merge them. Ah, my comment was probably not clear enough, I'm not saying that we should merge Register and Unregister, but wondering if we should have one (or fewer) atomic async method for Register and another for Unregister. Of course they could call common private methods internally when necessary (like currently we do in this patch), but they may not need to be in a series of multiple methods? For example in the current model we're supposed to make two async calls to 'Find' and 'UnregisterInternal' to unregister, but when we think about how we'll be interacting with the actual db this model sounds a bit weird, since we need to do always 'Find' the registration to 'Unregister' things (e.g. like we do registration_by_pattern_.find in both FindRegistrationForPattern and UnregisterInternal), this feels a bit wasteful. And because they're supposed to be called asynchronously another 'RegisterInternal' or 'UnregisterInternal' could easily interleave between the 'Find' and 'Unregister'. To be more specific, I think, say, having a separate 'Find' method itself totally makes sense as it'd make testing much easier, and I understand that the atomicity problem can be addressed at the bigger 'job' level as you mention, but I'm still feeling that some of them are probably not need to be separate *async* methods, and having simpler method model would also make the storage model simple and robust. https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... content/browser/service_worker/service_worker_context_unittest.cc:19: namespace { nit: please insert an empty line here https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... File content/browser/service_worker/service_worker_register_job.h (right): https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... content/browser/service_worker/service_worker_register_job.h:21: ServiceWorkerRegistration*)> RegistrationCompleteCallback; nit: for consistency, I'd drop 'status' on the second line or add param names to all https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:18: namespace { nit: I'd like to have one empty line after namespace { (unless it's for one-liner or forward decls etc) (A few old-timer reviewers gave me this comment in my early days, so I'm basically trying to keep the rule)
On 2013/12/06 15:24:48, kinuko wrote: > I'm giving lgtm to unblock things, though I still have some concern about > Storage's method organization. I hope we can iterate on this to make things > clearer & more robust as we go on. Of course. I fully expect to keep refactoring as we iterate. > > Ah, my comment was probably not clear enough, I'm not saying that we should > merge Register and Unregister, but wondering if we should have one (or fewer) > atomic async method for Register and another for Unregister. Of course they > could call common private methods internally when necessary (like currently we > do in this patch), but they may not need to be in a series of multiple methods? > For example in the current model we're supposed to make two async calls to > 'Find' and 'UnregisterInternal' to unregister, but when we think about how we'll > be interacting with the actual db this model sounds a bit weird, since we need > to do always 'Find' the registration to 'Unregister' things (e.g. like we do > registration_by_pattern_.find in both FindRegistrationForPattern and > UnregisterInternal), this feels a bit wasteful. And because they're supposed to > be called asynchronously another 'RegisterInternal' or 'UnregisterInternal' > could easily interleave between the 'Find' and 'Unregister'. So regarding the interleaving: That's what the registration_jobs_ structure will manage, though not in this patch: Basically only one job for a given pattern can run at a time. So the idea is that once a job starts, it really has complete authority over the registration data for the given pattern. The ServiceWorkerStorage is basically the authority over which jobs run and in which order. This is similar in some ways to the 'scoping' model in IndexedDB transactions: transactions consist of multiple asynchronous operations, but two readwrite transactions with overlapping scope cannot run at the same time - in the transactional database world, it's a way of avoiding locks by queuing smarter. (in our case, overlapping scope means having an identical pattern) Whats nice is that read-only operations (like Find) can probably, depending on how the spec is written, be interleaved with existing write operations. Regarding the wastefulness - I understand the concern, as maybe this looks unnecessarily granular. I personally lean towards having a bunch of small async operations that each do only one thing (i.e. "find", "unregister", "load database from disk", etc) as building blocks, and then chaining them together to make more complex pipelines. If we do find there are performance problems with specific parts of the pipeline I'm happy to make specific optimizations to combine these simple building blocks into fewer synchronous operations... but I only expect perf problems when these things occur in a tight loop.. but for something like the registration flow, which is going to occur at most once or twice per pageload, (and not in the critical path of the load either, since the API is asychronous!) I suspect that we won't see a problem. Regarding robustness - I am keeping robustness/correctness top of mind as I work through this - I've worked with a number of different storage systems with different threading/synchronizing models and those are driving the design here - I've found that having very small operations that you can reason about (i.e. chain them together naturally, define boundaries of atomicity, propagate errors, etc) usually results in valuing robustness over performance, but then you can optimize more easily. I'd rather have a robust system that we optimize later than trying to eek out tiny performance wins now. As an aside, one nice thing about expressing everything as small asynchronous operations like this is that we can keep inserting additional small operations (e.g. "load database from disk" and "write registration record") as well as error propagation (e.g. "Find" failed because "load database from disk" failed) into the existing pipeline without changing the external API or behavior of ServiceWorkerRegisterJob > > To be more specific, I think, say, having a separate 'Find' method itself > totally makes sense as it'd make testing much easier, and I understand that the > atomicity problem can be addressed at the bigger 'job' level as you mention, but > I'm still feeling that some of them are probably not need to be separate *async* > methods, and having simpler method model would also make the storage model > simple and robust. > > https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... > File content/browser/service_worker/service_worker_context_unittest.cc (right): > > https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... > content/browser/service_worker/service_worker_context_unittest.cc:19: namespace > { > nit: please insert an empty line here > > https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... > File content/browser/service_worker/service_worker_register_job.h (right): > > https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... > content/browser/service_worker/service_worker_register_job.h:21: > ServiceWorkerRegistration*)> RegistrationCompleteCallback; > nit: for consistency, I'd drop 'status' on the second line or add param names to > all > > https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... > File content/browser/service_worker/service_worker_storage_unittest.cc (right): > > https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... > content/browser/service_worker/service_worker_storage_unittest.cc:18: namespace > { > nit: I'd like to have one empty line after namespace { (unless it's for > one-liner or forward decls etc) > > (A few old-timer reviewers gave me this comment in my early days, so I'm > basically trying to keep the rule)
jam@ - TBR'ing you for new files added to content_*.gyp's https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... content/browser/service_worker/service_worker_context_unittest.cc:19: namespace { On 2013/12/06 15:24:49, kinuko wrote: > nit: please insert an empty line here Done. https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... File content/browser/service_worker/service_worker_register_job.h (right): https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... content/browser/service_worker/service_worker_register_job.h:21: ServiceWorkerRegistration*)> RegistrationCompleteCallback; On 2013/12/06 15:24:49, kinuko wrote: > nit: for consistency, I'd drop 'status' on the second line or add param names to > all Done. https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/62203007/diff/510001/content/browser/service_... content/browser/service_worker/service_worker_storage_unittest.cc:18: namespace { On 2013/12/06 15:24:49, kinuko wrote: > nit: I'd like to have one empty line after namespace { (unless it's for > one-liner or forward decls etc) > > (A few old-timer reviewers gave me this comment in my early days, so I'm > basically trying to keep the rule) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/62203007/530001
On 2013/12/06 18:58:44, alecflett wrote: > jam@ - TBR'ing you for new files added to content_*.gyp's lgtm
Retried try job too often on mac_rel for step(s) app_list_unittests, base_unittests, browser_tests, cacheinvalidation_unittests, cc_unittests, check_deps, chromedriver2_unittests, components_unittests, content_browsertests, content_unittests, crypto_unittests, google_apis_unittests, gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, message_center_unittests, nacl_integration, net_unittests, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/62203007/550001
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, cacheinvalidation_unittests, cc_unittests, check_deps, chromedriver2_unittests, components_unittests, content_browsertests, content_unittests, crypto_unittests, google_apis_unittests, gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, nacl_integration, net_unittests, ppapi_unittests, printing_unittests, remoting_unittests, sandbox_linux_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/62203007/570001
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alecflett@chromium.org/62203007/570001
Message was sent while issue was closed.
Change committed as 239380 |
