Define Content-Type manipulation in terms of MIME Sniffing by annevk · Pull Request #176 · whatwg/xhr

annevk · 2017-12-05T14:42:16Z

Note that this also replaces "utf-8" with "UTF-8" and no longer preserves duplicate parameters.

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

Preview | Diff

annevk · 2017-12-05T14:48:09Z

~~Note that this builds on whatwg/mimesniff#36 which has not landed yet.~~

annevk · 2017-12-05T14:48:52Z

It also seems like a few combinations might not be tested well here. Such as when mimeType or encoding is null.

domenic · 2018-02-05T14:01:09Z

-
-  <!-- reminder: if we ever change this to always include charset it has
-  to be included as the first parameter for compatibility reasons -->
+  <p>Otherwise, if <var>encoding</var> is non-null, run these steps:


What if mimeType is null but author request headers does not contain a Content-Type header?

domenic · 2018-02-05T14:02:08Z

+  <ol>
+   <li><p>Let <var>value</var> be the <a for=header>value</a> of the <a>header</a> whose
+   <a for=header>name</a> is a <a>byte-case-insensitive</a> match for `<code>Content-Type</code>` in
+   <a>author request headers</a>.


I'm surprised header list doesn't have a "get" operation that does the byte-case-insensitive part for you. It has so many of the other operations you'd expect.

I'd like to still get this fixed because it contrasts with "contains" very strongly. Reading without clicking links might lead one to say that this operation is byte-case-insensitive but the above check is case-sensitive.

domenic · 2018-02-05T14:03:25Z

+   <a>author request headers</a>.
+
+   <li><p>Let <var>mimeTypeRecord</var> be <var>value</var>,
+   <a for="parse a MIME type from bytes">parsed</a>.


This (and serialized) isn't linking in the previous but probably that's because this is an old PR. Worth confirming before merging.

domenic · 2018-02-05T14:03:49Z

+
+   <li>
+    <p>If <var>mimeTypeRecord</var> is not failure and
+    <var>mimeTypeRecord</var>["<code>charset</code>"] is not nothing, then:


This should be referencing the record's parameters, right?

And using "exists" for ordered maps, not "is not nothing"

Note that this also replaces "utf-8" with "UTF-8" and no longer preserves duplicate parameters. Tests: web-platform-tests/wpt#8422.

annevk · 2018-04-10T12:37:29Z

Thanks for the review and sorry for the slow turnaround. Really want to start closing all these minor MIME type issues everywhere, though I'm sure it'll take longer than anticipated.

See whatwg/xhr#176 and whatwg/mimesniff#36.

annevk · 2018-04-10T13:14:20Z

For commit message: fixes #188.

Needed for whatwg/xhr#176.

domenic

Looks pretty good although as noted I really think we should add "get" to headers list, preferably before merging this or at least very soon after.

I still find this algorithm a bit confusing to try to follow by hand but I'll noodle on whether we can do anything better in a follow-up.

For tests it'd be worth testing everything in the MIME types JSON file, I think, to make sure that we're using the same parser here as everywhere else on the platform.

domenic · 2018-04-10T17:30:59Z

+  <ol>
+   <li><p>Let <var>value</var> be the <a for=header>value</a> of the <a>header</a> whose
+   <a for=header>name</a> is a <a>byte-case-insensitive</a> match for `<code>Content-Type</code>` in
+   <a>author request headers</a>.


I'd like to still get this fixed because it contrasts with "contains" very strongly. Reading without clicking links might lead one to say that this operation is byte-case-insensitive but the above check is case-sensitive.

annevk · 2018-04-10T17:40:28Z

Getting header values is a can of worms. Currently Fetch only has accessors that also parse and I'm really not sure what the right approach is here yet.

domenic · 2018-04-10T17:41:44Z

I mean it seems like if you have "contains" you really should have an analogous "get". It should just return the byte sequence.

annevk · 2018-04-10T17:41:51Z

I'm not quite sure how it'd make sense to test all those MIME types here? There's far less observable bits.

annevk · 2018-04-10T17:42:08Z

@domenic but that's likely wrong given multiple headers and such, no?

domenic · 2018-04-10T17:48:47Z

I'm not sure why it would be. You already have an analogous operation you are using:

Let value be the value of the header whose name is a byte-case-insensitive match for Content-Type in author request headers.

What's wrong with just defining it that way?

As for testing all the MIME types, I was thinking, set Content-Type/the value from the JSON file for every value in the JSON file. Then, check that the resulting Content-Type the server sees is the same as the output from the JSON file, except all charset parameters must be changed to UTF-8 (so do something like s/charset=[A-Z-]+/charset=UTF-8).

Main issues fixed

annevk · 2018-04-10T18:05:06Z

@domenic that phrasing works here since setRequestHeader() aggressively combines, but a definition for "header list" would need to take other scenarios into account.

I'll see about reusing the MIME type parsing tests, perhaps at least the navigable ones can be used here (with an encoding).

domenic · 2018-04-10T18:22:01Z

You could also use all the others plus a null body (or any non-Document non-string body) to make sure they aren't being parsed, i.e. are just being passed through without changes.

annevk · 2018-04-11T08:08:18Z

You could also use all the others plus a null body (or any non-Document non-string body) to make sure they aren't being parsed, i.e. are just being passed through without changes.

