- From: Jake Archibald <notifications@github.com>
- Date: Fri, 29 Sep 2017 10:30:43 +0000 (UTC)
- To: w3c/ServiceWorker <ServiceWorker@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/ServiceWorker/pull/1190/review/66099101@github.com>
jakearchibald requested changes on this pull request.
This is coming along nicely!
>
- A <a>fetching record</a> has an associated <dfn id="dfn-incumbent-record">incumbent record</dfn> (a <a>fetching record</a>). It is initially set to null.
+ When a [=request response list=] is referenced from within an algorithm, an attribute getter, an attribute setter, or a method, it designates the instance that the [=context object=] represents, unless specified otherwise.
Feels like this would be better as a new definition. As in:
The **relevant request response list** is the instance that the context object represents.
Then it can be linked to when used.
>
- Each [=/origin=] has an associated <a>name to cache map</a>.
+ When a [=name to cache map=] is referenced from within an algorithm, an attribute getter, an attribute setter, or a method, it designates the instance of the [=context object=]'s associated [=CacheStorage/global object=]'s [=environment settings object=]'s [=environment settings object/origin=], unless specified otherwise.
As above.
> 1. Else if |request| is a string, then:
- 1. Set |r| to the associated [=Request/request=] of the result of invoking the initial value of {{Request}} as constructor with |request| as its argument. If this <a>throws</a> an exception, return a <a>promise</a> rejected with that exception.
- 1. Let |promise| be a new <a>promise</a>.
- 1. Run these substeps <a>in parallel</a>:
- 1. Let |responseArray| be an empty array.
+ 1. Set |r| to the associated [=Request/request=] of the result of invoking the initial value of {{Request}} as constructor with |request| as its argument. If this [=throws=] an exception, return [=a promise rejected with=] that exception.
+ 1. Let |realm| be the [=current Realm Record=].
Should this be https://html.spec.whatwg.org/multipage/webappapis.html#concept-relevant-realm for the context object?
> @@ -1861,15 +1862,15 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
<section>
<h3 id="cache-constructs">Constructs</h3>
- A <dfn id="dfn-fetching-record">fetching record</dfn> is a <a>Record</a> {\[[key]], \[[value]]} where \[[key]] is a {{Request}} and \[[value]] is a {{Response}}.
+ A <dfn id="dfn-request-response-list">request response list</dfn> is a [=list=] of [=pairs=] consisting of a request (a [=/request=]) and a response (a [=/response=]).
Should request and response here be defined as for="request response list"? Then they can be linked to when used.
> 1. Else:
- 1. Let |entries| be the result of running <a>Query Cache</a> algorithm passing a {{Request}} object associated with |r| and |options| as the arguments.
- 1. For each |entry| of |entries|:
- 1. Let |response| be null.
- 1. If the <a>incumbent record</a> |incumbentRecord| of the corresponding <a>fetching record</a> in <a>request to response map</a> is not null, set |response| to a copy of |incumbentRecord|.\[[value]].
- 1. Else, set |response| to a copy of |entry|[1].
- 1. Add |response| to |responseArray|.
+ 1. Let |requestResponses| be the result of running [=Query Cache=] with |r| and |options|.
+ 1. [=list/For each=] |requestResponse| of |requestResponses|:
+ 1. Add a copy of |requestResponse|'s response to |responses|.
+ 1. [=Queue a task=], on |promise|'s [=relevant settings object=]'s [=responsible event loop=] using the [=DOM manipulation task source=], to perform the following steps:
+ 1. Let |responseArray| be an empty JavaScript array, in |realm|.
I think we could create a sequence, then use https://heycam.github.io/webidl/#dfn-create-frozen-array to turn it into an array.
> 1. Else:
- 1. Let |entries| be the result of running <a>Query Cache</a> algorithm passing a {{Request}} object associated with |r| and |options| as the arguments.
- 1. For each |entry| of |entries|:
- 1. Let |response| be null.
- 1. If the <a>incumbent record</a> |incumbentRecord| of the corresponding <a>fetching record</a> in <a>request to response map</a> is not null, set |response| to a copy of |incumbentRecord|.\[[value]].
- 1. Else, set |response| to a copy of |entry|[1].
- 1. Add |response| to |responseArray|.
+ 1. Let |requestResponses| be the result of running [=Query Cache=] with |r| and |options|.
+ 1. [=list/For each=] |requestResponse| of |requestResponses|:
+ 1. Add a copy of |requestResponse|'s response to |responses|.
+ 1. [=Queue a task=], on |promise|'s [=relevant settings object=]'s [=responsible event loop=] using the [=DOM manipulation task source=], to perform the following steps:
+ 1. Let |responseArray| be an empty JavaScript array, in |realm|.
We need to update the IDL to return a FrozenArray rather than a sequence too.
> 1. For each |request| whose type is {{Request}} in |requests|:
- 1. Let |r| be |request|'s [=Request/request=].
+ 1. Set |r| to |request|'s [=Request/request=].
"Let" is [block-scoped](https://infra.spec.whatwg.org/#variables), so I think this should have remained "let".
> 1. If |r|'s [=request/url=]'s [=url/scheme=] is not one of "<code>http</code>" and "<code>https</code>", or |r|'s [=request/method=] is not \`<code>GET</code>\`, return a <a>promise</a> rejected with a <code>TypeError</code>.
1. For each |request| in |requests|:
- 1. Let |r| be the associated [=Request/request=] of the result of invoking the initial value of {{Request}} as constructor with |request| as its argument. If this <a>throws</a> an exception, return a <a>promise</a> rejected with that exception.
+ 1. Set |r| to the associated [=Request/request=] of the result of invoking the initial value of {{Request}} as constructor with |request| as its argument. If this <a>throws</a> an exception, return [=a promise rejected with=] that exception.
As above, should have remained "let"
> </section>
<section algorithm="cache-addAll">
<h4 id="cache-addAll">{{Cache/addAll(requests)}}</h4>
<dfn method for="Cache"><code>addAll(|requests|)</code></dfn> method *must* run these steps:
- 1. Let |responsePromiseArray| be an empty array.
- 1. Let |requestArray| be an empty array.
+ 1. Let |r| be null.
I don't think we need this, see below.
> </section>
<section algorithm="cache-addAll">
<h4 id="cache-addAll">{{Cache/addAll(requests)}}</h4>
<dfn method for="Cache"><code>addAll(|requests|)</code></dfn> method *must* run these steps:
- 1. Let |responsePromiseArray| be an empty array.
- 1. Let |requestArray| be an empty array.
+ 1. Let |r| be null.
+ 1. Let |responsePromises| be an empty [=list=].
+ 1. Let |requestList| be an empty [=list=].
1. For each |request| whose type is {{Request}} in |requests|:
Is this so we reject without consuming the body of the request? Might be worth adding a note about that.
> 1. If |r|'s [=request/url=]'s [=url/scheme=] is not one of "<code>http</code>" and "<code>https</code>", then:
- 1. [=fetch/Terminate=] all the ongoing <a>fetches</a> initiated by |requests| with reason *fatal*.
+ 1. [=fetch/Terminate=] all the ongoing [=fetches=] initiated by |requests| with reason *fatal*.
We've since dropped the "reason". There's an aborted flag which should probably be set in this case. https://fetch.spec.whatwg.org/#concept-fetch-terminate
> 1. If |r|'s [=request/url=]'s [=url/scheme=] is not one of "<code>http</code>" and "<code>https</code>", then:
- 1. [=fetch/Terminate=] all the ongoing <a>fetches</a> initiated by |requests| with reason *fatal*.
+ 1. [=fetch/Terminate=] all the ongoing [=fetches=] initiated by |requests| with reason *fatal*.
1. Break the loop.
Should we just return at this point?
> - 1. Else:
- 1. Delete |fetchingRecord| from <a>request to response map</a>.
- 1. Reject |responseBodyPromise| with a <code>TypeError</code>.
- 1. Else:
- 1. Set the <a>incumbent record</a> of the corresponding <a>fetching record</a> |fetchingRecord| in <a>request to response map</a> to the copy of |fetchingRecord|.
- 1. Let |invalidRecords| be the result of running <a>Query Cache</a> algorithm passing |fetchingRecord|.\[[key]] as the argument.
- 1. For each |invalidRecord| in |invalidRecords|:
- 1. If |invalidRecord| is not |fetchingRecord|, delete it from <a>request to response map</a>.
- 1. Resolve |responseBodyPromise| with |response|.
- 1. Add |responseBodyPromise| to |responseBodyPromiseArray|.
- 1. Let |q| be <a>waiting for all</a> of |responseBodyPromiseArray|.
- 1. Return the result of <a>transforming</a> |q| with a fulfillment handler that returns undefined.
+ 1. [=list/For each=] |fieldValue| of |fieldValues|:
+ 1. If |fieldValue| matches "`*`", then:
+ 1. Set |matchAsterisk| to true.
+ 1. [=Break=].
Could we just reject and abort steps here? Means we don't need *matchAsterisk*.
> - 1. Set the <a>incumbent record</a> of the corresponding <a>fetching record</a> |fetchingRecord| in <a>request to response map</a> to the copy of |fetchingRecord|.
- 1. Let |invalidRecords| be the result of running <a>Query Cache</a> algorithm passing |fetchingRecord|.\[[key]] as the argument.
- 1. For each |invalidRecord| in |invalidRecords|:
- 1. If |invalidRecord| is not |fetchingRecord|, delete it from <a>request to response map</a>.
- 1. Resolve |responseBodyPromise| with |response|.
- 1. Add |responseBodyPromise| to |responseBodyPromiseArray|.
- 1. Let |q| be <a>waiting for all</a> of |responseBodyPromiseArray|.
- 1. Return the result of <a>transforming</a> |q| with a fulfillment handler that returns undefined.
+ 1. [=list/For each=] |fieldValue| of |fieldValues|:
+ 1. If |fieldValue| matches "`*`", then:
+ 1. Set |matchAsterisk| to true.
+ 1. [=Break=].
+ 1. If |matchAsterisk| is true, reject |responsePromise| with a `TypeError`.
+
+ * To [=process response end-of-body=] for |response|, run these substeps:
+ 1. If |response|'s [=response/aborted flag=] is set, reject |responsePromise| with a `TypeError` and abort these steps.
Feels like this should be an AbortError.
> - 1. Add |responseBodyPromise| to |responseBodyPromiseArray|.
- 1. Let |q| be <a>waiting for all</a> of |responseBodyPromiseArray|.
- 1. Return the result of <a>transforming</a> |q| with a fulfillment handler that returns undefined.
+ 1. [=list/For each=] |fieldValue| of |fieldValues|:
+ 1. If |fieldValue| matches "`*`", then:
+ 1. Set |matchAsterisk| to true.
+ 1. [=Break=].
+ 1. If |matchAsterisk| is true, reject |responsePromise| with a `TypeError`.
+
+ * To [=process response end-of-body=] for |response|, run these substeps:
+ 1. If |response|'s [=response/aborted flag=] is set, reject |responsePromise| with a `TypeError` and abort these steps.
+ 1. Resolve |responsePromise| with |response|.
+
+ Note: The cache commit is allowed when the response's body is fully received.
+
+ * To [=process response done=] for |response|, do nothing.
I don't think we need this line.
> @@ -2134,23 +2104,29 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
<dfn method for="Cache"><code>keys(|request|, |options|)</code></dfn> method *must* run these steps:
- 1. Let |promise| be a new <a>promise</a>.
- 1. Run these substeps <a>in parallel</a>:
- 1. Let |resultArray| be an empty array.
+ 1. Let |r| be null.
+ 1. If the optional argument |request| is not omitted, then:
+ 1. If |request| is a {{Request}} object, then:
+ 1. Set |r| to |request|'s [=Request/request=].
+ 1. If |r|'s [=request/method=] is not \`<code>GET</code>\` and |options|.ignoreMethod is false, return [=a promise resolved with=] an empty array.
+ 1. Else if |request| is a string, then:
+ 1. Set |r| to the associated [=Request/request=] of the result of invoking the initial value of {{Request}} as constructor with |request| as its argument. If this [=throws=] an exception, return [=a promise rejected with=] that exception.
+ 1. Let |realm| be the [=current Realm Record=].
As above, we might not need this if we use frozenarray.
> 1. Else:
- 1. Let |p| be a <a>promise</a> resolved with undefined.
- 1. [=map/For each=] <var ignore>cacheName</var> → |cache| of <a>name to cache map</a>:
- 1. Set |p| to the result of <a>transforming</a> itself with a fulfillment handler that, when called with argument |v|, performs the following substeps <a>in parallel</a>:
- 1. If |v| is not undefined, return |v|.
- 1. Return the result of running the algorithm specified in {{Cache/match(request, options)}} method of {{Cache}} interface with |request| and |options| as the arguments (providing |cache|] as thisArgument to the <a>\[[Call]]</a> internal method of {{Cache/match(request, options)}}.)
- 1. Return |p|.
+ 1. Let |promise| be [=a promise resolved with=] undefined.
+ 1. [=map/For each=] <var ignore>cacheName</var> → |cache| of the [=name to cache map=]:
+ 1. Set |promise| to the result of [=transforming=] itself with a fulfillment handler that, when called with argument |response|, performs the following substeps [=in parallel=]:
+ 1. If |response| is not undefined, return |response|.
+ 1. Return the result of running the algorithm specified in {{Cache/match(request, options)}} method of {{Cache}} interface with |request| and |options| as the arguments (providing |cache| as thisArgument to the `\[[Call]]` internal method of {{Cache/match(request, options)}}.)
I don't think you need the slash in \[[Call]]
> 1. If |cacheName| matches |key|, then:
- 1. Return true.
- 1. Return false.
+ 1. Resolve |promise| with true.
+ 1. Abort these steps.
Could probably roll this into the line above. "Resolve promise with true and abort these steps".
> 1. If |cacheExists| is true, then:
- 1. Delete a <a>Record</a> {\[[key]], \[[value]]} <var ignore>entry</var> from its <a>name to cache map</a> where |cacheName| matches entry.\[[key]].
+ 1. [=map/Remove=] the [=name to cache map=][|cacheName|].
1. Return true.
I'm not sure we can "return" from in parallel steps.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/ServiceWorker/pull/1190#pullrequestreview-66099101
Received on Friday, 29 September 2017 10:31:08 UTC