Re: [whatwg/fetch] HTTP cache partitioning (#943)

domenic commented on this pull request.



> @@ -1417,42 +1417,44 @@ Unless stated otherwise, it is unset.
 "<code>no-cache</code>", "<code>force-cache</code>", or
 "<code>only-if-cached</code>". Unless stated otherwise, it is "<code>default</code>".
 
+<p>Let <var>HTTPCache</var> be the result of determining the <a>HTTP cache</a> partition for

We've traditionally cased variables in camelCase, so this would be `<var>httpCache</var>`.

>  <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 the HTTP cache
-  contains a matching <a>fresh response</a> it will be returned. 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. If the HTTP cache contains a matching
-  <a>stale response</a>, a conditional network fetch will be returned to update the entry in
-  the HTTP cache. Otherwise, a non-conditional network fetch will be returned to update the entry
-  in the HTTP cache. [[!HTTP]] [[!HTTP-SEMANTICS]] [[!HTTP-COND]] [[!HTTP-CACHING]] [[!HTTP-AUTH]]
-  [[!STALE-WHILE-REVALIDATE]]
+  <dd><a for=/>Fetch</a> will inspect the <var>HTTPCache</var> on the way to the network. If the

Since it's a variable reference, it should be `<var>httpCache</var>`, not `the <var>HTTPCache</var>`, here and throughout.

> @@ -2049,6 +2051,29 @@ clearly stipulates that <a>connections</a> are keyed on
 <!-- See https://github.com/whatwg/fetch/issues/114#issuecomment-143500095 for when we make
      WebSocket saner -->
 
+<h3 id=http-cache-partition>HTTP Cache partition</h3>
+
+<p>To determine the associated <dfn export id=http-cache>HTTP cache</dfn> partition, given

I think this should define HTTP cache partition, not HTTP cache. The `id` is also redundant. So just `<dfn export>HTTP cache partition</dfn>` would work.

> @@ -2049,6 +2051,29 @@ clearly stipulates that <a>connections</a> are keyed on
 <!-- See https://github.com/whatwg/fetch/issues/114#issuecomment-143500095 for when we make
      WebSocket saner -->
 
+<h3 id=http-cache-partition>HTTP Cache partition</h3>
+
+<p>To determine the associated <dfn export id=http-cache>HTTP cache</dfn> partition, given
+<var>request</var>, run these steps:
+
+<ol>
+ <li>
+  <p>If <var>request</var>'s <a for=request>client</a> is null, terminate these steps.</li>

You can't terminate the steps without returning something. Maybe return null? Per https://github.com/whatwg/html/pull/4966#issuecomment-577316426 it sounds like in that case nothing will be cached, so, you'd probably need to update the processing model to account for when the return value of this algorithm is null.

> @@ -2049,6 +2051,29 @@ clearly stipulates that <a>connections</a> are keyed on
 <!-- See https://github.com/whatwg/fetch/issues/114#issuecomment-143500095 for when we make
      WebSocket saner -->
 
+<h3 id=http-cache-partition>HTTP Cache partition</h3>
+
+<p>To determine the associated <dfn export id=http-cache>HTTP cache</dfn> partition, given
+<var>request</var>, run these steps:
+
+<ol>
+ <li>
+  <p>If <var>request</var>'s <a for=request>client</a> is null, terminate these steps.</li>
+
+ <li>
+  <p>Let <var>environmentSettings</var> be <var>request</var>'s <a for=request>client</a>.</li>
+
+ <li>
+  <p>Let <var>topLevelOrigin</var> be <var>environmentSettings</var>'s top-level origin and

Before we merge this you'll want to change this to `<a for="environment settings object">top-level origin</a>`. But, don't do that until https://github.com/whatwg/html/pull/4966 has merged and had a day to propagate into the linking database, as otherwise it will just cause spec compilation errors.

> @@ -2049,6 +2051,29 @@ clearly stipulates that <a>connections</a> are keyed on
 <!-- See https://github.com/whatwg/fetch/issues/114#issuecomment-143500095 for when we make
      WebSocket saner -->
 
+<h3 id=http-cache-partition>HTTP Cache partition</h3>
+
+<p>To determine the associated <dfn export id=http-cache>HTTP cache</dfn> partition, given
+<var>request</var>, run these steps:
+
+<ol>
+ <li>
+  <p>If <var>request</var>'s <a for=request>client</a> is null, terminate these steps.</li>
+
+ <li>
+  <p>Let <var>environmentSettings</var> be <var>request</var>'s <a for=request>client</a>.</li>
+
+ <li>
+  <p>Let <var>topLevelOrigin</var> be <var>environmentSettings</var>'s top-level origin and
+  <var>origin</var> be <var>environmentSettings</var>'s origin.</li>

This should be `<a for="environment settings object">origin</a>`.

> +  <p>If <var>request</var>'s <a for=request>client</a> is null, terminate these steps.</li>
+
+ <li>
+  <p>Let <var>environmentSettings</var> be <var>request</var>'s <a for=request>client</a>.</li>
+
+ <li>
+  <p>Let <var>topLevelOrigin</var> be <var>environmentSettings</var>'s top-level origin and
+  <var>origin</var> be <var>environmentSettings</var>'s origin.</li>
+
+ <li>
+  <p>Return the HTTP cache associated with <var>topLevelOrigin</var> and optionally,
+  <var>origin</var>.</li>
+</ol>
+
+<p class="note no-backref">User agents can use the registrable domains associated with
+<var>topLevelOrigin</var> and <var>origin</var> instead of the origins themselves.

Hmm, I don't like how many degrees of freedom this gives us. It seems like there are now six configurations: (top-level origin + origin, top-level origin) x (registrable domain instead of top-level origin, top-level origin as-is) x (registrable domain instead of origin, origin as-is). Do we expect browsers to implement all six possible points on this matrix? That seems like a pretty bad failure of the consensus process. Can we get agreement on just one, or maybe two?

> @@ -4692,7 +4719,7 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
    <li><p>If <var>response</var> is not a
    <a>network error</a> and <var>request</var>'s
    <a for=request>cache mode</a> is not "<code>no-store</code>",
-   update <var>response</var> in the HTTP cache for <var>request</var>.
+   update <var>response</var> in the <a>HTTP cache</a> for <var>request</var>.

This should be `<var>httpCache</var>`, and not the general definition of a HTTP cache, 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/whatwg/fetch/pull/943#pullrequestreview-348957058