Add support for blob range requests #1520

dlrobertson · 2022-10-29T05:03:05Z

At least two implementers are interested (and none opposed):
- Chromium and Webkit seem to have at least partial implementations
Tests are written and can be reviewed and commented upon at:
- Add range tests for blob web-platform-tests/wpt#34384
- Add Range header with HTTP OWS preflight test web-platform-tests/wpt#37569
Implementation bugs are filed:
- Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1784880
- Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1399678
- WebKit: https://bugs.webkit.org/show_bug.cgi?id=248994
MDN issue is filed: Fetch: blob: URL range requests mdn/content#24625

(See WHATWG Working Mode: Changes for more details.)

Implement support for blob range requests.

Related to: #1070

Preview | Diff

dlrobertson · 2022-10-29T05:09:29Z

Posting this a bit early in the implementation phase... See the many TODOs 😄

Any feedback would be really helpful!

annevk · 2022-11-02T14:42:59Z

fetch.bs

+     <li><p>If <var>response</var>'s <a for=response>range-requested flag</a> is unset, run the
+     following steps:
+
+     <ol>
+       <li><p>Set <var>response</var>'s <a for=response>status message</a> to `<code>OK</code>`


High-level formatting nit (this does not just apply here):

Suggested change

<li>If <var>response</var>'s <a for=response>range-requested flag</a> is unset, run the

following steps:

<ol>

<li>Set <var>response</var>'s <a for=response>status message</a> to `<code>OK</code>`

<li>

If <var>response</var>'s <a for=response>range-requested flag</a> is unset, then:

<ol>

<li>Set <var>response</var>'s <a for=response>status message</a> to `<code>OK</code>`.

annevk · 2022-11-03T09:55:28Z

fetch.bs

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


