Re: [w3c/ServiceWorker] Correcting the use of types and simplifying the response body read. (#1384)

annevk commented on this pull request.

Perhaps the editorial work (which is great) could land separately?

> @@ -1809,42 +1809,48 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
     <section algorithm="cache-put">
       <h4 id="cache-put">{{Cache/put(request, response)}}</h4>
 
-      <dfn method for="Cache"><code>put(|request|, |response|)</code></dfn> method *must* run these steps:
-
-        1. Let |r| be null.
-        1. If |request| is a {{Request}} object, then:
-            1. Set |r| to |request|'s [=Request/request=].
-            1. If |r|'s [=request/url=]'s [=url/scheme=] is not one of "`http`" and "`https`", or |r|'s [=request/method=] is not \`<code>GET</code>\`, return [=a promise rejected with=] a `TypeError`.
-        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. If |r|'s [=request/url=]'s [=url/scheme=] is not one of "`http`" and "`https`", return [=a promise rejected with=] a `TypeError`.
-        1. If |response|'s associated [=Response/response=]'s [=response/status=] is `206`, return [=a promise rejected with=] a `TypeError`.
-        1. If |response|'s associated [=Response/response=]'s [=response/header list=] contains a <a>header</a> [=header/named=] \`<code>Vary</code>\`, then:
+      <dfn method for="Cache"><code>put(|requestArg|, |responseObject|)</code></dfn> method *must* run these steps:

requestObject for consistency? (I've also never seen arg used so explicitly.)

> -        1. If |response| is [=Body/disturbed=] or [=Body/locked=], return [=a promise rejected with=] a `TypeError`.
-        1. Let |clonedResponse| be the result of [=response/clone|cloning=] |response|'s associated [=Response/response=].
-        1. If |response|'s body is non-null, run these substeps:
-            1. Let |dummyStream| be an [=empty=] {{ReadableStream}} object.
-            1. Set |response|'s [=response/body=] to a new [=/body=] whose [=stream=] is |dummyStream|.
-            1. Let |reader| be the result of [=get a reader|getting a reader=] from |dummyStream|.
-            1. [=Read all bytes=] from |dummyStream| with |reader|.
+        1. If |response|'s [=response/body=] is [=Body/disturbed=] or [=Body/locked=], return [=a promise rejected with=] a `TypeError`.
+        1. Let |clonedResponse| be a [=response/clone=] of |response|.
+        1. Let |bodyReadPromise| be [=a promise resolved with=] undefined.
+        1. If |response|'s [=response/body=] is non-null, run these substeps:
+            1. Let |stream| be |response|'s [=response/body=]'s [=body/stream=].
+            1. Let |reader| be the result of [=ReadableStream/get a reader|getting a reader=] for |stream|.
+            1. Set |bodyReadPromise| to the result of [=Read all bytes|reading all bytes=] from |stream| with |reader|.
+
+            Note: This ensures that |response|'s [=response/body=] is [=Body/locked=], and we have a full buffered copy of the body in |clonedResponse|.

I don't understand this. It seems there shouldn't be a clone at all. And if the stream is already tainted you'd throw.

-- 
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/1384#pullrequestreview-195038972