Re: [whatwg/fetch] HTTPS upgrades proposal (PR #1655)

@annevk commented on this pull request.

Mainly nits and still the fundamental question about why you're patching HTTP fetch rather than HTTP-network fetch.

Having a commit message somewhere explaining the rationale behind the changes would go a long way.

> @@ -2157,6 +2157,17 @@ Unless stated otherwise, it is false.
 
 <p class=note>This flag is for exclusive use by HTML's render-blocking mechanism. [[!HTML]]
 
+<p>A <a for=/>request</a> has an associated boolean <dfn export for=request>is HTTPS upgrade</dfn>.
+Unless stated otherwise, it is false.
+
+<p class=note>This is for exclusive use by HTTPS Upgrading algorithm.
+
+<p>A <a for=/>request</a> has an associated
+<dfn export for=request>HTTPS upgrade fallback URL</dfn>, which is null or a <a for=/>URL</a>.
+Unless otherwise stated, it is null.
+
+<p class=note>This is for exclusive use by HTTPS Upgrading algorithm.

```suggestion
<p class=note>This is for exclusive use by HTTPS upgrading.
```
It's in use by two algorithms and since we don't refer to them directly, this seems better.

> @@ -2157,6 +2157,17 @@ Unless stated otherwise, it is false.
 
 <p class=note>This flag is for exclusive use by HTML's render-blocking mechanism. [[!HTML]]
 
+<p>A <a for=/>request</a> has an associated boolean <dfn export for=request>is HTTPS upgrade</dfn>.
+Unless stated otherwise, it is false.
+
+<p class=note>This is for exclusive use by HTTPS Upgrading algorithm.

```suggestion
<p class=note>This is for exclusive use by HTTPS upgrading.
```

> +<h4 id=https-upgrades-upgrade>HTTPS upgrade algorithm</h4>
+<div algorithm>
+
+<p>To <dfn>upgrade an HTTP request</dfn> given a <a for=/>request</a> <var>request</var>:

```suggestion
<h4 id=https-upgrades-upgrade>HTTPS upgrade algorithm</h4>

<div algorithm>
<p>To <dfn>upgrade an HTTP request</dfn> given a <a for=/>request</a> <var>request</var>:
```

> @@ -3265,6 +3276,109 @@ through TLS using ALPN. The protocol cannot be spoofed through HTTP requests in
 </div>
 
 
+<h3 id=https-upgrades>HTTPS upgrading</h3>

```suggestion
<h3 id=https-upgrading>HTTPS upgrading</h3>
```

> @@ -3265,6 +3276,109 @@ through TLS using ALPN. The protocol cannot be spoofed through HTTP requests in
 </div>
 
 
+<h3 id=https-upgrades>HTTPS upgrading</h3>
+
+<p>User agents may optionally upgrade requests with URLs that are not
+<a>potentially trustworthy URLs</a> to attempt to fetch them over
+<a>potentially trustworthy URLs</a>. If an upgraded request fails with a network error, it is
+retried over the original URL.
+
+<p>The HTTPS upgrading algorithm consists of <a>upgrade an HTTP request</a> and
+<a>HTTPS upgrade fallback</a> algorithms.
+
+<h4 id=https-upgrades-upgrade>HTTPS upgrade algorithm</h4>

```suggestion
<h4 id=https-upgrading-upgrade>HTTPS upgrade algorithm</h4>
```

> + <li>
+  <p>Otherwise, set the following fields:
+  <ul>
+   <li><p>Set <a for=request>HTTPS upgrade fallback URL</a> to <var>request</var>'s
+   <a for="request">URL</a>.
+
+   <li><p>Set <var>request</var>'s <a for="request">URL</a>'s <a for="url">scheme</a> to
+   "<code>https</code>".
+
+   <li><p>Set <a for=request>is HTTPS upgrade</a> to true.
+  </ul>
+ </li>
+</ol>
+</div>
+
+<h4 id=https-upgrades-fallback>Fallback algorithm</h4>

```suggestion
<h4 id=https-upgrading-fallback>Fallback algorithm</h4>
```

> +<div algorithm>
+
+<p>To run <dfn>HTTPS upgrade fallback</dfn> given a <a for=/>request</a> <var>request</var> and

```suggestion

<div algorithm>
<p>To run <dfn>HTTPS upgrade fallback</dfn> given a <a for=/>request</a> <var>request</var> and
```

> +
+   <li><p><var>request</var>'s <a for="request">method</a> is not "<code>GET</code>"
+
+   <li><p><var>request</var>'s <a for="request">URL</a>'s <a for="url">scheme</a> is not
+   "<code>http</code>"
+
+   <li><p><var>request</var>'s <a for="request">URL</a>'s <a for="url">host</a> is exempted from
+   upgrades in an <a>implementation-defined</a> way.
+  </ul>
+ </li>
+
+ <li>
+  <p>If <var>request</var>'s <a for=request>HTTPS upgrade fallback URL</a> is non-null, clear
+  <a for=request>is HTTPS upgrade</a> and <a for=request>HTTPS upgrade fallback URL</a> and return.
+
+  <p class=note>This is a fallback request that shouldn't be upgraded again.

You cannot have "should not" in a note. https://infra.spec.whatwg.org/#conformance

> +   <li><p><var>request</var>'s <a for="request">URL</a>'s <a for="url">scheme</a> is not
+   "<code>http</code>"
+
+   <li><p><var>request</var>'s <a for="request">URL</a>'s <a for="url">host</a> is exempted from
+   upgrades in an <a>implementation-defined</a> way.
+  </ul>
+ </li>
+
+ <li>
+  <p>If <var>request</var>'s <a for=request>HTTPS upgrade fallback URL</a> is non-null, clear
+  <a for=request>is HTTPS upgrade</a> and <a for=request>HTTPS upgrade fallback URL</a> and return.
+
+  <p class=note>This is a fallback request that shouldn't be upgraded again.
+
+ <li>
+  <p>Otherwise, set the following fields:

```suggestion
  <p>Otherwise:
```

> +  <ul>
+   <li><p><var>request</var>'s <a for="request">destination</a> is not "<code>document</code>"
+
+   <li><p><var>request</var>'s <a for="request">method</a> is not "<code>GET</code>"
+
+   <li><p><var>request</var>'s <a for="request">URL</a>'s <a for="url">scheme</a> is not
+   "<code>http</code>"
+
+   <li><p><var>request</var>'s <a for="request">URL</a>'s <a for="url">host</a> is exempted from
+   upgrades in an <a>implementation-defined</a> way.
+  </ul>
+ </li>
+
+ <li>
+  <p>If <var>request</var>'s <a for=request>HTTPS upgrade fallback URL</a> is non-null, clear
+  <a for=request>is HTTPS upgrade</a> and <a for=request>HTTPS upgrade fallback URL</a> and return.

clear is not a defined operation. You have to spell out what they are set to.

> +   "<code>https</code>".
+
+   <li><p>Set <a for=request>is HTTPS upgrade</a> to true.
+  </ul>
+ </li>
+</ol>
+</div>
+
+<h4 id=https-upgrades-fallback>Fallback algorithm</h4>
+<div algorithm>
+
+<p>To run <dfn>HTTPS upgrade fallback</dfn> given a <a for=/>request</a> <var>request</var> and
+<a for=/>response</a> <var>response</var>:
+
+<ol>
+ <li><p>If <var>request</var>'s <a for=request>is HTTPS upgrade</a> is not set, then return

A boolean is false or true, it's not set/unset.

> +
+<p>To run <dfn>HTTPS upgrade fallback</dfn> given a <a for=/>request</a> <var>request</var> and
+<a for=/>response</a> <var>response</var>:
+
+<ol>
+ <li><p>If <var>request</var>'s <a for=request>is HTTPS upgrade</a> is not set, then return
+ <var>response</var>.
+
+ <li>
+  <p>If <var>response</var> is a <a>network error</a>:
+
+  <p class=note>This means that the upgrade failed and initiates a fallback load.
+
+  <ol>
+   <li><p>Let <var>serializedFallbackUrl</var> be the <a lt="URL serializer">serialization</a> of
+   <var>request</var>'s <a for=request>HTTPS upgrade fallback URL</a>.

You need to isomorphic encode this as well to get a byte sequence.

> +   (`<code>Location</code>`, <var>serializedFallbackUrl</var>)», and its
+   <a for="response">status</a> set to 307.

```suggestion
   (`<code>Location</code>`, <var>serializedFallbackUrl</var>) » and
   <a for="response">status</a> is 307.
```

> +
+</ol>

```suggestion
</ol>
```

> +   <a for=response>header list</a> is «
+   (`<code>Location</code>`, <var>serializedFallbackUrl</var>)», and its
+   <a for="response">status</a> set to 307.
+
+   <li><p>Return <var>fallbackResponse</var>.
+  </ol>
+
+ <li>
+  <p>Return <var>response</var>.
+
+  <p class=note>This means the upgrade was successful.
+
+</ol>
+
+<p class=note>User agents can implement a fast-fallback path by canceling slow fetches on upgraded
+requests, in order to quickly initiate a fallback http load.

```suggestion
requests, in order to quickly initiate a fallback HTTP fetch.
```

> +
+</div>

```suggestion
</div>
```

> @@ -5157,8 +5279,6 @@ these steps:
       <p>If one of the following is true
 
       <ul class=brief>
-       <li><p><var>response</var>'s <a for=response>type</a> is "<code>error</code>"

You're still making changes to HTTP fetch rather than HTTP-network fetch or HTTP-network-or-cache fetch. This ends up impacting service workers as I pointed out before and seems unlikely to be intended. Could you please clarify this? (If you have explained this somewhere and I overlooked it, apologies. The PR is pretty long and I tried to go through the comments, but there's a lot of them.)

-- 
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/fetch/pull/1655#pullrequestreview-1719729187
You are receiving this because you are subscribed to this thread.

Message ID: <whatwg/fetch/pull/1655/review/1719729187@github.com>