|
|
Created:
6 years, 4 months ago by gavinp Modified:
6 years, 3 months ago CC:
blink-reviews, michaeln, jsbell+serviceworker_chromium.org, jamesr, tzik, serviceworker-reviews, nhiroki, abarth-chromium, falken, dglazkov+blink, kinuko+serviceworker, horo+watch_chromium.org, alecflett+watch_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionIntroducing the WebServiceWorkerCache.
This CL provides the new WebServiceWorkerCache object, which connects the
ServiceWorker Cache between blink and content. There's two notable exceptions
in the implementation; the various Cache.add() and Cache.addAll() methods are
not implemented (they should do fetches in blink and provide the responses to
this API through the batch call), and Response bodies via blobs is not implemented.
R=dominicc@chromium.org,falken@chromium.org,jkarlin@chromium.org
BUG=399178
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182604
Patch Set 1 #Patch Set 2 : closer #Patch Set 3 : rename back to Cache, fill in most parts #Patch Set 4 : closer #Patch Set 5 : closer and narrower #Patch Set 6 : fix running with partial implementation #
Total comments: 42
Patch Set 7 : update for content side #Patch Set 8 : remediate #
Total comments: 6
Patch Set 9 : partial remediation #
Total comments: 27
Patch Set 10 : rebase #Patch Set 11 : rebase #Patch Set 12 : closer #
Total comments: 10
Patch Set 13 : rebase, some changes pulled out #
Total comments: 2
Patch Set 14 : rebase on top up new upstream CLs #Patch Set 15 : fix build #Patch Set 16 : add cacheName query param #Patch Set 17 : test isn't quite working #
Total comments: 3
Patch Set 18 : closer #Patch Set 19 : rebasing and fixing build #Patch Set 20 : for stitching use #Patch Set 21 : reviewable #Patch Set 22 : fix build #
Total comments: 22
Patch Set 23 : rebase only #Patch Set 24 : style remediation #Patch Set 25 : rebase on top of style nits cl #
Total comments: 1
Patch Set 26 : rebase, including jsbell's oilpan fixes #Patch Set 27 : improve and pass tests #
Total comments: 15
Patch Set 28 : remediate to dominicc review #
Total comments: 18
Patch Set 29 : remediate #Patch Set 30 : rebase to trunk #Patch Set 31 : narrow #Patch Set 32 : one last review comment #Patch Set 33 : remove crazy proxy thing #Patch Set 34 : rebase on top of NameToCacheMap impl #Patch Set 35 : cleaner and oilpan #Messages
Total messages: 55 (3 generated)
jkarlin, dominicc, falken: Please take a look over this for architecture; I'm not sending it out for review until the next CL in the series, the content IPCs, is closer for review, but I would really appreciate an early look now.
On 2014/08/08 20:34:40, gavinp wrote: > jkarlin, dominicc, falken: Please take a look over this for architecture; I'm > not sending it out for review until the next CL in the series, the content IPCs, > is closer for review, but I would really appreciate an early look now. One more note: dominicc, falken, do you think you'd like to take on implementing ::add() and ::addAll() ?
Comments inline. https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:122: // This management mechanism insures that we get the same Cache* object for every WebServiceWorkerCache object, so the ensures https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:127: return adoptRefWillBeNoop(new Cache(webCache)); I'd add Cache::create; it's typical Blink, more readable here. https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:140: // FIXME: We should throw the caught error. Why? I think the spec just says to reject the Promise: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#cache-match Ditto below. https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:281: m_webCache->setProxyInterface(this); Who owns m_webCache? https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:302: m_webCache->dispatchMatch(new CacheMatchCallbacks(resolver), webRequest, queryParamsFromDictionary(queryParamsDict)); Match all? Since the signatures are the same does it make sense to have a parameter for the cardinality? https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... File Source/modules/serviceworkers/Request.cpp (right): https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Request.cpp:284: // FIXME: Handle isReload correctly. Can you hint more here? Is this doable right now but not done to keep the change a sane size, or going to be fixed in this patch and just a WIP placeholder, or needs to wait until XXX? https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... File Source/modules/serviceworkers/Request.h (right): https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Request.h:6: No. https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... File Source/modules/serviceworkers/Response.cpp (right): https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Response.cpp:36: TrackExceptionState exceptionState; Why does this track exceptions? Seems rather voodoo. Where is JavaScript here? https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Response.cpp:217: // FIXME: Handle response body data. Where do things like opacity, etc. flow through? https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... File Source/modules/serviceworkers/Response.h (right): https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Response.h:35: Maybe tighten up the vertical whitespace here? https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:18: // An interface to the Cache API, implemented by the embedder and passed in to blink. Blink's implementation blink -> Blink; below too. I would only write blink when you intend to refer specifically to the namespace. https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:18: // An interface to the Cache API, implemented by the embedder and passed in to blink. Blink's implementation Can this be tightened up? "An interface to the Cache Application Programming Interface" seems duplicative; what about: The Service Worker Cache API. The embedder provides the implementation of the Cache to Blink. Blink uses this interface to operate on stores and entries. (I think the bit about callbacks is self evident from good naming below.) https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:29: class ProxyInterface { Not sure about naming this ...Interface; what's the thinking here? https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:31: virtual inline ~ProxyInterface() = 0; I guess this should be abstract by virtue of this being public/ ... +1 https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:36: QueryParams() : ignoreSearch(false), ignoreMethod(false), ignoreVary(false), prefixMatch(false) { } Regardless of how the spec has it, would it not be less unreadable to make these positive ("includeSearch" or something) to avoid double negatives? And do the mapping from spec-ese (which probably wants to use false as defaults for reasons unrelated to C++, perhaps because false is similarly falsy to a missing value in JavaScript) at a relatively early point in Blink? https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:63: void setProxyInterface(ProxyInterface* proxyInterface) { m_proxyInterface = proxyInterface; } Classes here usually fall into one of four categories: 1. Ones implemented by the embedder. 2. Ones implemented by Blink and exported to the embedder. 3. "Primitive" types like WebURL that are a bit of both (inline methods; INSIDE_BLINK ifdefs; knows about GURL in the embedder.) 4. Ones past the limit of Dominic's knowledge. The first type (which this is) tend strongly to have virtual methods, pure virtual or with empty/default value returning bodies for two-sided patch landiness, and no storage. This should hew to that. https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:74: virtual void notifyDone() = 0; This method has a very generic name for a very specific purpose. https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:80: // The pure virtual destructor makes the interface abstract, but we still want an inline destructor for descendents to call. Thus we If it's mysterious enough to warrant a comment, is it worth doing?
https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:122: // This management mechanism insures that we get the same Cache* object for every WebServiceWorkerCache object, so the On 2014/08/11 01:40:41, dominicc wrote: > ensures Done. https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:127: return adoptRefWillBeNoop(new Cache(webCache)); On 2014/08/11 01:40:41, dominicc wrote: > I'd add Cache::create; it's typical Blink, more readable here. Done. https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:281: m_webCache->setProxyInterface(this); On 2014/08/11 01:40:41, dominicc wrote: > Who owns m_webCache? I've cleared this up a lot in the newest upload; I wasn't totally sure earlier. It's owned by the Cache object. There's some comments in the WebServiceWorker* files, do you think those are adequate? https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:302: m_webCache->dispatchMatch(new CacheMatchCallbacks(resolver), webRequest, queryParamsFromDictionary(queryParamsDict)); On 2014/08/11 01:40:41, dominicc wrote: > Match all? > > Since the signatures are the same does it make sense to have a parameter for the > cardinality? The return value is different, although kind of trivially (a Response vs an array of them). But so many of these functions have nearly identical signatures; I don't like combining them where behaviour is different, because the untwisting on the other end gets complicated. For instance, Match is a lower latency function than MatchAll, since the underlying implementation doesn't have to iterate through all possible matches once it has one. So that's pretty different. Reasonable? https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... File Source/modules/serviceworkers/Request.cpp (right): https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Request.cpp:284: // FIXME: Handle isReload correctly. On 2014/08/11 01:40:41, dominicc wrote: > Can you hint more here? Is this doable right now but not done to keep the change > a sane size, or going to be fixed in this patch and just a WIP placeholder, or > needs to wait until XXX? Done. https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... File Source/modules/serviceworkers/Request.h (right): https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Request.h:6: On 2014/08/11 01:40:41, dominicc wrote: > No. No! https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... File Source/modules/serviceworkers/Response.cpp (right): https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Response.cpp:36: TrackExceptionState exceptionState; On 2014/08/11 01:40:41, dominicc wrote: > Why does this track exceptions? Seems rather voodoo. Where is JavaScript here? I have ABSOLUTELY NO IDEA. I was shocked that this function on headers requires this argument. Horo-san, what do you say? https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Response.cpp:217: // FIXME: Handle response body data. On 2014/08/11 01:40:41, dominicc wrote: > Where do things like opacity, etc. flow through? Initially that won't be handled; this CL tries to avoid widening the abstractions on the interface up to Chrome more than necessary to implement the API. We'll need to follow up and implement these. There's similar issues around referrer policies and a few other things like this. Would it be so crazy to stop having service_worker_types.h for Response/Request, and instead make our WebServiceWorkerFOO objects be POD enough that we can use them in messages? All these needless conversions make me sad, and worse yet questions like this about capabilities that have to do with all of these different data types make me even more sad. https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... File Source/modules/serviceworkers/Response.h (right): https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Response.h:35: On 2014/08/11 01:40:41, dominicc wrote: > Maybe tighten up the vertical whitespace here? Done. https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:18: // An interface to the Cache API, implemented by the embedder and passed in to blink. Blink's implementation On 2014/08/11 01:40:41, dominicc wrote: > blink -> Blink; below too. > > I would only write blink when you intend to refer specifically to the namespace. Done. https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:18: // An interface to the Cache API, implemented by the embedder and passed in to blink. Blink's implementation On 2014/08/11 01:40:42, dominicc wrote: > Can this be tightened up? "An interface to the Cache Application Programming > Interface" seems duplicative; what about: > > The Service Worker Cache API. The embedder provides the implementation of the > Cache to Blink. Blink uses this interface to operate on stores and entries. > > (I think the bit about callbacks is self evident from good naming below.) Done. https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:31: virtual inline ~ProxyInterface() = 0; On 2014/08/11 01:40:42, dominicc wrote: > I guess this should be abstract by virtue of this being public/ ... +1 It should be abstract in order to guard against instantiating this totally useless class; only a descendent of this class is reasonable, and abstract is the concept that protects against instantiating meaningless classes. Having the destructor be public is moot, I just put that in as boilerplate... In this case I don't think visibility matters for the destructor. Would removing the boilerplate help? https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:36: QueryParams() : ignoreSearch(false), ignoreMethod(false), ignoreVary(false), prefixMatch(false) { } On 2014/08/11 01:40:42, dominicc wrote: > Regardless of how the spec has it, would it not be less unreadable to make these > positive ("includeSearch" or something) to avoid double negatives? And do the > mapping from spec-ese (which probably wants to use false as defaults for reasons > unrelated to C++, perhaps because false is similarly falsy to a missing value in > JavaScript) at a relatively early point in Blink? I like this suggestion. I'll follow up with an upload that does this shortly, but the current upload lacks this. https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:63: void setProxyInterface(ProxyInterface* proxyInterface) { m_proxyInterface = proxyInterface; } On 2014/08/11 01:40:42, dominicc wrote: > Classes here usually fall into one of four categories: > > 1. Ones implemented by the embedder. > > 2. Ones implemented by Blink and exported to the embedder. > > 3. "Primitive" types like WebURL that are a bit of both (inline methods; > INSIDE_BLINK ifdefs; knows about GURL in the embedder.) > > 4. Ones past the limit of Dominic's knowledge. > > The first type (which this is) tend strongly to have virtual methods, pure > virtual or with empty/default value returning bodies for two-sided patch > landiness, and no storage. > > This should hew to that. I'm going to set up a VC for us to talk about this; I wonder if the proxy interface stuff should really be implemented in the embedder? https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:74: virtual void notifyDone() = 0; On 2014/08/11 01:40:42, dominicc wrote: > This method has a very generic name for a very specific purpose. And it's gone in the latest upload, as I cleared up ownership. https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:80: // The pure virtual destructor makes the interface abstract, but we still want an inline destructor for descendents to call. Thus we On 2014/08/11 01:40:42, dominicc wrote: > If it's mysterious enough to warrant a comment, is it worth doing? I don't know. I added the comment because the last time I created one of these types of method free abstract classes, the same question came up. Adding a constructor to the class seems to complicate the interface and make writing descendents harder; although I guess something similar can be done by just adding a do-nothing protected constructor. Of course, now that requires that descendents declare a constructor they don't need. So I'm beat. I'll do whatever the reviewers say here, but my intuition is that the pure virtual destructor with a body (that is in this case also inline by keyword) is the right call.... But I'm far from sure.
More comments inline. I just looked at your responses and the diffs to patchset 6, after we talk I'll take a look at the whole thing again. https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:140: // FIXME: We should throw the caught error. On 2014/08/11 01:40:41, dominicc wrote: > Why? I think the spec just says to reject the Promise: > > https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#cache-match > > Ditto below. Ping. https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:281: m_webCache->setProxyInterface(this); On 2014/08/13 23:25:22, gavinp wrote: > On 2014/08/11 01:40:41, dominicc wrote: > > Who owns m_webCache? > > I've cleared this up a lot in the newest upload; I wasn't totally sure earlier. > It's owned by the Cache object. There's some comments in the WebServiceWorker* > files, do you think those are adequate? I commented over there first. To me, the embedder = Chrome, so that comment doesn't jive with this; it is Blink that owns the WebServiceWorkerCache and not the embedder. How do caches get removed? What happens if one page is holding onto a Cache when that happens from another page? https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:302: m_webCache->dispatchMatch(new CacheMatchCallbacks(resolver), webRequest, queryParamsFromDictionary(queryParamsDict)); On 2014/08/13 23:25:22, gavinp wrote: > On 2014/08/11 01:40:41, dominicc wrote: > > Match all? > > > > Since the signatures are the same does it make sense to have a parameter for > the > > cardinality? > > The return value is different, although kind of trivially (a Response vs an > array of them). But so many of these functions have nearly identical signatures; > I don't like combining them where behaviour is different, because the untwisting > on the other end gets complicated. > > For instance, Match is a lower latency function than MatchAll, since the > underlying implementation doesn't have to iterate through all possible matches > once it has one. So that's pretty different. > > Reasonable? +1, yes, separate is simpler; simpler is better. If the return types are different presumably mixing up the methods will usually result in compile errors relating to the return type. Except when that return type is some untyped container like a Promise. Seriously though... this method is called match*All*Impl yet it dispatches just "match". What's up with that? I'm having a hard time seeing which characters differ from matchImpl directly above this one. https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:80: // The pure virtual destructor makes the interface abstract, but we still want an inline destructor for descendents to call. Thus we On 2014/08/13 23:25:23, gavinp wrote: > On 2014/08/11 01:40:42, dominicc wrote: > > If it's mysterious enough to warrant a comment, is it worth doing? > > I don't know. I added the comment because the last time I created one of these > types of method free abstract classes, the same question came up. > > Adding a constructor to the class seems to complicate the interface and make > writing descendents harder; although I guess something similar can be done by > just adding a do-nothing protected constructor. Of course, now that requires > that descendents declare a constructor they don't need. > > So I'm beat. I'll do whatever the reviewers say here, but my intuition is that > the pure virtual destructor with a body (that is in this case also inline by > keyword) is the right call.... But I'm far from sure. I'm fine with the inline virtual destructor either way. In the end a level 27 C++ Priest will come to kill you, and a level 29 C++ Priestess will come to kill *that* priest. Good luck! https://codereview.chromium.org/433793002/diff/140001/public/platform/WebServ... File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/140001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:75: // The Cache is owned by the embedder, and should not be deleted from within Blink; instead use notifyDone(), above. Ah... can you clarify what you mean by "the cache" here? I took it to mean the blink::Cache instance, but that is not right. https://codereview.chromium.org/433793002/diff/140001/public/platform/WebServ... File public/platform/WebServiceWorkerCacheStorage.h (right): https://codereview.chromium.org/433793002/diff/140001/public/platform/WebServ... public/platform/WebServiceWorkerCacheStorage.h:29: // Ownership of the CacheStorage*Callbacks methods passes to the WebServiceWorkerCacheStorage/ instance, which Remove the slash trailing Storage? https://codereview.chromium.org/433793002/diff/140001/public/platform/WebServ... public/platform/WebServiceWorkerCacheStorage.h:34: // the same WebServiceWorkerCache* object if the user opens the same Cache again. Can I see the Chromium side of this?
https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:140: // FIXME: We should throw the caught error. On 2014/08/14 02:35:12, dominicc wrote: > On 2014/08/11 01:40:41, dominicc wrote: > > Why? I think the spec just says to reject the Promise: > > > > https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#cache-match > > > > Ditto below. > > Ping. Yup, you're right. This is pretty trickyï¼› matchAll() actually behaves differently. The [[QueryCache]] algorithm says to rethrow exceptions if the promise constructor throws, see (4) in https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#query-cache-... . However, in the case of match, https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#cache-match , (3) says to reject with the exception. Looking at matchAll() through, there's no discussion of catching and rejecting, so I think we have to rethrow. Likewise for delete(), keys(), put(). This is pretty weird. I'll ping Jake to find out how intentional it is. I'll do this in a later upload, my next upload just deals with the easy stuff. https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:281: m_webCache->setProxyInterface(this); On 2014/08/14 02:35:12, dominicc wrote: > On 2014/08/13 23:25:22, gavinp wrote: > > On 2014/08/11 01:40:41, dominicc wrote: > > > Who owns m_webCache? > > > > I've cleared this up a lot in the newest upload; I wasn't totally sure > earlier. > > It's owned by the Cache object. There's some comments in the WebServiceWorker* > > files, do you think those are adequate? > > I commented over there first. > > To me, the embedder = Chrome, so that comment doesn't jive with this; it is > Blink that owns the WebServiceWorkerCache and not the embedder. > > How do caches get removed? What happens if one page is holding onto a Cache when > that happens from another page? Yeah, that comment was bad, it's gone now. So regarding caches and multiple pages, we're fine. Each service worker is in its own service worker scope, so that's a unique arena for these cache objects. I think we'll be fine if we add IPC on a per-scope basis announcing the end of using a cache in that scope, which is called from the destructor. WDYT? https://codereview.chromium.org/433793002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:302: m_webCache->dispatchMatch(new CacheMatchCallbacks(resolver), webRequest, queryParamsFromDictionary(queryParamsDict)); On 2014/08/14 02:35:12, dominicc wrote: > On 2014/08/13 23:25:22, gavinp wrote: > > On 2014/08/11 01:40:41, dominicc wrote: > > > Match all? > > > > > > Since the signatures are the same does it make sense to have a parameter for > > the > > > cardinality? > > > > The return value is different, although kind of trivially (a Response vs an > > array of them). But so many of these functions have nearly identical > signatures; > > I don't like combining them where behaviour is different, because the > untwisting > > on the other end gets complicated. > > > > For instance, Match is a lower latency function than MatchAll, since the > > underlying implementation doesn't have to iterate through all possible matches > > once it has one. So that's pretty different. > > > > Reasonable? > > +1, yes, separate is simpler; simpler is better. > > If the return types are different presumably mixing up the methods will usually > result in compile errors relating to the return type. > > Except when that return type is some untyped container like a Promise. > > Seriously though... this method is called match*All*Impl yet it dispatches just > "match". What's up with that? I'm having a hard time seeing which characters > differ from matchImpl directly above this one. That was a cut and paste mistake... I have now fixed it. Thanks. And, yeah, these promise returns make static typing very confusing. https://codereview.chromium.org/433793002/diff/140001/public/platform/WebServ... File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/140001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:75: // The Cache is owned by the embedder, and should not be deleted from within Blink; instead use notifyDone(), above. On 2014/08/14 02:35:12, dominicc wrote: > Ah... can you clarify what you mean by "the cache" here? I took it to mean the > blink::Cache instance, but that is not right. This comment is vestigial. Removing it. https://codereview.chromium.org/433793002/diff/140001/public/platform/WebServ... File public/platform/WebServiceWorkerCacheStorage.h (right): https://codereview.chromium.org/433793002/diff/140001/public/platform/WebServ... public/platform/WebServiceWorkerCacheStorage.h:29: // Ownership of the CacheStorage*Callbacks methods passes to the WebServiceWorkerCacheStorage/ instance, which On 2014/08/14 02:35:12, dominicc wrote: > Remove the slash trailing Storage? Done. https://codereview.chromium.org/433793002/diff/140001/public/platform/WebServ... public/platform/WebServiceWorkerCacheStorage.h:34: // the same WebServiceWorkerCache* object if the user opens the same Cache again. On 2014/08/14 02:35:12, dominicc wrote: > Can I see the Chromium side of this? Yes, please: https://codereview.chromium.org/474593002/
More comments inline. Going to look at the Chrome side next. I think once you add tests to this the patch may get unmanageably large; can you plan to break it down? https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:19: const char* cacheErrorToString(WebServiceWorkerCacheError reason) Why do all of this in the blank namespace? https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:21: // FIXME: Construct correct DOM error objects rather than returning strings. Do this. The spec says error objects and not strings, right? https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:53: m_resolver->resolve(Response::create(*webResponse)); Why not reset the pointer after this? It will detect a potential bug--multiple resolutions--earlier. Also might ameliorate the leak if CacheMatchCallbacks is leaked. Ditto below. https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:62: const RefPtr<ScriptPromiseResolver> m_resolver; Why const? https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:122: // This management mechanism ensures that we get the same Cache* object for every WebServiceWorkerCache object, so the Can this be tightened up? Something like: Ensure that we get the same Cache* for the same WebServiceWorkerCache object, to make == work in JavaScript. The raw pointer back to the blink::Cache is reset in the destructor. https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:124: // in our destructor we call WebServiceWorkerCache::notifyDone(), registering our destruction. I'm worried about how the object equality works. Aren't caches per-origin? What happens when one cache is exposed to two different workers in the same renderer? https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:130: Cache::~Cache() Line 124 suggests this calls notifyDone; what am I missing? https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:139: // FIXME: We should throw the caught error. Why should you throw it? I don't see anywhere it says to do this... https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:140: return ScriptPromise::reject(scriptState, V8ThrowException::createTypeError(exceptionState.message(), scriptState->isolate())); Why type error? https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... File Source/modules/serviceworkers/Response.cpp (right): https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Response.cpp:22: if (webResponse.status() < 200 || webResponse.status() >= 300) What about turning the cases around and doing if (200 <= webResponse.status() && webResponse.status() < 300) ? It's more evocative of a range. Helper method with descriptive name could also work. https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Response.cpp:36: TrackExceptionState exceptionState; Can you tell me how this runs JavaScript? I really want to know if there's the potential to run user code here or what is going on. https://codereview.chromium.org/433793002/diff/160001/public/platform/WebServ... File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/160001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:29: // The ProxyInterface is used to allow a single object in Blink to serve as its interface in JavaScript; so that == will I feel like there's redundancy here with the bit about ==. Maybe this description should just advance the story instead of repeating it; what does Blink use this for, at a high level? https://codereview.chromium.org/433793002/diff/160001/public/platform/WebServ... File public/platform/WebServiceWorkerCacheStorage.h (right): https://codereview.chromium.org/433793002/diff/160001/public/platform/WebServ... public/platform/WebServiceWorkerCacheStorage.h:34: // the same WebServiceWorkerCache* object if the user opens the same Cache again. This seems fraught; how is the ownership transferred from (random place in the V8 heap, where a wrapper is holding onto the cache) to here if the same cache is returned twice? If any JavaScript runs, or in future Oilpan GC happens, we could be in trouble. In fact, how do we *ever* release a Cache if the same object can be returned again later?
On 2014/08/15 06:20:24, dominicc wrote: > More comments inline. > > Going to look at the Chrome side next. > > I think once you add tests to this the patch may get unmanageably large; can you > plan to break it down? > Hmm, I'd love to hear advice on adding tests; do you have sample CLs that test objects this embedded in the ServiceWorker? I've been holding out hope we'd test this using the integrated tests being made by asanka. I have difficulty imagining this split up; perhaps we could split the WebServiceWorkerCache.h introduction from wiring it up, but that feels like a copout since the two patches are not particularly separable and don't have distinct reviewers.
This should be a lot closer. **IMPORTANT**: I have no idea how to create a DOM exception and pass it in. I did something that I think is crazy, just so you could see it, in Cache.cpp. I look forward to expert guidance. https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:19: const char* cacheErrorToString(WebServiceWorkerCacheError reason) On 2014/08/15 06:20:23, dominicc wrote: > Why do all of this in the blank namespace? It really should be moved out to a util module, together with fixing the code in CacheStorage. See comment below. https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:21: // FIXME: Construct correct DOM error objects rather than returning strings. On 2014/08/15 06:20:23, dominicc wrote: > Do this. The spec says error objects and not strings, right? This is a copy of a method currently used in CacheStorage.cpp; so that redundancy should probably be addressed as well at the same time as fixing this. Rather than bloat this CL I'd rather land this in a downstream. https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:53: m_resolver->resolve(Response::create(*webResponse)); On 2014/08/15 06:20:23, dominicc wrote: > Why not reset the pointer after this? It will detect a potential bug--multiple > resolutions--earlier. Also might ameliorate the leak if CacheMatchCallbacks is > leaked. Ditto below. Good idea. https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:62: const RefPtr<ScriptPromiseResolver> m_resolver; On 2014/08/15 06:20:23, dominicc wrote: > Why const? Up until we reset, it was never modified during the object lifetime, so const seemed a good way to convey it. But with the reset, it's not const any more, so this is moot. https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:122: // This management mechanism ensures that we get the same Cache* object for every WebServiceWorkerCache object, so the On 2014/08/15 06:20:23, dominicc wrote: > Can this be tightened up? Something like: > > Ensure that we get the same Cache* for the same WebServiceWorkerCache object, to > make == work in JavaScript. The raw pointer back to the blink::Cache is reset in > the destructor. I've just removed this comment, it was recapitulating the discussion in the interface. https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:124: // in our destructor we call WebServiceWorkerCache::notifyDone(), registering our destruction. On 2014/08/15 06:20:23, dominicc wrote: > I'm worried about how the object equality works. > > Aren't caches per-origin? What happens when one cache is exposed to two > different workers in the same renderer? It turns out, in the content side version of this CL, we're actually quite careful to only have one WebServiceWorkerCache object per cache_id, but that's on a _per ServiceWorker_ basis, so two different ServiceWorkers in the same renderer will have difference WebServiceWorkerCache objects and thus different Cache objects, so we're safe. https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:130: Cache::~Cache() On 2014/08/15 06:20:23, dominicc wrote: > Line 124 suggests this calls notifyDone; what am I missing? Vestigal comment, removed. It was just a recapitulation anyway. There's no more notifyDone() since we use the destructor. https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:139: // FIXME: We should throw the caught error. On 2014/08/15 06:20:23, dominicc wrote: > Why should you throw it? I don't see anywhere it says to do this... Good catch, that's true for match(). Almost all of the other functions on Cache do throw exceptions... https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:140: return ScriptPromise::reject(scriptState, V8ThrowException::createTypeError(exceptionState.message(), scriptState->isolate())); On 2014/08/15 06:20:23, dominicc wrote: > Why type error? Earlier revision of the spec perhaps? Now fixed. https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... File Source/modules/serviceworkers/Response.cpp (right): https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Response.cpp:22: if (webResponse.status() < 200 || webResponse.status() >= 300) On 2014/08/15 06:20:24, dominicc wrote: > What about turning the cases around and doing > > if (200 <= webResponse.status() && webResponse.status() < 300) > > ? > > It's more evocative of a range. Helper method with descriptive name could also > work. Done. https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Response.cpp:36: TrackExceptionState exceptionState; On 2014/08/15 06:20:24, dominicc wrote: > Can you tell me how this runs JavaScript? I really want to know if there's the > potential to run user code here or what is going on. I wish I understood too. Horo-san, can you fill us in? Why does Headers::append() take an exception state? This is the method to add headers, and I have no idea why it needs this. https://codereview.chromium.org/433793002/diff/160001/public/platform/WebServ... File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/160001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:29: // The ProxyInterface is used to allow a single object in Blink to serve as its interface in JavaScript; so that == will On 2014/08/15 06:20:24, dominicc wrote: > I feel like there's redundancy here with the bit about ==. Maybe this > description should just advance the story instead of repeating it; what does > Blink use this for, at a high level? How do you like that rewrite? https://codereview.chromium.org/433793002/diff/160001/public/platform/WebServ... File public/platform/WebServiceWorkerCacheStorage.h (right): https://codereview.chromium.org/433793002/diff/160001/public/platform/WebServ... public/platform/WebServiceWorkerCacheStorage.h:34: // the same WebServiceWorkerCache* object if the user opens the same Cache again. On 2014/08/15 06:20:24, dominicc wrote: > This seems fraught; how is the ownership transferred from (random place in the > V8 heap, where a wrapper is holding onto the cache) to here if the same cache is > returned twice? > > If any JavaScript runs, or in future Oilpan GC happens, we could be in trouble. > > In fact, how do we *ever* release a Cache if the same object can be returned > again later? I think we spoke about this offline: if the blink Cache object is deleted, it destroys its corresponding WebServiceWorkerCache object. We carefully insure that one, and only one WebServiceWorkerCache object exists for every scope, so this doesn't cause double deletes.
On 2014/08/19 21:09:37, gavinp wrote: > This should be a lot closer. > > **IMPORTANT**: I have no idea how to create a DOM exception and pass it in. I > did something that I think is crazy, just so you could see it, in Cache.cpp. I > look forward to expert guidance. Take a peek in ServiceWorkerContainer.cpp for examples: e.g. m_resolver->reject(DOMException::create(InvalidStateError, "No associated provider is available")); The model in ServiceWorkerError.h/cpp may be worth copying as well (map Web*Error types to DOMExceptions) so you can unify the onError() handlers.
https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... File Source/modules/serviceworkers/Response.cpp (right): https://codereview.chromium.org/433793002/diff/160001/Source/modules/servicew... Source/modules/serviceworkers/Response.cpp:36: TrackExceptionState exceptionState; On 2014/08/19 21:09:37, gavinp wrote: > On 2014/08/15 06:20:24, dominicc wrote: > > Can you tell me how this runs JavaScript? I really want to know if there's the > > potential to run user code here or what is going on. > > I wish I understood too. Horo-san, can you fill us in? Why does > Headers::append() take an exception state? Per Fetch spec, some calls throw (e.g. empty names). Since that's web-exposed and defined in prose not IDL, it's outside the scope of bindings and handled in the impl. > This is the method to add headers, > and I have no idea why it needs this. TrackExceptionState is just a helper that ignores exceptions. e.g. if the WebResponse data included an invalid header name. Alternately, this could be NonThrowableExceptionState which would ASSERT if append() throws. I assume WebResponse can include headers that user code is not allowed to set; if not, it should use NonThrowableExceptionState.
On 2014/08/20 00:13:52, jsbell wrote: > On 2014/08/19 21:09:37, gavinp wrote: > > This should be a lot closer. > > > > **IMPORTANT**: I have no idea how to create a DOM exception and pass it in. I > > did something that I think is crazy, just so you could see it, in Cache.cpp. I > > look forward to expert guidance. > > Take a peek in ServiceWorkerContainer.cpp for examples: e.g. > m_resolver->reject(DOMException::create(InvalidStateError, "No associated > provider is available")); I was doing that, and it wasn't compiling. I'll go hunting for the header that gets me the right implicit type conversion then... > The model in ServiceWorkerError.h/cpp may be worth copying as well (map > Web*Error types to DOMExceptions) so you can unify the onError() handlers. I'll peek, thank you.
I think this patch is unnecessarily large and could be broken up. The size makes it a bit hard to review. I think I found another problem similar to dispatchMatch/dispatchMatchAll confusion, noted inline, but these easily hide because of the size of the patch and the lack of tests. You might look at Source/modules/serviceworkers/ServiceWorkerContainerTest.cpp for an example test at this layer. It is certainly possible to test. https://codereview.chromium.org/433793002/diff/240001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/240001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:22: // FIXME: Construct correct DOM error objects rather than returning strings. One way to avoid the problem with cut-and-paste and obviate the FIXME is to refactor the place this was copied from in a separate CL and then make this CL be its second caller. https://codereview.chromium.org/433793002/diff/240001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:39: DictionaryHelper::get(dictionary, "ignoreSearch", queryParams.ignoreSearch); Where did we end up with the idea of making these positive, like includeSearch, to avoid negation, double negatives in boolean expressions that use these fields? https://codereview.chromium.org/433793002/diff/240001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:43: // FIXME: Add cacheName. Given that these other parameters are not hooked up, why omit cacheName in particular? https://codereview.chromium.org/433793002/diff/240001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:140: TrackExceptionState exceptionState; I think you can extract two helpers, one for String and another for Request, that does the first four lines of this function and the many like it below. https://codereview.chromium.org/433793002/diff/240001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:143: return ScriptPromise::reject(scriptState, DOMException::create(NotFoundError, "The specified Service Worker resource was not found.")->newLocalWrapper(scriptState->isolate())); rejectWithDOMException ditto below https://codereview.chromium.org/433793002/diff/240001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:161: // FIXME: We should throw the caught error. Just do it? Ditto below. https://codereview.chromium.org/433793002/diff/240001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:251: return keysImpl(scriptState, RefPtrWillBeRawPtr<Request>(), Dictionary()); If you have keysImpl(ScriptState) as line 262 indicates, is this method really pulling its weight? Just inline keysImpl(ScriptState*) here and call this. https://codereview.chromium.org/433793002/diff/240001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:356: WebServiceWorkerRequest webRequest; Why go to the trouble of creating the WebRequest and not using it? Is the 0 in line 361 in error? Similarly, there's a helper to schlep a Dictionary to QueryParams; why pass empty QueryParams? https://codereview.chromium.org/433793002/diff/240001/public/platform/WebServ... File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/240001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:29: // Blink can store its internal interface to this object as a ProxyInterface in the WebServiceWorkerCache. This allows I think this comment and the API would be better if it avoided "interface". Really this isn't an interface to anything; it is a relatively opaque way for Blink to stash and retrieve some stuff. Similarly, what is it a Proxy to? Maybe the concrete implementation is a proxy to something, but that's Blink's business. https://codereview.chromium.org/433793002/diff/240001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:30: // blink to quickly find the correct internal interface when it is passed the same WebServiceWorkerCache object. blink -> Blink
I'm proceeding to carve this up as much as possible; a few upstream changes have already landed. I'll carve those CLs up and review them, then bring this back to life once it's narrower and smaller. Hopefully testing catches up with it too. https://codereview.chromium.org/433793002/diff/260001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/260001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:144: // return ScriptPromise::reject(DOMException::create(NotFoundError, "The specified Service Worker resource was not found.")); Sadly, this doesn't work because the type coercion for ScriptPromise::reject isn't as fancy as for m_promise->reject(). I'm fixing that upstream so we can land code like this.
https://codereview.chromium.org/433793002/diff/260001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/260001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:144: // return ScriptPromise::reject(DOMException::create(NotFoundError, "The specified Service Worker resource was not found.")); On 2014/08/28 13:39:36, gavinp wrote: > Sadly, this doesn't work because the type coercion for ScriptPromise::reject > isn't as fancy as for m_promise->reject(). I'm fixing that upstream so we can > land code like this. The bindings code automagically converts throws exceptions to rejections if the return type of the method is Promise, so you should be able to do this by annotating the function with [RaisesException] and using exceptionState.throwDOMException().
On 2014/08/28 23:48:45, jsbell wrote: > The bindings code automagically converts throws exceptions to rejections if the > return type of the method is Promise, so you should be able to do this by > annotating the function with [RaisesException] and using > exceptionState.throwDOMException(). .... but apparently this isn't working as I expected, so ignore me for now.
On 2014/08/29 00:44:53, jsbell wrote: > .... but apparently this isn't working as I expected, so ignore me for now. or I managed to confuse myself with nested exceptions and rejections in a hacked up test. *sigh*. You can take a peek at: https://codereview.chromium.org/521513002
On 2014/08/29 00:53:42, jsbell wrote: > On 2014/08/29 00:44:53, jsbell wrote: > > .... but apparently this isn't working as I expected, so ignore me for now. > > or I managed to confuse myself with nested exceptions and rejections in a hacked > up test. *sigh*. > > You can take a peek at: > > https://codereview.chromium.org/521513002 Happy to report: that section of our code is a lot cleaner, and that without exceptions being thrown. Still not quite asking for this to enter another review cycle, but I'm narrowing it down on top of two new upstream CLs: The Request object stuff: https://codereview.chromium.org/516123004/ The Response stuff: https://codereview.chromium.org/519803002/
jsbell: i've cleaned this up quite a bit, but my last upload "test isn't quite working" shows a very intermediate state. The most basic test I can write crashes during any cache operation; as illustrated here. Do you have any wisdom that comes quickly here? I'll keep looking at this, I'd like to upload this patch with tests. https://codereview.chromium.org/433793002/diff/360001/Source/modules/servicew... File Source/modules/serviceworkers/CacheTest.cpp (right): https://codereview.chromium.org/433793002/diff/360001/Source/modules/servicew... Source/modules/serviceworkers/CacheTest.cpp:85: ScriptPromise blah = cache->match(scriptState(), url, Dictionary()); This line crashes. It's during construction of ScriptPromise::InternalResolver in ScriptPromiseResolver, which is interesting... Have I not baked up my fake objects well enough?
https://codereview.chromium.org/433793002/diff/360001/Source/modules/servicew... File Source/modules/serviceworkers/CacheTest.cpp (right): https://codereview.chromium.org/433793002/diff/360001/Source/modules/servicew... Source/modules/serviceworkers/CacheTest.cpp:85: ScriptPromise blah = cache->match(scriptState(), url, Dictionary()); On 2014/08/29 20:03:26, gavinp wrote: > This line crashes. It's during construction of ScriptPromise::InternalResolver > in ScriptPromiseResolver, which is interesting... Have I not baked up my fake > objects well enough? Toss ScriptState::Scope scriptScope(scriptState()); above this to scope the local script object allocations - with that fix I make it as far as ~TestWebCache
https://codereview.chromium.org/433793002/diff/360001/Source/modules/servicew... File Source/modules/serviceworkers/CacheTest.cpp (right): https://codereview.chromium.org/433793002/diff/360001/Source/modules/servicew... Source/modules/serviceworkers/CacheTest.cpp:85: ScriptPromise blah = cache->match(scriptState(), url, Dictionary()); On 2014/08/29 21:26:43, jsbell wrote: > On 2014/08/29 20:03:26, gavinp wrote: > > This line crashes. It's during construction of ScriptPromise::InternalResolver > > in ScriptPromiseResolver, which is interesting... Have I not baked up my fake > > objects well enough? > > Toss ScriptState::Scope scriptScope(scriptState()); above this to scope the > local script object allocations - with that fix I make it as far as > ~TestWebCache And I found it. Thanks Josh, I'll be uploading a review ready version of this shortly.
This latest upload should be much closer to what we finally want; I've carved out large chunks of the CL and landed them separately, and written an extensive test. Please take a look. I'll self review it concurrently, and go back to continue conversations from earlier reviews that are still relevant. Thanks!
style nits https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:39: CacheMatchCallbacks(PassRefPtr<ScriptPromiseResolver> resolver) : m_resolver(resolver) { } style: break before : per http://www.chromium.org/blink/coding-style#TOC-Line-breaking https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:60: CacheWithResponsesCallbacks(PassRefPtr<ScriptPromiseResolver> resolver) : m_resolver(resolver) { } style: break before : https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:84: CacheWithRequestsCallbacks(PassRefPtr<ScriptPromiseResolver> resolver) : m_resolver(resolver) { } style: break before : https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:243: Cache::Cache(WebServiceWorkerCache* webCache) : m_webCache(adoptPtr(webCache)) style: break before : https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:246: ASSERT(m_webCache->proxyInterface() == 0); nit: seems more common to ASSERT(!x) https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... File Source/modules/serviceworkers/CacheTest.cpp (right): https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/CacheTest.cpp:33: ErrorWebCacheForTests(const WebServiceWorkerCacheError error) : m_error(error), m_expectedUrl(0), m_expectedQueryParams(0), m_expectedBatchOperations(0) { } style: each initializer on separate line https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/CacheTest.cpp:146: NotImplementedErrorCache() : ErrorWebCacheForTests(WebServiceWorkerCacheErrorNotImplemented) { } style: wrap initializer https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/CacheTest.cpp:151: ServiceWorkerCacheTest() : m_page(DummyPageHolder::create(IntSize(1, 1))) { } style: wrap initializer
Few more nits. I've been distracted with meetings, so I can't page enough into my brain to say l*g*t*m yet but I didn't notice anything glaringly wrong. :) https://codereview.chromium.org/433793002/diff/460001/Source/modules/modules.... File Source/modules/modules.gypi (right): https://codereview.chromium.org/433793002/diff/460001/Source/modules/modules.... Source/modules/modules.gypi:1022: 'serviceworkers/RequestTest.cpp', not this patch, but can you alphabetize the {Request,Response}Test entries? https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... File Source/modules/serviceworkers/CacheStorage.cpp (right): https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/CacheStorage.cpp:71: m_resolver->resolve(false); This makes caches.get("no such cache") resolve with `false` instead of `undefined` (caught by asanka's tests, BTW). Spec says `undefined` in https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ca... unless I'm reading it wrong. Is this intentional? Does this callback get used in different contexts where it needs to return false? https://codereview.chromium.org/433793002/diff/460001/public/platform/WebServ... File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/460001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:30: // blink to quickly find the correct internal interface when it is passed the same WebServiceWorkerCache object. nit: Blink
TY for your style nits Josh. https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:29: class ProxyInterface { On 2014/08/11 01:40:41, dominicc--slow until 9.9 wrote: > Not sure about naming this ...Interface; what's the thinking here? It holds the blink::Cache object, which must be an interface (at least that's what Cache.idl tells me...) and the Cache object is basically a proxy around WebServiceWorkerCache, since most calls to Cache::foo() end up being a single call into WebServiceWorkerCache. Any ideas for a better name? https://codereview.chromium.org/433793002/diff/460001/Source/modules/modules.... File Source/modules/modules.gypi (right): https://codereview.chromium.org/433793002/diff/460001/Source/modules/modules.... Source/modules/modules.gypi:1022: 'serviceworkers/RequestTest.cpp', On 2014/09/04 00:28:08, jsbell wrote: > not this patch, but can you alphabetize the {Request,Response}Test entries? I'll upload a cleanup patch that does this. https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:39: CacheMatchCallbacks(PassRefPtr<ScriptPromiseResolver> resolver) : m_resolver(resolver) { } On 2014/09/03 19:15:00, jsbell wrote: > style: break before : > > per http://www.chromium.org/blink/coding-style#TOC-Line-breaking Done. But isn't the rule you mean to quote http://www.chromium.org/blink/coding-style#TOC-Other-Punctuation instead? https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:60: CacheWithResponsesCallbacks(PassRefPtr<ScriptPromiseResolver> resolver) : m_resolver(resolver) { } On 2014/09/03 19:15:00, jsbell wrote: > style: break before : Done. https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:84: CacheWithRequestsCallbacks(PassRefPtr<ScriptPromiseResolver> resolver) : m_resolver(resolver) { } On 2014/09/03 19:15:01, jsbell wrote: > style: break before : Done. https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:243: Cache::Cache(WebServiceWorkerCache* webCache) : m_webCache(adoptPtr(webCache)) On 2014/09/03 19:15:00, jsbell wrote: > style: break before : Done. https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:243: Cache::Cache(WebServiceWorkerCache* webCache) : m_webCache(adoptPtr(webCache)) On 2014/09/03 19:15:00, jsbell wrote: > style: break before : Done. https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:246: ASSERT(m_webCache->proxyInterface() == 0); On 2014/09/03 19:15:00, jsbell wrote: > nit: seems more common to ASSERT(!x) Done. https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... File Source/modules/serviceworkers/CacheStorage.cpp (right): https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/CacheStorage.cpp:71: m_resolver->resolve(false); On 2014/09/04 00:28:08, jsbell wrote: > This makes caches.get("no such cache") resolve with `false` instead of > `undefined` (caught by asanka's tests, BTW). Spec says `undefined` in > https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ca... > unless I'm reading it wrong. > > Is this intentional? Does this callback get used in different contexts where it > needs to return false? > Not intentional. This was merge churn. https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... File Source/modules/serviceworkers/CacheTest.cpp (right): https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/CacheTest.cpp:33: ErrorWebCacheForTests(const WebServiceWorkerCacheError error) : m_error(error), m_expectedUrl(0), m_expectedQueryParams(0), m_expectedBatchOperations(0) { } On 2014/09/03 19:15:01, jsbell wrote: > style: each initializer on separate line Done. https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/CacheTest.cpp:146: NotImplementedErrorCache() : ErrorWebCacheForTests(WebServiceWorkerCacheErrorNotImplemented) { } On 2014/09/03 19:15:01, jsbell wrote: > style: wrap initializer Done. https://codereview.chromium.org/433793002/diff/460001/Source/modules/servicew... Source/modules/serviceworkers/CacheTest.cpp:151: ServiceWorkerCacheTest() : m_page(DummyPageHolder::create(IntSize(1, 1))) { } On 2014/09/03 19:15:01, jsbell wrote: > style: wrap initializer Done.
This will need significant rebasing due to (1) Oilpan being turned on for modules/serviceworker and (2) toNativeXXX changing to toImplXXX #1 is particularly nefarious since `const RefPtr<X>` and `const X*` don't mean the same thing. It looks like we can const-sprinkle in Request and Response in the cloning ctors and populateWebXXX functions, though. I'll try and get it working locally and upload to save gavin some time. https://codereview.chromium.org/433793002/diff/520001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/520001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:234: return keysImpl(scriptState); Needs to pass request and queryParamsDict
Also, the Cache needs to be GarbageCollectedFinalized since it has an OwnPtr member. Which results in an interesting crash: the finalization happens lazily, and when running layout tests is deferred long enough that by the time WebCache::~WebCache() runs and tries to clean up the IDMap, base::NonThreadSafeImpl::CalledOnValidThread() fails...
Yeah, the assumption (on the chromium side) that ServiceWorkerCacheStorageDispatcher will outlive WebCache objects is now invalid. We can neuter the WebCache objects (null out _dispatcher) by iterating the IDMap when the dispatcher goes away, or make the dispatcher refcounted. The former keeps ownership more reasonable, so I'll give it a shot.
On 2014/09/05 23:15:32, jsbell wrote: > Yeah, the assumption (on the chromium side) that > ServiceWorkerCacheStorageDispatcher will outlive WebCache objects is now > invalid. > > We can neuter the WebCache objects (null out _dispatcher) by iterating the IDMap > when the dispatcher goes away, or make the dispatcher refcounted. The former > keeps ownership more reasonable, so I'll give it a shot. Okay.. https://codereview.chromium.org/544413002 is a Chromium-side patch that needs be applied to https://codereview.chromium.org/474593002 https://codereview.chromium.org/546153002 is a Blink-side patch that needs to be applied to https://codereview.chromium.org/433793002/ (this patch) With those stacked up, asanka's CacheStorage tests pass (except for a known enumeration ordering issue)
On 2014/09/05 23:34:53, jsbell wrote: > On 2014/09/05 23:15:32, jsbell wrote: > > Yeah, the assumption (on the chromium side) that > > ServiceWorkerCacheStorageDispatcher will outlive WebCache objects is now > > invalid. > > > > We can neuter the WebCache objects (null out _dispatcher) by iterating the > IDMap > > when the dispatcher goes away, or make the dispatcher refcounted. The former > > keeps ownership more reasonable, so I'll give it a shot. > > Okay.. > > https://codereview.chromium.org/544413002 is a Chromium-side patch that needs be > applied to https://codereview.chromium.org/474593002 > > https://codereview.chromium.org/546153002 is a Blink-side patch that needs to be > applied to https://codereview.chromium.org/433793002/ (this patch) > > With those stacked up, asanka's CacheStorage tests pass (except for a known > enumeration ordering issue) Josh, Thanks for the oilpan fixes. This patch needed a good amount of rebasing since last upload because of quite a bit of surgery in Request* and Response* to implement the unified Body ancestor. As a result, the oilpan patches you wrote needed reworking too. Folks, can you please take a look at this?
> Folks, can you please take a look at this? Looking. FYI when running asanka's tests against this + the chromium side patch I get a crash coming out of V8, coming through CacheStorageWithCacheCallbacks::onError() - looks like the JS-side Promise was freed maybe?
On 2014/09/17 00:02:38, jsbell wrote: > > Folks, can you please take a look at this? > > Looking. FYI when running asanka's tests against this + the chromium side patch > I get a crash coming out of V8, coming through > CacheStorageWithCacheCallbacks::onError() - looks like the JS-side Promise was > freed maybe? More likely, an Error is coming through on a promise that was already resolved (was happening at the end of the tests, so likely during tear-down.) Another crash seen during tear-down: #13 base::NonThreadSafeImpl::CalledOnValidThread() #14 _ZN5IDMapIN7content35ServiceWorkerCacheStorageDispatcher8WebCacheEL23IDMapOwnershipSemantics0EE6RemoveEi #15 content::ServiceWorkerCacheStorageDispatcher::onWebCacheDestruction() #16 content::ServiceWorkerCacheStorageDispatcher::WebCache::~WebCache() #17 content::ServiceWorkerCacheStorageDispatcher::WebCache::~WebCache() #18 WTF::OwnedPtrDeleter<>::deletePtr() #19 WTF::OwnPtr<>::~OwnPtr() #20 blink::Cache::~Cache() #21 blink::GarbageCollectedFinalized<>::finalizeGarbageCollectedObject() #22 blink::FinalizerTraitImpl<>::finalize() #23 blink::FinalizerTrait<>::finalize() #24 blink::HeapObjectHeader::finalize() #25 blink::FinalizedHeapObjectHeader::finalize() #26 blink::HeapPage<>::finalize() #27 blink::HeapPage<>::sweep() #28 blink::ThreadHeap<>::sweepNormalPages() #29 blink::ThreadHeap<>::sweep() #30 blink::ThreadState::performPendingSweep() #31 blink::Heap::collectGarbageForTerminatingThread() #32 blink::ThreadState::cleanup() #33 blink::ThreadState::detach() #34 blink::WorkerThread::cleanup() Does the onWebCacheDestruction() call need to get proxied to the main thread?
Some comments inline. https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:130: Request* request = Request::create(scriptState->executionContext(), originalRequest, exceptionState); Why does this one have this V8 exception stuff here? Why does this exception map to "not found"? This part is a bit mysterious to me. https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.h (right): https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... Source/modules/serviceworkers/Cache.h:18: #include <v8.h> Why is this include needed? https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... Source/modules/serviceworkers/Cache.h:26: class WebServiceWorkerCache; This might be redundant with the new include. https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... File Source/modules/serviceworkers/CacheTest.cpp (right): https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... Source/modules/serviceworkers/CacheTest.cpp:51: // deep inspecting WebFoo objects for strict equality and test only for sanity, trusting the WebFoo test to catch lurking problems earlier. Lurking problems? Be specific (but keep it brief.) https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... Source/modules/serviceworkers/CacheTest.cpp:103: void CheckUrlIfProvided(const KURL& url) Blink style. https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... Source/modules/serviceworkers/CacheTest.cpp:205: class NullFunction : public ScriptFunction { This seems to do more than "null"... how about unreachable or failure function or something. https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... Source/modules/serviceworkers/CacheTest.cpp:222: class Function : public ScriptFunction { Likewise this could have a more precise name.
On 2014/09/17 00:17:11, jsbell wrote: > On 2014/09/17 00:02:38, jsbell wrote: > > > Folks, can you please take a look at this? > > > > Looking. FYI when running asanka's tests against this + the chromium side > patch > > I get a crash coming out of V8, coming through > > CacheStorageWithCacheCallbacks::onError() - looks like the JS-side Promise was > > freed maybe? > > More likely, an Error is coming through on a promise that was already resolved > (was happening at the end of the tests, so likely during tear-down.) > > Another crash seen during tear-down: > > #13 base::NonThreadSafeImpl::CalledOnValidThread() > #14 > _ZN5IDMapIN7content35ServiceWorkerCacheStorageDispatcher8WebCacheEL23IDMapOwnershipSemantics0EE6RemoveEi > #15 content::ServiceWorkerCacheStorageDispatcher::onWebCacheDestruction() > #16 content::ServiceWorkerCacheStorageDispatcher::WebCache::~WebCache() > #17 content::ServiceWorkerCacheStorageDispatcher::WebCache::~WebCache() > #18 WTF::OwnedPtrDeleter<>::deletePtr() > #19 WTF::OwnPtr<>::~OwnPtr() > #20 blink::Cache::~Cache() > #21 blink::GarbageCollectedFinalized<>::finalizeGarbageCollectedObject() > #22 blink::FinalizerTraitImpl<>::finalize() > #23 blink::FinalizerTrait<>::finalize() > #24 blink::HeapObjectHeader::finalize() > #25 blink::FinalizedHeapObjectHeader::finalize() > #26 blink::HeapPage<>::finalize() > #27 blink::HeapPage<>::sweep() > #28 blink::ThreadHeap<>::sweepNormalPages() > #29 blink::ThreadHeap<>::sweep() > #30 blink::ThreadState::performPendingSweep() > #31 blink::Heap::collectGarbageForTerminatingThread() > #32 blink::ThreadState::cleanup() > #33 blink::ThreadState::detach() > #34 blink::WorkerThread::cleanup() > > Does the onWebCacheDestruction() call need to get proxied to the main thread? The problem was lifetime issues around shutdown in the content:: implementation; the WebCache object could be deleted after the ServiceWorkerCacheStorageDispatcher, which was proxying for it. Practically this is fine; but we needed to use a WeakPtr in the WebCache object so we didn't end up calling in to the undead ServiceWorkerCacheStorageDispatcher object and as in the example stack above, using a pthread lock that is invalid (22 is EINVAL, probably the lock had already been destroyed in ~AutoLock(), hence that error). There's an upload over on that other CL that fixes this. Thanks for running the code with Asanka's tests and catching this! Asanka's tests now run without crashing, although Cache doesn't pass them (since there's no implementation backing it up yet).
TY everyone for your close looks. This is much cleaner now. https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:130: Request* request = Request::create(scriptState->executionContext(), originalRequest, exceptionState); On 2014/09/17 12:53:58, dominicc wrote: > Why does this one have this V8 exception stuff here? Why does this exception map > to "not found"? This part is a bit mysterious to me. These should be rethrown. NotFound is arbitrary; it's sort of unfortunate that we don't have a good way to extract an exception from TrackExceptionState to easily reject the promise with it. The spec is a bit convoluted about how it talks about rethrowing exceptions; in many cases it's explicitly addressed, in others it's implicit in that code for a promise that throws is supposed to reject with that promise. I could add, to each method, a comment from the spec (the Fetch code does that), but it would get pretty strange on methods like Cache::add() where the rule comes transitively from the fetch. We need to figure out how to get exceptions out of our trackers, or in the alternative, let the promise be an exception tracker of some kind? I have added a FIXME at each of these sites briefly saying what we should do. https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.h (right): https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... Source/modules/serviceworkers/Cache.h:18: #include <v8.h> On 2014/09/17 12:53:58, dominicc wrote: > Why is this include needed? It's been there from the start; likely for no good reason. Removing it. https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... Source/modules/serviceworkers/Cache.h:26: class WebServiceWorkerCache; On 2014/09/17 12:53:58, dominicc wrote: > This might be redundant with the new include. Done. https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... File Source/modules/serviceworkers/CacheTest.cpp (right): https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... Source/modules/serviceworkers/CacheTest.cpp:51: // deep inspecting WebFoo objects for strict equality and test only for sanity, trusting the WebFoo test to catch lurking problems earlier. On 2014/09/17 12:53:58, dominicc wrote: > Lurking problems? Be specific (but keep it brief.) Removed most of this language, it was more a note to me. The ResponseTest checks that responses can be copied; that's not the role of this test. https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... Source/modules/serviceworkers/CacheTest.cpp:103: void CheckUrlIfProvided(const KURL& url) On 2014/09/17 12:53:58, dominicc wrote: > Blink style. Done. https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... Source/modules/serviceworkers/CacheTest.cpp:205: class NullFunction : public ScriptFunction { On 2014/09/17 12:53:58, dominicc wrote: > This seems to do more than "null"... how about unreachable or failure function > or something. Done. https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... Source/modules/serviceworkers/CacheTest.cpp:222: class Function : public ScriptFunction { On 2014/09/17 12:53:58, dominicc wrote: > Likewise this could have a more precise name. I had a hard time doing better than "TestFunction", so that's what I used.
jsbell@chromium.org changed reviewers: + tkent@chromium.org
lgtm but I think we should do some refactoring after landing to bring this more in line with other code. tkent@ - OWNERS review for the public/platform bits? https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/560001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:130: Request* request = Request::create(scriptState->executionContext(), originalRequest, exceptionState); On 2014/09/17 15:41:52, gavinp wrote: > On 2014/09/17 12:53:58, dominicc wrote: > > Why does this one have this V8 exception stuff here? Why does this exception > map > > to "not found"? This part is a bit mysterious to me. > > These should be rethrown. NotFound is arbitrary; it's sort of unfortunate that > we don't have a good way to extract an exception from TrackExceptionState to > easily reject the promise with it. > > The spec is a bit convoluted about how it talks about rethrowing exceptions; in > many cases it's explicitly addressed, in others it's implicit in that code for a > promise that throws is supposed to reject with that promise. I could add, to > each method, a comment from the spec (the Fetch code does that), but it would > get pretty strange on methods like Cache::add() where the rule comes > transitively from the fetch. > > We need to figure out how to get exceptions out of our trackers, or in the > alternative, let the promise be an exception tracker of some kind? > > I have added a FIXME at each of these sites briefly saying what we should do. The alternate approach I suggested back in #c15 - let the bindings code automatically map exceptions to rejections - would take care of this for you. See https://codereview.chromium.org/521513002 for a VERY old delta CL showing how that'd look. Agreed that we can live with the FIXMEs for now. https://codereview.chromium.org/433793002/diff/580001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/580001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:21: WebServiceWorkerCache::QueryParams queryParamsFromDictionary(const Dictionary& dictionary) We can now use real Dictionary subclasses in modules but let's save that for a follow-up CL and get this in! https://codereview.chromium.org/433793002/diff/580001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:36: class CacheMatchCallbacks : public WebServiceWorkerCache::CacheMatchCallbacks { As noted in a FIXME over in CacheStorage.cpp we should consider using CallbackPromiseAdapter here and making this template-driven. But it's very readable as-is and we can do a bulk-conversion later if we decide it's an improvement. https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServ... File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:29: // Blink can store its internal interface to this object as a ProxyInterface in the WebServiceWorkerCache. This allows I'm not completely thrilled with the naming here, but don't have great suggestions. I'm surprised we don't have this pattern elsewhere but I'm not spotting it. I think in most other places where the embedder (chromium) provides an impl to blink on demand it doesn't re-use the object if it's re-requested - e.g. because blink satisfies the request on its own, or because there's no identity-preservation requirement, so this link-up isn't required. There's a similar use in WebServiceWorkerProxy. That and other examples indicate it's acceptable do this in a typesafe way, using a forward declaration of blink::Cache rather than an abstract interface. It'd also be nice to factor this pattern out similar to the WebPrivatePtr/WebPrivateOwnPtr which could make it typesafe and assert that it's assigned exactly once. But that can probably be done in a follow-up. We'll need an OWNERS review for the public API changes, who may have more input.
https://codereview.chromium.org/433793002/diff/580001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/580001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:91: Vector<Request*> requests; This should be HeapVector<Member<Request>>. https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServ... File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:8: #include "WebCommon.h" should be "public/platform/WebCommon.h" https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:29: // Blink can store its internal interface to this object as a ProxyInterface in the WebServiceWorkerCache. This allows On 2014/09/18 21:45:00, jsbell wrote: > I'm not completely thrilled with the naming here, but don't have great > suggestions. > > I'm surprised we don't have this pattern elsewhere but I'm not spotting it. I > think in most other places where the embedder (chromium) provides an impl to > blink on demand it doesn't re-use the object if it's re-requested - e.g. because > blink satisfies the request on its own, or because there's no > identity-preservation requirement, so this link-up isn't required. I have the same impression. Can we make WebServiceWorkerCache when Cache needs it? > There's a similar use in WebServiceWorkerProxy. That and other examples indicate > it's acceptable do this in a typesafe way, using a forward declaration of > blink::Cache rather than an abstract interface. It'd also be nice to factor this > pattern out similar to the WebPrivatePtr/WebPrivateOwnPtr which could make it > typesafe and assert that it's assigned exactly once. But that can probably be > done in a follow-up. Platform classes shouldn't refer to modules classes. https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:33: virtual inline ~ProxyInterface() = 0; Is |virtual inline| helpful? https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:47: enum WebServiceWorkerCacheOperationType { nit: This is an inner enum, and the name can be shorter. |OperationType| would be enough.
https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServ... File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:29: // Blink can store its internal interface to this object as a ProxyInterface in the WebServiceWorkerCache. This allows On 2014/09/18 23:35:56, tkent (overloaded) wrote: > On 2014/09/18 21:45:00, jsbell wrote: > > I'm not completely thrilled with the naming here, but don't have great > > suggestions. > > > > I'm surprised we don't have this pattern elsewhere but I'm not spotting it. I > > think in most other places where the embedder (chromium) provides an impl to > > blink on demand it doesn't re-use the object if it's re-requested - e.g. > because > > blink satisfies the request on its own, or because there's no > > identity-preservation requirement, so this link-up isn't required. > > I have the same impression. Can we make WebServiceWorkerCache when Cache needs > it? It is happening on demand; Blink has asked Chromium for a cache, and Chromium's impl of WSWC is what's being returned. Blink then mints a Cache and associates it with the WSWC before handing the Cache off to script. Conceptually, the ProxyInterface is implementing a "WeakMap" allowing a WSWC->Cache lookup. Within Blink we might use Supplementable for this. > > > There's a similar use in WebServiceWorkerProxy. That and other examples > indicate > > it's acceptable do this in a typesafe way, using a forward declaration of > > blink::Cache rather than an abstract interface. It'd also be nice to factor > this > > pattern out similar to the WebPrivatePtr/WebPrivateOwnPtr which could make it > > typesafe and assert that it's assigned exactly once. But that can probably be > > done in a follow-up. > > Platform classes shouldn't refer to modules classes. That's what I thought... but it looks like some references have snuck in from various modules. :( Maybe the secret cabal of API owners should do a quick audit. Or maybe I'm just misinterpreting what I'm seeing.
I think if we just renamed 'ProxyInterface' -> 'ScriptProxy' or even just 'Proxy' it'd clearer what's going on. The extra 'Interface' just makes it seem more complicated than it is.
On 2014/09/19 16:35:35, jsbell wrote: > I think if we just renamed 'ProxyInterface' -> 'ScriptProxy' or even just > 'Proxy' it'd clearer what's going on. The extra 'Interface' just makes it seem > more complicated than it is. I like "Proxy". I'll update along those lines shortly.
Thanks for the reviews. jsbell: your suggestions about followups are well on point. However, this patch is in rebase hell, and if it's good enough, let's land it with FIXMEs and continue on this in the repo with a lot less rebase pressure. tkent: PTAL. I'm especially interested in knowing more about the HeapVector<Member...> stuff. https://codereview.chromium.org/433793002/diff/580001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/580001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:91: Vector<Request*> requests; On 2014/09/18 23:35:55, tkent (high review load) wrote: > This should be HeapVector<Member<Request>>. Done. What about Vector<Response*> above? https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServ... File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:8: #include "WebCommon.h" On 2014/09/18 23:35:56, tkent (high review load) wrote: > should be "public/platform/WebCommon.h" Done. https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:27: virtual ~WebServiceWorkerCache() { } So.... how scary is it that this destructor is inlined, given that this class is not trivial? It has m_proxyInterface as a member, but that's just a pointer, with no destructor behaviour, so I _think_ this is safe and not going to generate tons of code. Is this still really bad? Should we add a .cpp file? https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:33: virtual inline ~ProxyInterface() = 0; On 2014/09/18 23:35:56, tkent (high review load) wrote: > Is |virtual inline| helpful? Yes. It's a trivial class, with no members, so inlining the destructor doesn't have any cost. This is very similar to virtual ~Foo() {}, which occurs all over. The only difference is that it's a pure virtual function; it's pure virtual to make the class abstract, which seems important because it communicates that this must be subclassed to be useful. The syntactic problem is that you can't provide a body together with a pure virtual declaration, so it's done using the inline keyword (see the end of this header to see the provided body). The destructor requires a body, since the destructor is called using non-virtual calling conventions from the destructors of classes that inherit from this. It's always legal to provide a body for a pure virtual function, but in this case it's required. https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:47: enum WebServiceWorkerCacheOperationType { On 2014/09/18 23:35:56, tkent (high review load) wrote: > nit: This is an inner enum, and the name can be shorter. |OperationType| would > be enough. Done.
jamesr@chromium.org changed reviewers: + jamesr@chromium.org
https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServ... File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServ... public/platform/WebServiceWorkerCache.h:33: virtual inline ~ProxyInterface() = 0; On 2014/09/22 16:05:24, gavinp wrote: > On 2014/09/18 23:35:56, tkent (high review load) wrote: > > Is |virtual inline| helpful? No. Just write this as: virtual ~ProxyInterface() { } the interface is not pure virtual because there are no pure virtual functions on it, so declaring it as such is not useful. The d'tor is empty so inlining it means nothing. In the Blink API, we normally call this an ExtraData type: https://code.google.com/p/chromium/codesearch#search/&q=extradata%20file:thir...
https://codereview.chromium.org/433793002/diff/580001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/580001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:91: Vector<Request*> requests; On 2014/09/22 16:05:24, gavinp wrote: > On 2014/09/18 23:35:55, tkent (high review load) wrote: > > This should be HeapVector<Member<Request>>. > > Done. What about Vector<Response*> above? Might as well be consistent. :) Oilpan protects objects referenced by pointers on the stack (so a local Foo* is safe and doesn't need to be Member<Foo>), but since the Vector's contents are on the heap they would not be protected. That said... the Vector<Foo*> here is passed directly into a function (ScriptPromiseResolver::resolve) which will wrap the objects before handing them to script, and those wrappers should have Member<> refs. So Oilpan GC should never get a chance to run before the vector's contents end up held safely. So using HeapVector<Member<>> shouldn't hurt, but I'm not sure it's necessary here. tkent@ - am I misunderstanding?
https://codereview.chromium.org/433793002/diff/580001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/580001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:91: Vector<Request*> requests; On 2014/09/22 16:42:50, jsbell wrote: > On 2014/09/22 16:05:24, gavinp wrote: > > On 2014/09/18 23:35:55, tkent (high review load) wrote: > > > This should be HeapVector<Member<Request>>. > > > > Done. What about Vector<Response*> above? > > Might as well be consistent. :) > > Oilpan protects objects referenced by pointers on the stack (so a local Foo* is > safe and doesn't need to be Member<Foo>), but since the Vector's contents are on > the heap they would not be protected. > > That said... the Vector<Foo*> here is passed directly into a function > (ScriptPromiseResolver::resolve) which will wrap the objects before handing them > to script, and those wrappers should have Member<> refs. So Oilpan GC should > never get a chance to run before the vector's contents end up held safely. > > So using HeapVector<Member<>> shouldn't hurt, but I'm not sure it's necessary > here. tkent@ - am I misunderstanding? > > And http://www.chromium.org/blink/blink-gc requires using HeapVector<Member<>> anyway. So yes, you should convert them all.
The latest upload gets oilpan correct on some of the callbacks per tkent's review. As well, we got rid of the really confusing proxy object entirely, thanks to jamesr's drive by yesterday. The result is a much cleaner interface, just relying on the CacheStorage implementation of NameToCache, which is more standards correct anyway (consider expandos on the cache object). The downside is that we'll NEVER destruct caches. Oh wells.
Clarifying for drive-bys: > The downside is that we'll NEVER destruct caches. ... for the lifetime of the worker, since the ServiceWorkerCacheStorage is owned by ServiceWorkerGlobalScope and the SWCS holds a reference to each Cache that's been accessed and never drops it. Yeah, unfortunate, but maybe we can be more clever here in the future. lgtm again
lgtm https://codereview.chromium.org/433793002/diff/580001/Source/modules/servicew... File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/580001/Source/modules/servicew... Source/modules/serviceworkers/Cache.cpp:91: Vector<Request*> requests; On 2014/09/22 16:45:34, jsbell wrote: > On 2014/09/22 16:42:50, jsbell wrote: > > On 2014/09/22 16:05:24, gavinp wrote: > > > On 2014/09/18 23:35:55, tkent (high review load) wrote: > > > > This should be HeapVector<Member<Request>>. > > > > > > Done. What about Vector<Response*> above? > > > > Might as well be consistent. :) > > > > Oilpan protects objects referenced by pointers on the stack (so a local Foo* > is > > safe and doesn't need to be Member<Foo>), but since the Vector's contents are > on > > the heap they would not be protected. > > > > That said... the Vector<Foo*> here is passed directly into a function > > (ScriptPromiseResolver::resolve) which will wrap the objects before handing > them > > to script, and those wrappers should have Member<> refs. So Oilpan GC should > > never get a chance to run before the vector's contents end up held safely. > > > > So using HeapVector<Member<>> shouldn't hurt, but I'm not sure it's necessary > > here. tkent@ - am I misunderstanding? > > > > > > And http://www.chromium.org/blink/blink-gc requires using HeapVector<Member<>> > anyway. So yes, you should convert them all. Right. We should use HeapVector<Member<Response>> too.
Thanks for the reviews everyone. I'll land this later today.
The CQ bit was checked by gavinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/433793002/720001
Message was sent while issue was closed.
Committed patchset #35 (id:720001) as 182604
Message was sent while issue was closed.
On 2014/09/24 15:01:27, I haz the power (commit-bot) wrote: > Committed patchset #35 (id:720001) as 182604 This is causing a build failure in http://build.chromium.org/p/chromium.webkit/builders/Linux%20GN/builds/6752 , so I'm baking up a revert.
Message was sent while issue was closed.
On 2014/09/24 15:16:07, gavinp wrote: > On 2014/09/24 15:01:27, I haz the power (commit-bot) wrote: > > Committed patchset #35 (id:720001) as 182604 > > This is causing a build failure in > http://build.chromium.org/p/chromium.webkit/builders/Linux%20GN/builds/6752 , so > I'm baking up a revert. This conflicted with my https://codereview.chromium.org/555133003/ which landed around the same time as this. It changed the signature of toImplArray<>() in V8Binding.h, called in CacheTest.cpp here. See https://codereview.chromium.org/555133003/diff/200001/Source/bindings/core/v8... for how to fix the issue. |