- From: Jake Archibald <notifications@github.com>
- Date: Fri, 25 Aug 2017 08:46:07 -0700
- To: w3c/ServiceWorker <ServiceWorker@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/ServiceWorker/pull/1190/review/58672366@github.com>
jakearchibald requested changes on this pull request.
It isn't a problem caused by this PR, but I think we're using JS objects too much in the cache spec. Ideally we should only create new `Request` and `Response` objects when we're about to return them, on the main thread.
> @@ -1861,13 +1861,9 @@ 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 |request| (a {{Request}} object) and |response| (a {{Response}} object).
Does it make sense to be storing JS objects here? Would it be better to store the concepts and create the objects just as we return them? (I realise that wasn't changed in this PR)
> @@ -1965,17 +1961,14 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. Run these substeps <a>in parallel</a>:
1. Let |responseArray| be an empty array.
1. If the optional argument |request| is omitted, then:
- 1. For each <a>fetching record</a> |entry| of its <a>request to response map</a>, in key insertion order:
- 1. Add a copy of |entry|.\[[value]] to |responseArray|.
+ 1. [=list/For each=] |item| of the [=context object=]'s [=request response list=]:
+ 1. Add a copy of |item|'s |response| to |responseArray|.
Given that it's a JS object, we can't really just say "copy", but I think the right answer here is to use concepts rather than objects, where "copy" appears to be fine.
> @@ -1965,17 +1961,14 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. Run these substeps <a>in parallel</a>:
1. Let |responseArray| be an empty array.
1. If the optional argument |request| is omitted, then:
- 1. For each <a>fetching record</a> |entry| of its <a>request to response map</a>, in key insertion order:
- 1. Add a copy of |entry|.\[[value]] to |responseArray|.
+ 1. [=list/For each=] |item| of the [=context object=]'s [=request response list=]:
+ 1. Add a copy of |item|'s |response| to |responseArray|.
This could be part of an additional PR though, as this PR doesn't introduce this problem.
> 1. And then, if an exception was <a lt="throw">thrown</a>, then:
- 1. Set the <a>context object</a>'s <a>request to response map</a> to |itemsCopy|.
+ 1. Set |cache| to |itemsCopy|.
Isn't this just setting a local variable? As in, it no longer reverts the operation.
Again, this isn't a new problem, but isn't there a bit of a race condition here? Since we're replacing the whole cache with an older copy, there may be concurrent operations of this resulting in data loss.
> </section>
<section algorithm>
<h3 id="batch-cache-operations-algorithm"><dfn>Batch Cache Operations</dfn></h3>
: Input
- :: |operations|, an array of {{CacheBatchOperation}} dictionary objects
+ :: |operations|, a [=list=] of {{CacheBatchOperation}} dictionary objects
We never expose this, so it doesn't need to be a dictionary right?
> 1. Try running the following substeps atomically:
- 1. Let |resultArray| be an empty array.
- 1. For each |operation| in |operations|:
+ 1. Let |resultList| be an empty [=list=].
+ 1. [=list/For each=] |operation| in |operations|:
1. If |operation|.{{CacheBatchOperation/type}} matches neither "delete" nor "put", <a>throw</a> a <code>TypeError</code>.
We should probably make this an enum or list of possible values.
> 1. If |operation|.{{CacheBatchOperation/type}} matches "put", then:
1. If |operation|.{{CacheBatchOperation/response}} is null, <a>throw</a> a <code>TypeError</code>.
1. Let |r| be |operation|.{{CacheBatchOperation/request}}'s associated [=Request/request=].
1. If |r|'s [=request/url=]'s [=url/scheme=] is not one of "<code>http</code>" and "<code>https</code>", <a>throw</a> a <code>TypeError</code>.
1. If |r|'s [=request/method=] is not \`<code>GET</code>\`, <a>throw</a> a <code>TypeError</code>.
1. If |operation|.{{CacheBatchOperation/options}} is not null, <a>throw</a> a <code>TypeError</code>.
- 1. Set |requestResponseArray| to the result of running <a>Query Cache</a> algorithm passing |operation|.{{CacheBatchOperation/request}}.
- 1. If |requestResponseArray| is not an empty array, then:
- 1. Let |requestResponse| be |requestResponseArray|[0].
- 1. Let |fetchingRecord| be the corresponding <a>fetching record</a> for |requestResponse|[0] and |requestResponse|[1] in <a>request to response map</a>.
- 1. Set |fetchingRecord|.\[[key]] to |operation|.{{CacheBatchOperation/request}} and |fetchingRecord|.\[[value]] to |operation|.{{CacheBatchOperation/response}}.
- 1. Else:
- 1. Set a newly-created <a>fetching record</a> {\[[key]]: |operation|.{{CacheBatchOperation/request}}, \[[value]]: |operation|.{{CacheBatchOperation/response}}} to <a>request to response map</a>.
-
- Note: The cache commit is allowed as long as the response's headers are available.
-
+ 1. Set |requestResponseList| to the result of running [=Query Cache=] with |operation|.{{CacheBatchOperation/request}}.
+ 1. If |requestResponseList| [=list/is not empty=], [=list/replace=] the [=list/item=] of |cache| that matches |requestResponseList|[0] with |operation|.{{CacheBatchOperation/request}}/|operation|.{{CacheBatchOperation/response}}.
What if there are multiple matches, shouldn't we be removing those? It might be easiest to remove all matches then just append the new entry.
> 1. If |operation|.{{CacheBatchOperation/type}} matches "put", then:
1. If |operation|.{{CacheBatchOperation/response}} is null, <a>throw</a> a <code>TypeError</code>.
1. Let |r| be |operation|.{{CacheBatchOperation/request}}'s associated [=Request/request=].
1. If |r|'s [=request/url=]'s [=url/scheme=] is not one of "<code>http</code>" and "<code>https</code>", <a>throw</a> a <code>TypeError</code>.
1. If |r|'s [=request/method=] is not \`<code>GET</code>\`, <a>throw</a> a <code>TypeError</code>.
1. If |operation|.{{CacheBatchOperation/options}} is not null, <a>throw</a> a <code>TypeError</code>.
- 1. Set |requestResponseArray| to the result of running <a>Query Cache</a> algorithm passing |operation|.{{CacheBatchOperation/request}}.
- 1. If |requestResponseArray| is not an empty array, then:
- 1. Let |requestResponse| be |requestResponseArray|[0].
- 1. Let |fetchingRecord| be the corresponding <a>fetching record</a> for |requestResponse|[0] and |requestResponse|[1] in <a>request to response map</a>.
- 1. Set |fetchingRecord|.\[[key]] to |operation|.{{CacheBatchOperation/request}} and |fetchingRecord|.\[[value]] to |operation|.{{CacheBatchOperation/response}}.
- 1. Else:
- 1. Set a newly-created <a>fetching record</a> {\[[key]]: |operation|.{{CacheBatchOperation/request}}, \[[value]]: |operation|.{{CacheBatchOperation/response}}} to <a>request to response map</a>.
-
- Note: The cache commit is allowed as long as the response's headers are available.
-
+ 1. Set |requestResponseList| to the result of running [=Query Cache=] with |operation|.{{CacheBatchOperation/request}}.
+ 1. If |requestResponseList| [=list/is not empty=], [=list/replace=] the [=list/item=] of |cache| that matches |requestResponseList|[0] with |operation|.{{CacheBatchOperation/request}}/|operation|.{{CacheBatchOperation/response}}.
Oh this is now done in addAll, but I'm not sure if it's right.
--
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-58672366
Received on Friday, 25 August 2017 15:46:29 UTC