Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define Content-Length parser #1183

Merged
merged 2 commits into from Mar 3, 2021
Merged

Define Content-Length parser #1183

merged 2 commits into from Mar 3, 2021

Conversation

annevk
Copy link
Member

@annevk annevk commented Mar 1, 2021

As the body concept is refactored for #604, XMLHttpRequest will need to use this algorithm for its events. We also want to require browsers to use this algorithm instead of the one defined by HTTP as part of #1156.

Tests: web-platform-tests/wpt#10548.

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

@MattMenke2 I haven't attempted to actually require browsers use this yet. I suspect we want some kind of "HTTP monkey patching" section that outlines the various things we want browsers (and clients that emulate them) to implement for that. Ideas welcome, but it's not my main priority here (I'm trying to scope my yak shaving).


Preview | Diff

As the body concept is refactored for #604, XMLHttpRequest will need to use this algorithm for its events. We also want to require browsers to use this algorithm instead of the one defined by HTTP as part of #1156.

Tests: web-platform-tests/wpt#10548 & web-platform-tests/wpt#27837.
@annevk
Copy link
Member Author

annevk commented Mar 1, 2021

One could argue that we should only define this for a response. I don't think I have found a place where browsers use this for requests.

annevk added a commit that referenced this pull request Mar 1, 2021
Together with #1183 this solves the Fetch side of #604. Changes:

* No longer set a length for network responses. Callers will have to parse Content-Length themselves.
* Make extract set length so requests will set Content-Length correctly.

XMLHttpRequest PR: TODO.

Tests: web-platform-tests/wpt#27837.
Copy link
Contributor

@MattMenke2 MattMenke2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks good to me.

I'm not sure I'll have the time to fully dig through #1184, make sure it happily interacts with the chunked encoding bits, etc.

annevk added a commit to web-platform-tests/wpt that referenced this pull request Mar 2, 2021
annevk added a commit to whatwg/xhr that referenced this pull request Mar 2, 2021
The only information browsers use for progress events for responses is the Content-Length header.

Align on that and only use body's length concept for uploads.

Tests: web-platform-tests/wpt#27837.

Fetch PRs: whatwg/fetch#1183 & whatwg/fetch#1184.
annevk added a commit to whatwg/xhr that referenced this pull request Mar 2, 2021
The only information browsers use for progress events for responses is the Content-Length header.

Align on that and only use body's length concept for uploads.

Tests: web-platform-tests/wpt#27837.

Fetch PRs: whatwg/fetch#1183 & whatwg/fetch#1184.
@annevk
Copy link
Member Author

annevk commented Mar 3, 2021

I've decided to land this part and leave landing the additional tests and browser bugs to #1184 as this is fairly standalone.

@annevk annevk merged commit f2a7922 into main Mar 3, 2021
@annevk annevk deleted the annevk/content-length-parser branch March 3, 2021 16:20
annevk added a commit to whatwg/xhr that referenced this pull request Mar 3, 2021
The only information browsers use for progress events for responses is the Content-Length header.

Align on that and only use body's length concept for uploads.

Tests: web-platform-tests/wpt#27837.

Fetch PRs: whatwg/fetch#1183 & whatwg/fetch#1184.
@mnot
Copy link
Member

mnot commented Mar 3, 2021

When I run http://wpt.live/fetch/content-length/content-length.html in both FF and Safari, my browser doesn't fetch content-lengths.json, and only runs one test; wpt.fyi shows only one test result at https://wpt.fyi/results/fetch/content-length/content-length.html?label=master&label=experimental&aligned. What am I missing?

@MattMenke2
Copy link
Contributor

Hrm...You're right. Looks like there's nothing that loads parsing.window.js or too-long.window.js.

Anne: Did you forget to upload a file?

@annevk
Copy link
Member Author

annevk commented Mar 4, 2021

No, you want to run http://wpt.live/fetch/content-length/parsing.window.html and http://wpt.live/fetch/content-length/too-long.window.html for the added tests. .window.js (as well as .any.js) are shorthands to avoid having to write a bunch of boilerplate: https://web-platform-tests.org/writing-tests/testharness.html#without-html-boilerplate-window-js.

@mnot
Copy link
Member

mnot commented Mar 4, 2021

Aha - they are suddenly showing up now. Weird, they weren't before, even with the same commit; caching, maybe?

@annevk
Copy link
Member Author

annevk commented Mar 4, 2021

Ah, I think the results page is often behind a bit as it takes a while to run all the tests. So it only updates every so often.

@mnot
Copy link
Member

mnot commented Mar 4, 2021

Considering that you're really only showing interoperability between Blink-derived browsers, it's a bit premature to be saying that 'the model defined in HTTP is not compatible with web content', don't you think?

@annevk
Copy link
Member Author

annevk commented Mar 4, 2021

I suppose that Safari's results are closest to indicating that things could perhaps be more strict (although Safari is less strict than Chrome in many cases as well).

Perhaps what you're saying though is that https://httpwg.org/http-core/draft-ietf-httpbis-semantics-latest.html#rfc.section.8.6 doesn't contain many requirements on recipients so anything is fine? But even then, the model where recipients get to choose how to behave does not seem compatible as duplicate Content-Length headers are a thing.

@martinthomson
Copy link
Contributor

The reporting here is just odd. All the percent encoding, and no expectations. Do these have to be like this?

@annevk
Copy link
Member Author

annevk commented Mar 4, 2021

There's a variety of ways to do it, but this seemed nice in that you can see the exact input (albeit encoded).

@martinthomson
Copy link
Contributor

It doesn't say what the expectation is. And all the percent encoding makes it very hard to parse. Given that this test doesn't depend on the spacing, removing the encoding would be a start at making the output readable.

annevk added a commit to web-platform-tests/wpt that referenced this pull request Mar 4, 2021
@annevk
Copy link
Member Author

annevk commented Mar 4, 2021

Sure: web-platform-tests/wpt#27894. (The expectations are also part of the Message column, but it can be a bit cryptic if you don't know what you're looking for.)

annevk added a commit to web-platform-tests/wpt that referenced this pull request Mar 5, 2021
annevk added a commit that referenced this pull request Mar 10, 2021
Together with #1183 this solves the Fetch side of #604. Changes:

* No longer set a length for network responses. Callers will have to parse Content-Length themselves.
* Make extract set length so requests can set Content-Length correctly.

XMLHttpRequest PR: whatwg/xhr#317.

Tests: web-platform-tests/wpt#27837.
annevk added a commit to whatwg/xhr that referenced this pull request Mar 11, 2021
The only information browsers use for progress events for responses is the Content-Length header.

Align on that and only use body's length concept for uploads.

Tests: web-platform-tests/wpt#27837.

Fetch PRs: whatwg/fetch#1183 & whatwg/fetch#1184.

Closes whatwg/fetch#604.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 15, 2021
Automatic update from web-platform-tests
Fetch: parsing Content-Length

For whatwg/fetch#1183.
--

wpt-commits: 3a48a22074a41e7817a17dfbf5cb6c61277f8877
wpt-pr: 10548
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 15, 2021
…ames, a=testonly

Automatic update from web-platform-tests
Fetch: redo Content-Length parser test names

See whatwg/fetch#1183 for context.

--

wpt-commits: 34f0fc0f2d7e04c980e6ec708b590c0c220127e4
wpt-pr: 27894
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Mar 15, 2021
Automatic update from web-platform-tests
Fetch: parsing Content-Length

For whatwg/fetch#1183.
--

wpt-commits: 3a48a22074a41e7817a17dfbf5cb6c61277f8877
wpt-pr: 10548

UltraBlame original commit: 5c299636aa6340c1499f89538f61c57c9ea2c615
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Mar 15, 2021
…ames, a=testonly

Automatic update from web-platform-tests
Fetch: redo Content-Length parser test names

See whatwg/fetch#1183 for context.

--

wpt-commits: 34f0fc0f2d7e04c980e6ec708b590c0c220127e4
wpt-pr: 27894

UltraBlame original commit: b75ba7dbf5a92b46ca08d90e19e22af43647ef01
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Mar 15, 2021
Automatic update from web-platform-tests
Fetch: parsing Content-Length

For whatwg/fetch#1183.
--

wpt-commits: 3a48a22074a41e7817a17dfbf5cb6c61277f8877
wpt-pr: 10548

UltraBlame original commit: 5c299636aa6340c1499f89538f61c57c9ea2c615
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Mar 15, 2021
…ames, a=testonly

Automatic update from web-platform-tests
Fetch: redo Content-Length parser test names

See whatwg/fetch#1183 for context.

--

wpt-commits: 34f0fc0f2d7e04c980e6ec708b590c0c220127e4
wpt-pr: 27894

UltraBlame original commit: b75ba7dbf5a92b46ca08d90e19e22af43647ef01
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Mar 15, 2021
Automatic update from web-platform-tests
Fetch: parsing Content-Length

For whatwg/fetch#1183.
--

wpt-commits: 3a48a22074a41e7817a17dfbf5cb6c61277f8877
wpt-pr: 10548

UltraBlame original commit: 5c299636aa6340c1499f89538f61c57c9ea2c615
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Mar 15, 2021
…ames, a=testonly

Automatic update from web-platform-tests
Fetch: redo Content-Length parser test names

See whatwg/fetch#1183 for context.

--

wpt-commits: 34f0fc0f2d7e04c980e6ec708b590c0c220127e4
wpt-pr: 27894

UltraBlame original commit: b75ba7dbf5a92b46ca08d90e19e22af43647ef01
Bishwarupjee pushed a commit to Bishwarupjee/xhr that referenced this pull request Jan 31, 2024
The only information browsers use for progress events for responses is the Content-Length header.

Align on that and only use body's length concept for uploads.

Tests: web-platform-tests/wpt#27837.

Fetch PRs: whatwg/fetch#1183 & whatwg/fetch#1184.

Closes whatwg/fetch#604.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants