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 the Content-Type header parser #831

Merged
merged 17 commits into from Nov 27, 2018

Conversation

4 participants
@annevk
Copy link
Member

commented Nov 9, 2018

Also known as "extract a MIME type" down right.

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

Helps with #814.

Fixes #529. Closes whatwg/mimesniff#30.


Preview | Diff

Define the response Content-Type header parser
Also known as "extract a MIME type" down right.

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

Helps with #814.

Fixes #529. Closes whatwg/mimesniff#30.

@annevk annevk force-pushed the annevk/response-content-type branch from e259098 to a28d42c Nov 12, 2018

Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs Outdated
@domenic
Copy link
Member

left a comment

Some clarity suggestions. Very exciting stuff.

Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs
Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs
Show resolved Hide resolved fetch.bs
Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs
@domenic

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

Oh, also, should the new Content-Type header section mention that "extract a MIME type" is meant only for use on responses? Maybe the algorithm name should be more specific, e.g. "parse the response Content-Type header"?

@annevk

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2018

My plan was that we use the algorithm for Content-Type for both requests and responses, but have a stricter parser solely for CORS purposes. It seemed better for Request and Response in particular to not deviate here in how they determine the MIME type for a Blob.

@annevk annevk changed the title Define the response Content-Type header parser Define the Content-Type header parser Nov 13, 2018

annevk added some commits Nov 13, 2018

annevk added a commit to whatwg/xhr that referenced this pull request Nov 13, 2018

Extract a MIME type no longer returns bytes
This aligns with the changes made in whatwg/fetch#831. Fortunately the remainder of the prose already assumed it got a MIME type record rather than bytes.
@annevk

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2018

@MattMenke2 @bzbarsky any final thoughts here? I realize this doesn't match browsers 100% due to eager combining, but it feels like the only sensible model to specify given that intermediaries are allowed to combine. Hopefully given enough time browsers can converge on this model. And if they can't, we can revisit…

Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs
@bzbarsky

This comment has been minimized.

Copy link

commented Nov 13, 2018

I don't really understand this space well enough at this point to comment intelligently...

annevk added a commit to whatwg/mimesniff that referenced this pull request Nov 14, 2018

@annevk

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2018

@MattMenke2 I created an algorithm for collecting an HTTP quoted string based on the MIME type parser (and also created whatwg/mimesniff#92 to use it there). It's a little ugly as here we care about moving the position only whereas in the MIME type parser we also care about extracting a value, but this seemed like a reasonable compromise that would not lead to code duplication. (I'd be open to changing the algorithm to take a boolean to return one or the other value in the end, rather than both.)

@annevk

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2018

There's now additional tests for escaped quotes as well.

@MattMenke2
Copy link

left a comment

This all looks pretty good to me.

Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs
Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs Outdated
@annevk

This comment has been minimized.

Copy link
Member Author

commented Nov 19, 2018

Thanks for asking about value, that simplified things quite a bit.

@MattMenke2
Copy link

left a comment

Latest changes look good. If you want to deal with my suggest format changes later (Or leave things as-is), that's fine with me, though I do think the current format is very hard to follow.

@annevk

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2018

Yeah, I agree https://whatpr.org/fetch/831.html#example-header-list-get-decode-split doesn't look good. I filed whatwg/infra#228 to discuss it.

I'll let @domenic do a review pass as well and will attempt to bring the formatting issue to a satisfactory resolution before landing.

Show resolved Hide resolved fetch.bs
Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs
Show resolved Hide resolved fetch.bs
Show resolved Hide resolved fetch.bs
Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs
Show resolved Hide resolved fetch.bs

annevk added some commits Nov 21, 2018

@domenic
Copy link
Member

left a comment

Really great, thanks for all the iteration! Only a couple minor suggestions.

Show resolved Hide resolved fetch.bs
Show resolved Hide resolved fetch.bs Outdated

@annevk annevk merged commit 0b2bc05 into master Nov 27, 2018

2 checks passed

Participation annevk participates on behalf of Mozilla Corporation
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@annevk annevk deleted the annevk/response-content-type branch Nov 27, 2018

annevk added a commit to web-platform-tests/wpt that referenced this pull request Nov 27, 2018

annevk added a commit to whatwg/mimesniff that referenced this pull request Nov 28, 2018

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 11, 2018

Bug 1454984 [wpt PR 10525] - Parsing Content-Type, a=testonly
Automatic update from web-platform-tests
Fetch: Content-Type parsing

See whatwg/fetch#831 for context.
--

wpt-commits: 62317fb983ca5687e4133d89f5523839fdab7f69
wpt-pr: 10525

xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Dec 11, 2018

Bug 1454984 [wpt PR 10525] - Parsing Content-Type, a=testonly
Automatic update from web-platform-tests
Fetch: Content-Type parsing

See whatwg/fetch#831 for context.
--

wpt-commits: 62317fb983ca5687e4133d89f5523839fdab7f69
wpt-pr: 10525

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 11, 2018

Bug 1454984 [wpt PR 10525] - Parsing Content-Type, a=testonly
Automatic update from web-platform-tests
Fetch: Content-Type parsing

See whatwg/fetch#831 for context.
--

wpt-commits: 62317fb983ca5687e4133d89f5523839fdab7f69
wpt-pr: 10525

mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Dec 12, 2018

Bug 1454984 [wpt PR 10525] - Parsing Content-Type, a=testonly
Automatic update from web-platform-tests
Fetch: Content-Type parsing

See whatwg/fetch#831 for context.
--

wpt-commits: 62317fb983ca5687e4133d89f5523839fdab7f69
wpt-pr: 10525

annevk added a commit to whatwg/xhr that referenced this pull request Mar 24, 2019

Extract a MIME type no longer returns bytes
This aligns with the changes made in whatwg/fetch#831. Fortunately the remainder of the prose already assumed it got a MIME type record rather than bytes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.