Re: [whatwg/fetch] Add support for blob range requests (PR #1520)

@annevk commented on this pull request.

Thanks, this looks quite clean. I still have a couple inline comments and a longer thought here for a somewhat more substantial change.

So in principle we can get the type and size from a blob, although unfortunately they are not internal concepts as-of-yet.

That means we can avoid extracting twice.

It also means we can avoid setting up a new response upfront and can probably have two return statements at the end of the two branches where we create a response and set all its fields as we do now. (They're equivalent, but I kinda like the atomic look of the current setup.)

We also need only one conditional, whether request contains a `Range` header and the two branches can flow from that. (This is doable either way.)

Even if editorial, this would be some work. What do you think?

> +
+      <ol>
+       <li><p>Let <var>rangeHeader</var> be the result of <a for="header list">getting</a>
+       `<code>Range</code>` from <var>request</var>'s <a for=request>header list</a>.
+       <!-- Range should be added to the headers defined in HTTP extensions. -->
+
+       <li><p>Let <var>rangeValue</var> be the result of <a>parsing a single range header value</a>
+       given <var>rangeHeader</var>.
+
+       <li><p>If <var>rangeValue</var> is failure, then return a <a>network error</a>.
+
+       <li><p>Let <var>sliceEndRange</var> be null.
+
+       <li>
+        <p>If <var>rangeValue</var>[1] is non-null, then set <var>sliceEndRange</var> to
+        <code><var>rangeValue</var>[1] + 1</code>.

```suggestion
        <var>rangeValue</var>[1] + 1.
```
Equations aren't code. At least that's not what we've gone with thus far.

> +       <li><p>Let <var>slicedBlob</var> be the result of invoking <a>slice blob</a> given
+       <var>blobURLEntry</var>'s <a for="blob URL entry">object</a>, <var>rangeValue</var>[0],
+       <var>sliceEndRange</var>, and <var>type</var>.

So technically "slice blob" cannot be invoked with arguments that are null. Perhaps "slice blob" should require the arguments (and allow null) and `slice()` should do some appropriate defaulting before forwarding?

> +       <li><p>Set <var>response</var>'s <a for=response>status message</a> to `<code>OK</code>`.
+
+       <li><p>Set <var>response</var>'s <a for=response>body</a> to <var>body</var>.
+
+       <li><p>Set <var>response</var>'s <a for=response>header list</a> to «
+       (`<code>Content-Length</code>`, <var>fullLength</var>), (`<code>Content-Type</code>`,
+       <var>type</var>) ».
+      </ol>
+
+     <li>
+      <p>Otherwise:
+
+      <ol>
+       <li><p>Let <var>rangeHeader</var> be the result of <a for="header list">getting</a>
+       `<code>Range</code>` from <var>request</var>'s <a for=request>header list</a>.
+       <!-- Range should be added to the headers defined in HTTP extensions. -->

Yeah, I guess that makes sense. And then move the single range header value parser there?

> +       <li><p>Set <var>response</var>'s <a for=response>status message</a> to
+       `<code>Partial Content</code>`.
+
+       <li><p>Set <var>response</var>'s <a for=response>status</a> to 206.

Let's flip these two steps. Status is more significant.

> +
+        <p class="note">A range header denotes an inclusive byte range, while the <a>slice blob</a>
+        algorithm input range does not. To use the <a>slice blob</a> algorithm, we must increment
+        the parsed range header end value.
+
+       <li><p>Let <var>slicedBlob</var> be the result of invoking <a>slice blob</a> given
+       <var>blobURLEntry</var>'s <a for="blob URL entry">object</a>, <var>rangeValue</var>[0],
+       <var>sliceEndRange</var>, and <var>type</var>.
+
+       <li><p>Let <var>slicedBodyWithType</var> be the result of
+       <a for=BodyInit>safely extracting</a> <var>slicedBlob</var>.
+
+       <li><p>Set <var>response</var>'s <a for=response>body</a> to <var>slicedBodyWithType</var>'s
+       <a for="body with type">body</a>.
+
+       <li><p>Let <var>contentRange</var> be `<code>bytes </code>`.

Yeah please open it now so we can link it from the comment.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/fetch/pull/1520#pullrequestreview-1194120996

You are receiving this because you are subscribed to this thread.

Message ID: <whatwg/fetch/pull/1520/review/1194120996@github.com>