It seems that can be confirmed with a single test (and I strongly suspect already is). (Edit: I had already written a test for this.)

annevk · 2018-04-11T09:09:57Z

I don't really see an easy way to reuse https://github.com/w3c/web-platform-tests/blob/master/mimesniff/mime-types/resources/mime-types.json here other than adding more fields to it. Otherwise there's a lot of post-processing of the kind you'd then object to. I think we should just go ahead here and maybe file that as follow-up if anyone is interested in doing the work.

domenic · 2018-04-11T12:48:58Z

That seems reasonable. I think it's the right strategy, but we don't want to burden spec maintainers with comprehensive tests and block all forward spec progress until they're done.

The reason I prefer being more comprehensive than a single test is that if a browser parses-then-reserializes using some XHR-specific parser, a single test might not catch a difference, depending on the algorithm.

annevk · 2018-04-16T09:50:41Z

See whatwg/xhr#176 and whatwg/mimesniff#36.

…ng, a=testonly Automatic update from web-platform-testsAdjust XMLHttpRequest Content-Type handling See whatwg/xhr#176 and whatwg/mimesniff#36. -- wpt-commits: 84e7972a0518fb57f39740143d4b63e79b14e9f4 wpt-pr: 8422

…ng, a=testonly Automatic update from web-platform-testsAdjust XMLHttpRequest Content-Type handling See whatwg/xhr#176 and whatwg/mimesniff#36. -- wpt-commits: 84e7972a0518fb57f39740143d4b63e79b14e9f4 wpt-pr: 8422 UltraBlame original commit: be93580d93e3b6e94946124f06c188bc8daac745

…ng, a=testonly Automatic update from web-platform-testsAdjust XMLHttpRequest Content-Type handling See whatwg/xhr#176 and whatwg/mimesniff#36. -- wpt-commits: 84e7972a0518fb57f39740143d4b63e79b14e9f4 wpt-pr: 8422

annevk mentioned this pull request Dec 7, 2017

Clarify getting a key that does not exist from a map whatwg/infra#179

Open

annevk mentioned this pull request Feb 5, 2018

Fix send() charset overriding to use new MIME type infrastructure #188

Closed

domenic previously requested changes Feb 5, 2018

View reviewed changes

annevk added 2 commits April 10, 2018 14:28

Define Content-Type manipulation in terms of MIME Sniffing

3ce0d2a

Note that this also replaces "utf-8" with "UTF-8" and no longer preserves duplicate parameters. Tests: web-platform-tests/wpt#8422.

address feedback

bfc49d8

annevk force-pushed the annevk/content-type-manipulation branch from 5079f6f to bfc49d8 Compare April 10, 2018 12:35

annevk requested a review from domenic April 10, 2018 12:36

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

Adjust XMLHttpRequest Content-Type handling

6a721f9

See whatwg/xhr#176 and whatwg/mimesniff#36.

annevk added a commit to whatwg/mimesniff that referenced this pull request Apr 10, 2018

Meta: export MIME type bits

68c85a3

Needed for whatwg/xhr#176.

annevk mentioned this pull request Apr 10, 2018

Meta: export MIME type bits whatwg/mimesniff#66

Merged

annevk added a commit to whatwg/mimesniff that referenced this pull request Apr 10, 2018

Meta: export MIME type bits

5dac5da

Needed for whatwg/xhr#176.

annevk added 2 commits April 10, 2018 16:53

nits

bd03ede

ugh

a85a8e9

domenic reviewed Apr 10, 2018

View reviewed changes

domenic mentioned this pull request Apr 10, 2018

Editorial: less-confusing content-type manipulation algorithm for send() #197

Closed

annevk merged commit edc6f8f into master Apr 16, 2018

annevk deleted the annevk/content-type-manipulation branch April 16, 2018 09:51

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

Adjust XMLHttpRequest Content-Type handling

84e7972

See whatwg/xhr#176 and whatwg/mimesniff#36.

annevk mentioned this pull request Oct 30, 2018

Limit Content-Type overrides to when charset isn't already UTF-8 #224

Merged

Conversation

annevk commented Dec 5, 2017 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

annevk commented Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

annevk commented Dec 5, 2017

Uh oh!

domenic Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

domenic Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

domenic Apr 10, 2018

Choose a reason for hiding this comment

Uh oh!

domenic Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

domenic Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

annevk commented Apr 10, 2018

Uh oh!

annevk commented Apr 10, 2018

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

domenic Apr 10, 2018

Choose a reason for hiding this comment

Uh oh!

annevk commented Apr 10, 2018

Uh oh!

domenic commented Apr 10, 2018

Uh oh!

annevk commented Apr 10, 2018

Uh oh!

annevk commented Apr 10, 2018

Uh oh!

domenic commented Apr 10, 2018

Uh oh!

annevk commented Apr 10, 2018

Uh oh!

domenic commented Apr 10, 2018

Uh oh!

annevk commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

annevk commented Apr 11, 2018

Uh oh!

domenic commented Apr 11, 2018

Uh oh!

annevk commented Apr 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

annevk commented Dec 5, 2017 •

edited by pr-preview bot

Loading

annevk commented Dec 5, 2017 •

edited

Loading

annevk commented Apr 11, 2018 •

edited

Loading