Re: [whatwg/fetch] Define the response Content-Type header parser (#831)

domenic commented on this pull request.

Some clarity suggestions. Very exciting stuff.

> + <li><p>Let <var>values</var> be a <a for=/>list</a> of <a for=/>strings</a>, initially empty.
+
+ <li><p>Let <var>value</var> be null.
+
+ <li>
+  <p>While <var>position</var> is not past the end of <var>input</var>:
+
+  <ol>
+   <li>
+    <p>Let <var>temporaryValue</var> be the result of <a>collecting a sequence of code points</a>
+    that are not U+0022 (") or U+002C (,) from <var>input</var>, given <var>position</var>.
+
+    <p class=note><var>temporaryValue</var> might be the empty string.
+
+   <li><p>If <var>value</var> is null, then set <var>value</var> to <var>temporaryValue</var>, and
+   append <var>temporaryValue</var> to <var>value</var> otherwise.

I find this structure a bit jarring to read an prefer "If X, then Y; otherwise Z". (Or as two separate sentences.)

> +    </ol>
+
+   <li><p>Remove all <a>HTTP tab or space</a> from the start and end of <var>value</var>.
+
+   <li><p><a for=list>Append</a> <var>value</var> to <var>values</var>.
+
+   <li><p>Set <var>value</var> to null.
+  </ol>
+
+ <li><p>Return <var>values</var>.
+</ol>
+
+<div class=example id=example-header-list-get-decode-split>
+ <p>This is how <a>get, decode, and split</a> functions in practice:
+
+ <table>

It'd be good to have an example of a comma inside quotes.

> +
+   <li><p>Set <var>value</var> to null.
+  </ol>
+
+ <li><p>Return <var>values</var>.
+</ol>
+
+<div class=example id=example-header-list-get-decode-split>
+ <p>This is how <a>get, decode, and split</a> functions in practice:
+
+ <table>
+  <tr>
+   <th>Input
+   <th>Output
+  <tr>
+   <td>`<code>nosniff,</code>`

You could use list notation on the right hand side.

> @@ -471,6 +584,9 @@ each other by 0x2C 0x20, in order.
      "<code>text/plain</code>", then return false.
     </ol>
 
+    <p class=warning>This intentionally does not use <a>extract a MIME type</a> as it is rather
+    forgiving and servers are not expected to implement it.

Which is "it"?

> +<p>To
+<dfn export for="header list" lt="extract a MIME type|extracting a MIME type" id=concept-header-extract-mime-type>extract a MIME type</dfn>
+from a <a for=/>header list</a> <var>headers</var>, run these steps:
+
+<ol>
+ <li><p>Let <var>charset</var> be null.
+
+ <li><p>Let <var>essence</var> be null.
+
+ <li><p>Let <var>mimeType</var> be null.
+
+ <li><p>Let <var>values</var> be the result of
+ <a for="header list">getting, decoding, and splitting</a> `<code>Content-Type</code>` from
+ <var>headers</var>.
+
+ <li><p>If <var>values</var> is null, then return failure.

I wonder if getting/decoding/splitting should return failure too?

> + <li><p>Let <var>essence</var> be null.
+
+ <li><p>Let <var>mimeType</var> be null.
+
+ <li><p>Let <var>values</var> be the result of
+ <a for="header list">getting, decoding, and splitting</a> `<code>Content-Type</code>` from
+ <var>headers</var>.
+
+ <li><p>If <var>values</var> is null, then return failure.
+
+ <li>
+  <p><a for=list>For each</a> <var>value</var> of <var>values</var>:
+
+  <ol>
+   <li><p>Set <var>mimeType</var> to the result of <a lt="parse a MIME type">parsing</a>
+   <var>value</var>.

It might be clearer to immediately "continue" if mimeType is failure.

> +   <li><p>Otherwise, if <var>mimeType</var> is not failure, <var>mimeType</var>'s
+   <a for="MIME type">essence</a> is <var>essence</var>, <var>mimeType</var>'s
+   <a for="MIME type">parameters</a>["<code>charset</code>"] does not <a for=map>exist</a>, and
+   <var>charset</var> is non-null, set <var>mimeType</var>'s
+   <a for="MIME type">parameters</a>["<code>charset</code>"] to <var>charset</var>.
+  </ol>
+
+ <li><p>Return <var>mimeType</var>.
+</ol>
+
+<p class=warning>Treat <a>extract a MIME type</a> returning failure or anything but the needed
+<a for=/>MIME type</a>'s <a for="MIME type">essence</a> as a fatal error. Existing web platform
+features not following this has been a major source of security vulnerabilities over the years.
+
+<p class=note>A <a for=/>MIME type</a>'s <a for="MIME type">parameters</a> are typically ignored and
+this practice has not been a problem.

"this has not been a problem in practice"?

> +     <li><p>Set <var>essence</var> to <var>mimeType</var>'s <a for="MIME type">essence</a>.
+    </ol>
+
+   <li><p>Otherwise, if <var>mimeType</var> is not failure, <var>mimeType</var>'s
+   <a for="MIME type">essence</a> is <var>essence</var>, <var>mimeType</var>'s
+   <a for="MIME type">parameters</a>["<code>charset</code>"] does not <a for=map>exist</a>, and
+   <var>charset</var> is non-null, set <var>mimeType</var>'s
+   <a for="MIME type">parameters</a>["<code>charset</code>"] to <var>charset</var>.
+  </ol>
+
+ <li><p>Return <var>mimeType</var>.
+</ol>
+
+<p class=warning>Treat <a>extract a MIME type</a> returning failure or anything but the needed
+<a for=/>MIME type</a>'s <a for="MIME type">essence</a> as a fatal error. Existing web platform
+features not following this has been a major source of security vulnerabilities over the years.

s/has/have

> +
+     <li><p>Set <var>essence</var> to <var>mimeType</var>'s <a for="MIME type">essence</a>.
+    </ol>
+
+   <li><p>Otherwise, if <var>mimeType</var> is not failure, <var>mimeType</var>'s
+   <a for="MIME type">essence</a> is <var>essence</var>, <var>mimeType</var>'s
+   <a for="MIME type">parameters</a>["<code>charset</code>"] does not <a for=map>exist</a>, and
+   <var>charset</var> is non-null, set <var>mimeType</var>'s
+   <a for="MIME type">parameters</a>["<code>charset</code>"] to <var>charset</var>.
+  </ol>
+
+ <li><p>Return <var>mimeType</var>.
+</ol>
+
+<p class=warning>Treat <a>extract a MIME type</a> returning failure or anything but the needed
+<a for=/>MIME type</a>'s <a for="MIME type">essence</a> as a fatal error. Existing web platform

This is a bit unclear. It returns a whole MIME type, not an essence, so strictly speaking according to this sentence, its return value should always be treated as fatal error.

> +     <li><p>If <var>mimeType</var>'s <a for="MIME type">parameters</a>["<code>charset</code>"]
+     <a for=map>exists</a>, then set <var>charset</var> to <var>mimeType</var>'s
+     <a for="MIME type">parameters</a>["<code>charset</code>"].
+
+     <li><p>Set <var>essence</var> to <var>mimeType</var>'s <a for="MIME type">essence</a>.
+    </ol>
+
+   <li><p>Otherwise, if <var>mimeType</var> is not failure, <var>mimeType</var>'s
+   <a for="MIME type">essence</a> is <var>essence</var>, <var>mimeType</var>'s
+   <a for="MIME type">parameters</a>["<code>charset</code>"] does not <a for=map>exist</a>, and
+   <var>charset</var> is non-null, set <var>mimeType</var>'s
+   <a for="MIME type">parameters</a>["<code>charset</code>"] to <var>charset</var>.
+  </ol>
+
+ <li><p>Return <var>mimeType</var>.
+</ol>

This algorithm would probably also benefit from a few examples. The main thing to communicate is how later values win, and how this charset overriding business works.

-- 
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/831#pullrequestreview-174012362