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

@yoavweiss commented on this pull request.

A few more nits. Otherwise, non-authoritiative LGTM

> +
+      <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">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>.
+    </ul>
+  </li>
+
+  <li><p>If <var>request</var>'s <code>https-upgrade-fallback-url</code> is non-null, clear the
+  <code>https-upgrade</code> and <code>https-upgrade-fallback-url</code> flags and return.
+
+  <p class="note">This is a fallback request that shouldn't be upgraded again.
+
+  <li><p>Otherwise, upgrade the request by setting the following fields:

"Upgradt the request" is not needed here. Maybe move it to a note?

> +  </li>
+</ol>
+
+<h4 id=https-upgrades-fallback>Fallback algorithm</h4>
+
+To run <dfn>HTTPS upgrade fallback</dfn> given a <a for=/>request</a>
+<var>request</var> and <a for=/>response</a> <var>response</var>, run these steps:
+
+<ol>
+  <li><p>If <var>request</var>'s <code>https-upgrade</code> flag is not set, return
+  <var>response</var>.
+
+  <p class="note">This means <var>request</var> was not upgraded.
+
+  <li>
+    <p class="note">The request was upgraded.

This note is rendered right after the last note. Can you rearrange them to be clearer?

> +  <var>response</var>.
+
+  <p class="note">This means <var>request</var> was not upgraded.
+
+  <li>
+    <p class="note">The request was upgraded.
+
+    <p>If <var>response</var> is a network error, run the following steps:
+    <p class="note">This means that the upgrade failed and initiates a fallback load.
+     <ol>
+      <li><p>Let <var>fallbackResponse</var> be a new <a>Response</a> with its <code>Location</code> header set to
+      <code>https-upgrade-fallback-url</code>, and its <a for="response">status</a> set to 307.
+      <li>Return <var>fallbackResponse</var>.
+     </ol>
+
+  <li><p>Otherwise, the upgrade was successful. Return <var>response</var>.

No need for "otherwise" here as everything above early returns. "upgrade was successful" should be a note.

> @@ -4405,6 +4523,8 @@ steps:
 
  <li><p><a>Upgrade <var>request</var> to a potentially trustworthy URL, if appropriate</a>.
 
+ <li><p><a href="#https-upgrades-upgrade">Optionally, run HTTPS upgrading algorithm on <var>request</var>, if appropriate</a>.

OK, cool. Maybe add a note to that effect?

> +  </li>
+</ol>
+
+<h4 id=https-upgrades-fallback>Fallback algorithm</h4>
+
+To run <dfn>HTTPS upgrade fallback</dfn> given a <a for=/>request</a>
+<var>request</var> and <a for=/>response</a> <var>response</var>, run these steps:
+
+<ol>
+  <li><p>If <var>request</var>'s <code>https-upgrade</code> flag is not set, return
+  <var>response</var>.
+
+  <p class="note">This means <var>request</var> was not upgraded.
+
+  <li>
+    <p class="note">The request was upgraded.

Maybe drop the "request was not upgraded" and "request was upgraded" comments, as they're somewhat obvious.

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

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