You want to set status to 206 as well here. (It might be okay to not set status message at all and let it be the empty byte sequence as it would be in HTTP/2 and up, but I don't care strongly.)

annevk · 2022-11-03T09:55:55Z

fetch.bs

+         <p>blob.slice(<var>rangeStart</var>[0], <var>rangeValue</var>[1], <var>type</var>) here
+         on <var>body</var>.
+
+         <p class="note">TODO(dlrobertson): This requires a lot more thought.


Primarily we don't want to use the public JS API here but instead call an internal algorithm.

Yeah, the more I've looked at the the more I think your suggestion to upstream a change to FileAPI to make the guts of blob.slice a separate algorithm would be real nice.

Made a first attempt at this in w3c/FileAPI#183.

annevk · 2022-11-03T09:58:16Z

fetch.bs

+       <var>contentRange</var>.
+
+       <li>
+         <p>If <var>rangeValue</var>[0] is null, <a lt="serialize an integer">Serialize</a> and


Maybe use "Otherwise" here instead to make it clear it's one or the other?

annevk · 2022-11-03T09:59:40Z

fetch.bs

     <li><p>Let <var>bodyWithType</var> be the result of <a for=BodyInit>safely extracting</a>
     <var>blobURLEntry</var>'s <a for="blob URL entry">object</a>.

     <li><p>Let <var>body</var> be <var>bodyWithType</var>'s <a for="body with type">body</a>.

-     <li><p>Let <var>length</var> be <var>body</var>'s <a for=body>length</a>,
+     <li><p>Let <var>bodyLength</var> be <var>body</var>'s <a for=body>length</a>,


Maybe initialLength/originalLength/fullLength is clearer?

dlrobertson · 2022-11-23T15:38:04Z

Updated to use slice blob.

fetch.bs

annevk

I spotted a lot of tiny nits, but I might have missed something bigger therefore. Probably needs another close look once these are addressed. Hope that's understandable.

annevk · 2022-11-24T15:13:41Z

fetch.bs

+     <li><p>Set <var>response</var> to a new <a for=/>response</a>.
+
+     <li><p>If <var>request</var>'s <a for=response>header list</a> contains `<code>Range</code>`,
+     set <var>response</var>'s <a for=response>range-requested flag</a>.


Nit:

Suggested change

set <var>response</var>'s <a for=response>range-requested flag</a>.

then set <var>response</var>'s <a for=response>range-requested flag</a>.

annevk · 2022-11-24T15:14:14Z

fetch.bs

+      <p>If <var>response</var>'s <a for=response>range-requested flag</a> is unset, run the
+      following steps:


Suggested change

If <var>response</var>'s <a for=response>range-requested flag</a> is unset, run the

following steps:

If <var>response</var>'s <a for=response>range-requested flag</a> is unset:

annevk · 2022-11-24T15:17:45Z

fetch.bs

+      <p>Otherwise, if <var>response</var>'s <a for=response>range-requested flag</a> is set, run the
+      following steps:


Suggested change

Otherwise, if <var>response</var>'s <a for=response>range-requested flag</a> is set, run the

following steps:

Otherwise:

(I was initially thinking we could maybe keep it without the "if" or have it as assert, but since there's only two branches I don't think that's warranted.)

annevk · 2022-11-24T15:19:13Z

fetch.bs

+      following steps:
+
+      <ol>
+       <li><p>Let <var>rangeHeader</var> be the result of <a>extracting header list values</a> given


You want to use "get" or "get, decode, and split". We're trying to get rid of "extracting header list values".

annevk · 2022-11-24T15:19:47Z

fetch.bs

+       <li><p>Let <var>sliceEndRange</var> be null.
+
+       <li>
+        <p>If <var>rangeValue</var>[1] is non-null, let <var>sliceEndRange</var> be


then set sliceEndRange to*

annevk · 2022-11-24T15:40:40Z

fetch.bs

+       <li><p>Set <var>response</var>'s <a for=response>status</a> to `<code>206</code>`.
+
+       <li><p>Set <var>response</var>'s <a for=response>header list</a> to «
+       (`<code>Content-Length</code>`, <var>length</var>), (`<code>Content-Type</code>`,


Where is <var>length</var> defined?

annevk · 2022-11-24T15:40:54Z

fetch.bs

+       <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 `<code>206</code>`.


I'm pretty sure this field is an integer.

Suggested change

<li>Set <var>response</var>'s <a for=response>status</a> to `<code>206</code>`.

<li>Set <var>response</var>'s <a for=response>status</a> to 206.

fetch.bs

annevk · 2022-11-24T15:42:36Z

fetch.bs

+       <var>type</var>), (`<code>Content-Range</code>`, <var>contentRange</var>) ».
+      </ol>
+
+     <li><p> return <var>response</var>.


Suggested change

<li> return <var>response</var>.

<li>Return <var>response</var>.

dlrobertson

I spotted a lot of tiny nits, but I might have missed something bigger therefore. Probably needs another close look once these are addressed. Hope that's understandable.

Definitely. Feedback is greatly appreciated!

fetch.bs

annevk

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?

annevk · 2022-11-25T10:06:57Z

fetch.bs

+
+       <li>
+        <p>If <var>rangeValue</var>[1] is non-null, then set <var>sliceEndRange</var> to
+        <code><var>rangeValue</var>[1] + 1</code>.


Suggested change

<code><var>rangeValue</var>[1] + 1</code>.

<var>rangeValue</var>[1] + 1.

Equations aren't code. At least that's not what we've gone with thus far.

fetch.bs

annevk · 2022-11-25T10:13:01Z

fetch.bs

+      <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?

annevk · 2022-11-25T10:17:16Z

fetch.bs

+       <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.

fetch.bs

dlrobertson · 2022-11-29T16:45:28Z

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.

Yeah, this makes sense. I didn't change this in order to attempt to keep things closer to how they were before, but we could easily remove the bodyWithType steps in the section of the algorithm before the branch.

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?

I like the idea of the single return statement using Blob size and type definitions, but I'm happy to change this if this isn't preferred.

dlrobertson · 2022-12-01T02:39:01Z

Updated to use Blob.size and Blob.type

annevk

Specifications are generally quite happy with multiple return statements, but I'm okay sticking with one here. Couple final nits and this seems good to go.

annevk · 2022-12-01T11:51:14Z

fetch.bs

+     <li><p>If <var>request</var>'s <a for=response>header list</a> contains `<code>Range</code>`,
+     then set <var>response</var>'s <a for=response>range-requested flag</a>.


I still think we should deduplicate this with the conditional below.

annevk · 2022-12-01T11:52:32Z

fetch.bs


-     <li><p>Let <var>length</var> be <var>body</var>'s <a for=body>length</a>,
+     <li><p>Let <var>fullLength</var> be <var>blobURLEntry</var>'s
+     <a for="blob URL entry">object</a>'s <a for=Blob data-link-type=attribute>size</a>,


We can use {{Blob/size}} as we do elsewhere in this document. Same for type.

No problem! Just for future reference, what is the benefit of {{Blob/size}} and how does it differ from <a for=Blob data-link-type=attribute>size</a>? Both create links to https://w3c.github.io/FileAPI/#dfn-size for me.

Does the latter include also result in <code> being included? Anyway, the former is a shorthand that should do the right thing and it's useful to do it consistently within a document so you can easily find all instances.

Now having said all that, typically you would never want to reference an IDL attribute as it's the public API. The problem here is that Blob still doesn't define its internal model.

Does the latter include also result in <code> being included?

Ah! No it does not.

Anyway, the former is a shorthand that should do the right thing and it's useful to do it consistently within a document so you can easily find all instances.

This makes a lot of sense.

annevk · 2022-12-01T11:59:30Z

fetch.bs

+
+       <li><p>Let <var>slicedLength</var> be <var>slicedBlob</var>'s
+       <a for=Blob data-link-type=attribute>size</a>, <a lt="serialize an integer">serialized</a>
+       and <a>isomorphic encoded</a>.


Here we could still refer to the length of the body (that's the result of extracting), but I'm on the fence as to whether that's better. In any event we should use {{Blob/size}} until we can reference some internal field on Blob.

dlrobertson · 2022-12-03T01:03:53Z

Updated to follow your suggestions. Let me know if there is anything else you'd like changed!

annevk

Modulo a couple tiny nits this looks good.

Are Chromium and WebKit passing all the tests or should there be bugs on them as well?

annevk · 2022-12-08T11:21:19Z

fetch.bs

@@ -4648,21 +4648,107 @@ steps:
      <p class=note>The `<code>GET</code>` <a for=/>method</a> restriction serves no useful purpose
      other than being interoperable.

-     <li><p>Let <var>bodyWithType</var> be the result of <a for=BodyInit>safely extracting</a>
-     <var>blobURLEntry</var>'s <a for="blob URL entry">object</a>.
+     <li><p>Set <var>response</var> to a new <a for=/>response</a>.


Suggested change

<li>Set <var>response</var> to a new <a for=/>response</a>.

<li>Let <var>response</var> be a new <a for=/>response</a>.

annevk · 2022-12-08T11:31:55Z

fetch.bs

+       <li><p>If <var>rangeValue</var>[0] is non-null, then
+       <a lt="serialize an integer">serialize</a> and
+       <a>isomorphic encode</a> <var>rangeValue</var>[0], and append the result to
+       <var>contentRange</var>.


Suggested change

<li>If <var>rangeValue</var>[0] is non-null, then

<a lt="serialize an integer">serialize</a> and

<a>isomorphic encode</a> <var>rangeValue</var>[0], and append the result to

<var>contentRange</var>.

<li>If <var>rangeValue</var>[0] is non-null, then

<a lt="serialize an integer">serialize</a> and <a>isomorphic encode</a>

<var>rangeValue</var>[0], and append the result to <var>contentRange</var>.

annevk · 2022-12-08T11:33:08Z

fetch.bs

+       <li><p>If <var>sliceEndRange</var> is non-null, then
+       <a lt="serialize an integer">serialize</a> and
+       <a>isomorphic encode</a> <var>sliceEndRange</var>, and append the result to
+       <var>contentRange</var>.


Suggested change

<li>If <var>sliceEndRange</var> is non-null, then

<a lt="serialize an integer">serialize</a> and

<a>isomorphic encode</a> <var>sliceEndRange</var>, and append the result to

<var>contentRange</var>.

<li>If <var>sliceEndRange</var> is non-null, then

<a lt="serialize an integer">serialize</a> and <a>isomorphic encode</a>

<var>sliceEndRange</var>, and append the result to <var>contentRange</var>.

dlrobertson · 2022-12-08T21:46:09Z

Are Chromium and WebKit passing all the tests or should there be bugs on them as well?

Good question! Both fail most of the upsupported range cases tested in web-platform-tests/wpt#34384. WebKit returns a response with a 416 status when the range is invalid and Chromium only fails 3 of the unsupported ranges, but does return a network error. We'll need to create bugs for this I guess.

dlrobertson · 2022-12-09T01:19:23Z

Filed the bugs:

WebKit: https://bugs.webkit.org/show_bug.cgi?id=248994
Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1399678

I definitely didn't file the chromium bug right... any feedback on filing non-firefox implementation bugs would be appreciated

annevk · 2022-12-09T08:25:51Z

I was about to merge this, but then read #1070 (comment) again. I think we should address the whitespace issue first, perhaps in a separate PR to constrain it to CORS?

dlrobertson · 2022-12-09T13:44:15Z

No problem! I'll take a stab at it this evening

annevk · 2023-02-17T13:48:44Z

fetch.bs

+            https://github.com/whatwg/fetch/issues/1553 for future work -->
+
+       <li><p>Let <var>rangeValue</var> be the result of <a>parsing a single range header value</a>
+       given <var>rangeHeader</var>.


Suggested change

given <var>rangeHeader</var>.

given <var>rangeHeader</var> and true.

I think this is the only change needed here. But if you could gloss it over one last time that would be appreciated.

I suspect we'll land this somewhere next week given the Bikeshed issues.

👍 I'll re-read and post an update this weekend

Implement support for blob range requests.

No normative changes. This is a pre-requisite for #1520.

For whatwg/fetch#1520.

dlrobertson · 2023-02-21T00:11:12Z

Thanks @annevk for all the help on this!

Automatic update from web-platform-tests Fetch: blob: URL range requests For whatwg/fetch#1520. -- wpt-commits: edc7cac3a37e9478fa301fe4c001bcaa3d79c62e wpt-pr: 34384

For whatwg/fetch#1520.

Automatic update from web-platform-tests Fetch: blob: URL range requests For whatwg/fetch#1520. -- wpt-commits: edc7cac3a37e9478fa301fe4c001bcaa3d79c62e wpt-pr: 34384

annevk reviewed Nov 3, 2022

View reviewed changes

dlrobertson force-pushed the blob-range-req branch from 811e0db to b1686f0 Compare November 4, 2022 02:21

dlrobertson force-pushed the blob-range-req branch from b1686f0 to 55b7dc9 Compare November 23, 2022 15:33

dlrobertson commented Nov 23, 2022

View reviewed changes

fetch.bs Show resolved Hide resolved

annevk reviewed Nov 24, 2022

View reviewed changes

dlrobertson force-pushed the blob-range-req branch from 55b7dc9 to ccdf858 Compare November 24, 2022 17:44

dlrobertson commented Nov 24, 2022

View reviewed changes

fetch.bs Show resolved Hide resolved

fetch.bs Show resolved Hide resolved

annevk reviewed Nov 25, 2022

View reviewed changes

This was referenced Nov 26, 2022

Editorial: slice blob should use nullable args w3c/FileAPI#184

Merged

Move Content-Range header builder to separate algorithm #1552

Closed

Create Range header section in HTTP extensions section #1553

Open

dlrobertson force-pushed the blob-range-req branch from ccdf858 to bdb0211 Compare November 27, 2022 02:31

dlrobertson force-pushed the blob-range-req branch from bdb0211 to 0573367 Compare December 1, 2022 02:36

annevk reviewed Dec 1, 2022

View reviewed changes

dlrobertson force-pushed the blob-range-req branch from 0573367 to 7bae1c4 Compare December 3, 2022 01:02

annevk reviewed Dec 8, 2022

View reviewed changes

dlrobertson force-pushed the blob-range-req branch from 7bae1c4 to 94bb82f Compare December 9, 2022 01:40

annevk added the do not merge yet Pull request must not be merged per rationale in comment label Feb 8, 2023

annevk reviewed Feb 17, 2023

View reviewed changes

annevk mentioned this pull request Feb 17, 2023

Add HTTP OWS handling to single range header parsing #1564

Merged

Add support for blob range requests

e330519

Implement support for blob range requests.

dlrobertson force-pushed the blob-range-req branch from 94bb82f to e330519 Compare February 18, 2023 18:45

annevk pushed a commit that referenced this pull request Feb 20, 2023

Add HTTP OWS handling to single range header parsing

e4f6c00

No normative changes. This is a pre-requisite for #1520.

annevk removed the do not merge yet Pull request must not be merged per rationale in comment label Feb 20, 2023

annevk merged commit dc0f9e7 into whatwg:main Feb 20, 2023

annevk mentioned this pull request Feb 20, 2023

Fetch: blob: URL range requests mdn/content#24625

Open

annevk added the addition/proposal New features or enhancements label Feb 20, 2023

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 20, 2023

Fetch: blob: URL range requests

edc7cac

For whatwg/fetch#1520.

dlrobertson deleted the blob-range-req branch February 21, 2023 00:10

annevk mentioned this pull request Mar 21, 2023

Correct blob: URL range requests #1621

Merged

3 tasks

marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 28, 2023

Fetch: blob: URL range requests

0de2cb0

For whatwg/fetch#1520.

	set <var>response</var>'s <a for=response>range-requested flag</a>.
	then set <var>response</var>'s <a for=response>range-requested flag</a>.

		<p>If <var>response</var>'s <a for=response>range-requested flag</a> is unset, run the
		following steps:

		<p>Otherwise, if <var>response</var>'s <a for=response>range-requested flag</a> is set, run the
		following steps:

	<p>Otherwise, if <var>response</var>'s <a for=response>range-requested flag</a> is set, run the
	following steps:
	<p>Otherwise:

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

	<li><p> return <var>response</var>.
	<li><p>Return <var>response</var>.

	<code><var>rangeValue</var>[1] + 1</code>.
	<var>rangeValue</var>[1] + 1.

		<li><p>If <var>request</var>'s <a for=response>header list</a> contains `<code>Range</code>`,
		then set <var>response</var>'s <a for=response>range-requested flag</a>.

	<li><p>Set <var>response</var> to a new <a for=/>response</a>.
	<li><p>Let <var>response</var> be a new <a for=/>response</a>.

	given <var>rangeHeader</var>.
	given <var>rangeHeader</var> and true.

Add support for blob range requests #1520

Add support for blob range requests #1520

Conversation

dlrobertson commented Oct 29, 2022 • edited by annevk Loading

dlrobertson commented Oct 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlrobertson commented Nov 23, 2022

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlrobertson left a comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlrobertson commented Nov 29, 2022

dlrobertson commented Dec 1, 2022

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlrobertson Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlrobertson commented Dec 3, 2022

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlrobertson commented Dec 8, 2022

dlrobertson commented Dec 9, 2022

annevk commented Dec 9, 2022

dlrobertson commented Dec 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlrobertson commented Feb 21, 2023

dlrobertson commented Oct 29, 2022 •

edited by annevk

Loading

dlrobertson Dec 1, 2022 •

edited

Loading