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

@annevk commented on this pull request.

Thanks for working on this. I've done an initial review that mainly focuses on editorial issues.

> @@ -2133,6 +2133,19 @@ 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>https-upgrade</dfn>.
+Unless stated otherwise, it is false.
+
+<p class=note>This flag is for exclusive use by https-upgrades algorithm.

This should probably directly reference the actual algorithm(s) or simply state "HTTPS upgrading". Same below.

> @@ -2133,6 +2133,19 @@ 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>https-upgrade</dfn>.
+Unless stated otherwise, it is false.
+
+<p class=note>This flag is for exclusive use by https-upgrades algorithm.
+
+<p>A <a for=/>request</a> has an associated
+<dfn export for=request id=https-upgrade-fallback-url>https-upgrade-fallback-url</dfn>. It is a copy
+of the <a for=/>request</a>'s original <a for=/>URL</a> before <a for=/>request</a> is

`for=request>URL`, no?

>  
+<div algorithm="https-upgrades">

This contains two algorithms, that doesn't work. I recommend following the style we use elsewhere for these.

>  
+<div algorithm="https-upgrades">
+
+<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 URL</a>s.
+If an upgraded request fails with a network error, it is retried over the original URL.
+
+<p>The HTTPS upgrading algorithm consists of Upgrade and Fallback algorithms, with the following
+steps:

If you cross-reference the algorithms here you don't have a need for the colon followed by a heading (which is not a step).

> +<div algorithm="https-upgrades">
+
+<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 URL</a>s.
+If an upgraded request fails with a network error, it is retried over the original URL.
+
+<p>The HTTPS upgrading algorithm consists of Upgrade and Fallback algorithms, with the following
+steps:
+
+<h4 id=https-upgrades-upgrade>HTTPS upgrade algorithm</h4>
+
+To <dfn>upgrade an HTTP request</dfn> given a <a for=/>request</a> <var>request</var>, run these
+steps:
+
+<ol>
+  <li>

Fetch uses single-space indentation.

>  
+<div algorithm="https-upgrades">
+
+<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 URL</a>s.

This line exceeds 100 columns.

> @@ -5139,12 +5253,14 @@ these steps:
      <a>filtered response</a>; otherwise to <var>response</var>'s
      <a for="filtered response">internal response</a>.
 
+     <li><p>If <var>response</var>'s <a for=response>type</a> is "<code>error</code>" then
+      return the result of running <a>HTTPS upgrade fallback</a> given <var>request</var>
+      and a <a>network error</a>.

It seems this would return the original response rather than a new network error. That's probably okay though.

Nit: indentation is wrong. We don't further indent for wrapped lines.

> @@ -2133,6 +2133,19 @@ 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>https-upgrade</dfn>.
+Unless stated otherwise, it is false.
+
+<p class=note>This flag is for exclusive use by https-upgrades algorithm.
+
+<p>A <a for=/>request</a> has an associated
+<dfn export for=request id=https-upgrade-fallback-url>https-upgrade-fallback-url</dfn>. It is a copy

This doesn't state the type.

> @@ -5232,6 +5348,10 @@ these steps:
   <a>CORS check</a>, as <var>request</var>'s <a for=request>client</a> and the service worker can
   have different embedder policies.
 
+ <li>If <var>request</var>'s <code>https-upgrade</code> flag is set, set <var>response</var> and

It's not a flag. It's also not code.

> @@ -5232,6 +5348,10 @@ these steps:
   <a>CORS check</a>, as <var>request</var>'s <a for=request>client</a> and the service worker can
   have different embedder policies.
 
+ <li>If <var>request</var>'s <code>https-upgrade</code> flag is set, set <var>response</var> and
+  <var>internalResponse</var> to the result of running
+  <a>HTTPS upgrade fallback</a> given <var>request</var> and <var>response</var>.

Indentation is wrong.

> @@ -4418,6 +4526,12 @@ steps:
 
  <li><p><a>Upgrade <var>request</var> to a potentially trustworthy URL, if appropriate</a>.
 
+ <li><p>Optionally, run <a>upgrade an HTTP request</a> algorithm on <var>request</var>, if appropriate</a>.

It seems that this `<li>` contains multiple children. As such the `<p>` cannot be on the same line and the subsequent `<p>` needs indentation.

> +  <li>
+    <p class="note">The upgrade was successful.
+    <p>Return <var>response</var>.
+</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.
+
+</div>
+
+<h4 id=http-upgrades-examples>Examples</h4>
+
+<div id=example-https-upgrade-good-https class=example>
+ <p>
+ <code>a.com</code> serves both <code>http://a.com</code> and <code>https://a.com</code>.

I'm not sure why this is on a newline. A `<p>` should have its contents on the same line (up to 100 columns). This applies various times.

>  
+<div algorithm="https-upgrades">
+
+<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 URL</a>s.
+If an upgraded request fails with a network error, it is retried over the original URL.
+
+<p>The HTTPS upgrading algorithm consists of Upgrade and Fallback algorithms, with the following
+steps:
+
+<h4 id=https-upgrades-upgrade>HTTPS upgrade algorithm</h4>
+
+To <dfn>upgrade an HTTP request</dfn> given a <a for=/>request</a> <var>request</var>, run these

Please use `<p>` here and elsewhere.

> @@ -2133,6 +2133,19 @@ 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>https-upgrade</dfn>.

"HTTPS upgrade" or "is HTTPS upgrade" seems more reasonable as a name. Not sure why it's all lowercase.

> +    <p>If one or more of the following conditions are met, return:
+    <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.
+
+      <p class="example" id="example-https-upgrades-exempted-hosts">If <a for=url>host</a> is a
+      non-registrable or non-assignable domain name such as .local or an IP address that falls in a
+      range reserved for non-publicly routable networks, the implementation might return without
+      modifying <var>request</var>.

Is registrable domain really what you're after? We have a definition of those, but it looks like this wants something else. Best not to use that term in that case. And pretty soon we have a definition for local IP addresses as well.

> @@ -2133,6 +2133,19 @@ 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>https-upgrade</dfn>.
+Unless stated otherwise, it is false.
+
+<p class=note>This flag is for exclusive use by https-upgrades algorithm.
+
+<p>A <a for=/>request</a> has an associated
+<dfn export for=request id=https-upgrade-fallback-url>https-upgrade-fallback-url</dfn>. It is a copy
+of the <a for=/>request</a>'s original <a for=/>URL</a> before <a for=/>request</a> is

I thought the idea was that with synthetic redirects the initial URL would not be modified, but maybe the idea of synthetic redirects was abandoned?

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

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