- From: Anne van Kesteren <notifications@github.com>
- Date: Tue, 12 Mar 2019 08:38:45 -0700
- To: whatwg/fetch <fetch@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <whatwg/fetch/pull/853/review/213454404@github.com>
annevk commented on this pull request.
Thanks, this looks very different from last time I looked! I've given mostly style feedback assuming not cloning the request is an oversight and the other errors are too.
@mnot the current state of this PR seems to be that we're not adding new API surface, but we are requiring that the fetch (request, if you will) made by the HTTP cache layer follows Fetch's general rules. That is quite important for browsers to meet their security guarantees. I guess in general when new HTTP features come up it would be good to see what their end-to-end integration might end up being. At least for features aimed at browsers.
> @@ -1404,10 +1407,16 @@ Unless stated otherwise, it is unset.
<div class="note no-backref">
<dl>
<dt>"<code>default</code>"
- <dd><a for=/>Fetch</a> will inspect the HTTP cache on the way to the network.
- If there is a fresh response it will be used. If there is a stale response a conditional request
- will be created, and a normal request otherwise. It then updates the HTTP cache with the response.
- [[!HTTP]] [[!HTTP-SEMANTICS]] [[!HTTP-COND]] [[!HTTP-CACHING]] [[!HTTP-AUTH]]
+ <dd><a for=/>Fetch</a> will inspect the HTTP cache on the way to the network:
+ <ol>
+ <li><p>If the HTTP cache contains a matching <a>fresh response</a> it will be returned.
+ <li><p>If the HTTP cache contains a matching <a>stale-while-revalidate response</a> it will be returned,
+ and a conditional network fetch will be made to update the entry in the HTTP cache.
+ <li><p>If the HTTP cache contains a matching <a>stale response</a>, a conditional network will be created
+ and returned.
This drops the cache updating that the original text had. (Same for the next point.)
Also, perhaps this should be a long paragraph instead similar to the original? It's not really meant to be an algorithm. Just an explanation.
> @@ -1896,6 +1909,16 @@ is a <a>filtered response</a> whose
<li><p>Return <var>newResponse</var>.
</ol>
+<p>A <dfn export id=concept-fresh-response>fresh response</dfn> is a <a for=/>response</a> whose
+age is within its <a href=https://tools.ietf.org/html/rfc7234#section-4.2.1>freshness lifetime</a>.
+
+<p>A <dfn export id=concept-stale-while-revalidate-response>stale-while-revalidate response</dfn> is a
+<a for=/>response</a> that is not a <a>fresh response</a>, and its current age is within
+the <a href=https://tools.ietf.org/html/rfc5861#section-3>stale-while-revalidate lifetime</a>.
+
+<p>A <dfn export id=concept-stale-response>stale response</dfn> is
+a <a for=/>response</a> is a response that is not a <a>fresh response</a> or a
+<a>stale-while-revalidate response</a>.
Need an additional newline here since there were two before the `<h4>`.
> @@ -4151,7 +4174,8 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
`<code>If-Range</code>`, then set <var>httpRequest</var>'s
<a for=request>cache mode</a> to "<code>no-store</code>".
- <li><p>If <var>httpRequest</var>'s <a for=request>cache mode</a> is "<code>no-cache</code>" and
+ <li><p>If <var>httpRequest</var>'s <a for=request>cache mode</a> is "<code>no-cache</code>",
+ <a for=request>prevent no-cache cache-control header modification flag</a> is unset and
You need to lead this with _httpRequest_ again assuming that's where you grab the flag from.
> @@ -4151,7 +4174,8 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
`<code>If-Range</code>`, then set <var>httpRequest</var>'s
<a for=request>cache mode</a> to "<code>no-store</code>".
- <li><p>If <var>httpRequest</var>'s <a for=request>cache mode</a> is "<code>no-cache</code>" and
+ <li><p>If <var>httpRequest</var>'s <a for=request>cache mode</a> is "<code>no-cache</code>",
+ <a for=request>prevent no-cache cache-control header modification flag</a> is unset and
Also need an Oxford comma.
> @@ -4283,32 +4307,50 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
<!-- cache hit -->
<ol>
- <li><p>If <var>storedResponse</var> requires validation (i.e., it is not fresh), then set the
- <var>revalidatingFlag</var>.
-
- <li>
- <p>If the <var>revalidatingFlag</var> is set and <var>httpRequest</var>'s
- <a for=request>cache mode</a> is neither "<code>force-cache</code>" nor
- "<code>only-if-cached</code>", then:
+ <li><p>If <a for=request>cache mode</a> is "<code>default</code>" and
The indenting is wrong here. If an `<li>` has multiple nested elements you need to put the `<p>` on a newline (and indent with an extra space).
> @@ -4283,32 +4307,50 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
<!-- cache hit -->
<ol>
- <li><p>If <var>storedResponse</var> requires validation (i.e., it is not fresh), then set the
- <var>revalidatingFlag</var>.
-
- <li>
- <p>If the <var>revalidatingFlag</var> is set and <var>httpRequest</var>'s
- <a for=request>cache mode</a> is neither "<code>force-cache</code>" nor
- "<code>only-if-cached</code>", then:
+ <li><p>If <a for=request>cache mode</a> is "<code>default</code>" and
+ <var>storedResponse</var> is a <a>stale-while-revalidate response</a>, then:
+ <ol>
+ <li><p>Set <var>response</var> to <var>storedResponse</var>.
+ <li><p>Set <var>response</var>'s <a for=response>cache state</a> to "<code>local</code>".
+ <li><p>In parallel perform <a for=main>main fetch</a> using <var>request</var> with
Same here.
> @@ -4283,32 +4307,50 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
<!-- cache hit -->
<ol>
- <li><p>If <var>storedResponse</var> requires validation (i.e., it is not fresh), then set the
- <var>revalidatingFlag</var>.
-
- <li>
- <p>If the <var>revalidatingFlag</var> is set and <var>httpRequest</var>'s
- <a for=request>cache mode</a> is neither "<code>force-cache</code>" nor
- "<code>only-if-cached</code>", then:
+ <li><p>If <a for=request>cache mode</a> is "<code>default</code>" and
+ <var>storedResponse</var> is a <a>stale-while-revalidate response</a>, then:
+ <ol>
+ <li><p>Set <var>response</var> to <var>storedResponse</var>.
+ <li><p>Set <var>response</var>'s <a for=response>cache state</a> to "<code>local</code>".
+ <li><p>In parallel perform <a for=main>main fetch</a> using <var>request</var> with
Also use `<a>In parallel</a>`?
> @@ -4283,32 +4307,50 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
<!-- cache hit -->
<ol>
- <li><p>If <var>storedResponse</var> requires validation (i.e., it is not fresh), then set the
- <var>revalidatingFlag</var>.
-
- <li>
- <p>If the <var>revalidatingFlag</var> is set and <var>httpRequest</var>'s
- <a for=request>cache mode</a> is neither "<code>force-cache</code>" nor
- "<code>only-if-cached</code>", then:
+ <li><p>If <a for=request>cache mode</a> is "<code>default</code>" and
+ <var>storedResponse</var> is a <a>stale-while-revalidate response</a>, then:
+ <ol>
+ <li><p>Set <var>response</var> to <var>storedResponse</var>.
+ <li><p>Set <var>response</var>'s <a for=response>cache state</a> to "<code>local</code>".
+ <li><p>In parallel perform <a for=main>main fetch</a> using <var>request</var> with
+ <ul class=brief>
Indentation is wrong here.
> -
- <li>
- <p>If the <var>revalidatingFlag</var> is set and <var>httpRequest</var>'s
- <a for=request>cache mode</a> is neither "<code>force-cache</code>" nor
- "<code>only-if-cached</code>", then:
+ <li><p>If <a for=request>cache mode</a> is "<code>default</code>" and
+ <var>storedResponse</var> is a <a>stale-while-revalidate response</a>, then:
+ <ol>
+ <li><p>Set <var>response</var> to <var>storedResponse</var>.
+ <li><p>Set <var>response</var>'s <a for=response>cache state</a> to "<code>local</code>".
+ <li><p>In parallel perform <a for=main>main fetch</a> using <var>request</var> with
+ <ul class=brief>
+ <li><p><a for=request>cache mode</a> set to "<code>no-cache</code>".
+ <li><p><a for=request>prevent no-cache cache-control header modification flag</a> set.
+ <li><p><a for=request>service-workers mode</a> set to "<code>none</code>".
+ </ul>
You're modifying the request here. It seems to me you'd need to copy it first?
> <ol>
- <li><p>If <var>storedResponse</var>'s <a for=response>header list</a>
- <a for="header list">contains</a> `<code>ETag</code>`, then <a for="header list">append</a>
- `<code>If-None-Match</code>` with its value to <var>httpRequest</var>'s
- <a for=request>header list</a>.
-
- <li><p>If <var>storedResponse</var>'s <a for=response>header list</a>
- <a for="header list">contains</a> `<code>Last-Modified</code>`, then
- <a for="header list">append</a> `<code>If-Modified-Since</code>` with its value to
- <var>httpRequest</var>'s <a for=request>header list</a>.
+ <li><p>If <var>storedResponse</var> is a <a>stale response</a>, then set the
+ <var>revalidatingFlag</var>.
+ <p>If the <var>revalidatingFlag</var> is set and <a for=request>cache mode</a>
It seems this should be a new list item?
> <ol>
- <li><p>If <var>storedResponse</var>'s <a for=response>header list</a>
- <a for="header list">contains</a> `<code>ETag</code>`, then <a for="header list">append</a>
- `<code>If-None-Match</code>` with its value to <var>httpRequest</var>'s
- <a for=request>header list</a>.
-
- <li><p>If <var>storedResponse</var>'s <a for=response>header list</a>
- <a for="header list">contains</a> `<code>Last-Modified</code>`, then
- <a for="header list">append</a> `<code>If-Modified-Since</code>` with its value to
- <var>httpRequest</var>'s <a for=request>header list</a>.
+ <li><p>If <var>storedResponse</var> is a <a>stale response</a>, then set the
+ <var>revalidatingFlag</var>.
+ <p>If the <var>revalidatingFlag</var> is set and <a for=request>cache mode</a>
+ is not one of "<code>force-cache</code>", "<code>only-if-cached</code>", then:
This sentence doesn't make sense? Now that there's no new enum value the original wording should be restored I think.
> <ol>
- <li><p>If <var>storedResponse</var>'s <a for=response>header list</a>
- <a for="header list">contains</a> `<code>ETag</code>`, then <a for="header list">append</a>
- `<code>If-None-Match</code>` with its value to <var>httpRequest</var>'s
- <a for=request>header list</a>.
-
- <li><p>If <var>storedResponse</var>'s <a for=response>header list</a>
- <a for="header list">contains</a> `<code>Last-Modified</code>`, then
- <a for="header list">append</a> `<code>If-Modified-Since</code>` with its value to
- <var>httpRequest</var>'s <a for=request>header list</a>.
+ <li><p>If <var>storedResponse</var> is a <a>stale response</a>, then set the
+ <var>revalidatingFlag</var>.
+ <p>If the <var>revalidatingFlag</var> is set and <a for=request>cache mode</a>
This also dropped where you got "cache mode" from. Which request?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/fetch/pull/853#pullrequestreview-213454404
Received on Tuesday, 12 March 2019 15:39:08 UTC