Add more XMLHttpRequest overrideMimeType() tests #8449

annevk · 2017-11-27T13:06:48Z

For whatwg/xhr#157 and whatwg/mimesniff#42.

ghost · 2017-11-27T13:13:56Z

Build PASSED

Started: 2017-12-04 14:39:08
Finished: 2017-12-04 14:43:15

View more information about this build on:

annevk · 2017-11-28T14:35:03Z

This is ready for review now. This is what is remaining:

Merging of Revamp MIME type section whatwg/mimesniff#36 (the new MIME type specification)
~~Usage of that MIME type specification in the XMLHttpRequest Standard~~

Reset override MIME type when open() is invoked. Actually make overriding the charset parameter work (setting override MIME type to the "MIME type portion" (now essence) as done previously would always erase it. Tests: web-platform-tests/wpt#8449.

For whatwg/xhr#157 and whatwg/mimesniff#42.

annevk · 2018-04-11T13:28:06Z

This could use review now.

domenic

LGTM with comments

domenic · 2018-04-12T21:53:41Z

xhr/overridemimetype-blob.html

@@ -6,60 +6,67 @@
 <div id="log"></div>
 <script>
 async_test(t => {
-  const client = new XMLHttpRequest()
+  const client = new XMLHttpRequest();


This is extraordinarily hard to review because almost every line has changed, but I guess only because of semicolons? Please split up the commits at least next time.

Sorry, I figured it wouldn't be much of an issue given how GitHub highlights stuff.

domenic · 2018-04-12T21:55:21Z

xhr/overridemimetype-blob.html

+  client.open("GET", "resources/status.py?content=thisshouldnotmakeadifferencebutdoes");
+  client.responseType = "blob";
+  client.send();
+}, "Use text/xml as fallback MIME type, 2");


I don't understand why the above two tests are in a file related to overrideMimeType()?

No good reason.

domenic · 2018-04-12T21:56:15Z

xhr/overridemimetype-blob.html

+    client.open("GET", "resources/status.py");
+    client.responseType = "blob";
+    client.overrideMimeType(type);
+  client.send();


Nit: whitespace is off here

domenic · 2018-04-12T22:00:13Z

xhr/overridemimetype-blob.html

+ ["text/html;", "text/html"],
+ ["x/x;?=x;charset=y", "x/x;charset=y"],
+ ["text/html;x=ÿ", "text/html;x=\"ÿ\""]
+].forEach(([inputType, outputType], index) => {


Seems like you could use the whole JSON file for the above two tests. (If the JSON files says a parse failure, application/octet-stream; otherwise use the output.) As I've argued previously, this helps ensure that the same parser is being used in all parts of the codebase.

Reset override MIME type when open() is invoked. Actually make overriding the charset parameter work (setting override MIME type to the "MIME type portion" (now essence) as done previously would always erase it. Tests: web-platform-tests/wpt#8449.

annevk · 2018-04-16T11:32:55Z

I've made the change of using the smaller JSON resource here (since it's networking it doesn't seem good to use all), but I need to verify whether some tests should be added to that JSON resource resource that I've removed here or whether coverage remained equivalent. I should probably also have another look at code point usage just in case.

ghost · 2018-04-16T11:36:10Z

Build PASSED

Started: 2018-04-16 11:32:46
Finished: 2018-04-16 11:37:29

View more information about this build on:

This makes a number of changes: * Integrates with the new MIME Sniffing infrastructure. * Reset override MIME type when open() is invoked. * Actually make overriding the charset parameter work (setting override MIME type to the "MIME type portion" (now essence) as done previously would always erase it). * Use "get an encoding" (previously undefined) to convert from a label to an encoding. Tests: web-platform-tests/wpt#8449. Fixes #157.

thw0rted · 2018-09-27T12:42:31Z

Can I suggest a test here? I don't see any tests to confirm that the browser follows step 12 of the updated spec for open(). I just got bit by this behavior -- Edge respects the direction to reset the override MIME type while apparently all other modern browsers do not (!). So, it would be helpful to have a test for:

make XHR for a resource with MIME type type1
call overrideMimeType("type2")
call open
verify that response has MIME type type1 not type2.

ETA: if this merits a new issue, I'd be happy to open one.

ETA Again: just found #12404, please disregard.

wisniewskit · 2018-09-27T15:04:01Z

@thworted, yeah, it turned out that resetting the MIME type wasn't web compatible (or at least that's how I read the outcome of the investigation in #12289).

thw0rted · 2018-09-28T07:21:12Z

The worst part is that to be truly compatible, everybody will have to treat open as though it resets the MIME type or risk breaking if somebody has one of the affected builds of Edge, and given the way corporate environments work, that could be an issue for years to come.

annevk · 2018-09-28T15:11:25Z

Web developers should probably fix their servers to not send broken MIME types and migrate to fetch(). 😊

thw0rted · 2018-10-15T10:08:21Z

The first thing that comes to mind is XHR against a peer resource in a file:/// origin. Of course, a local file won't have a stored MIME type, as such, and while the browser (or OS?) might make a best-effort based on file extension, you aren't likely to have another place to step in and assert that you know better.

Maybe that's not compelling enough? Still, overrideMimeType exists and consistent behavior is in everybody's best interest.

This makes a number of changes: * Integrates with the new MIME Sniffing infrastructure. * Reset override MIME type when open() is invoked. * Actually make overriding the charset parameter work (setting override MIME type to the "MIME type portion" (now essence) as done previously would always erase it). * Use "get an encoding" (previously undefined) to convert from a label to an encoding. Tests: web-platform-tests/wpt#8449. Fixes #157.

wpt-pr-bot added the xhr label Nov 27, 2017

wpt-pr-bot requested review from caitp, emilio, hallvors, ibelem, jdm, jungkees, kangxu, Manishearth, mathiasbynens, ronkorving and wisniewskit November 27, 2017 13:06

annevk mentioned this pull request Nov 27, 2017

Sort out MIME type tests whatwg/mimesniff#42

Closed

4 tasks

annevk added do not merge yet status:needs-spec-decision labels Nov 27, 2017

annevk mentioned this pull request Dec 4, 2017

Fix overrideMimeType() again whatwg/xhr#174

Merged

annevk force-pushed the annevk/xhr-overridemimetype branch from 5768bb1 to bc47269 Compare April 10, 2018 12:18

Add more XMLHttpRequest overrideMimeType() tests

e7bc32b

For whatwg/xhr#157 and whatwg/mimesniff#42.

annevk force-pushed the annevk/xhr-overridemimetype branch from bc47269 to e7bc32b Compare April 10, 2018 16:27

test bogus charsets (also add a baseline test since we didn't have any)

d6b347a

annevk removed do not merge yet status:needs-spec-decision labels Apr 11, 2018

annevk requested a review from domenic April 11, 2018 13:28

domenic approved these changes Apr 12, 2018

View reviewed changes

replace last two tests with using the JSON resource

605f6a8

annevk added the do not merge yet label Apr 16, 2018

minor tweaks

df15684

wpt-pr-bot added the mimesniff label Apr 17, 2018

annevk removed the do not merge yet label Apr 17, 2018

annevk merged commit ae41496 into master Apr 17, 2018

annevk deleted the annevk/xhr-overridemimetype branch April 17, 2018 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more XMLHttpRequest overrideMimeType() tests #8449

Add more XMLHttpRequest overrideMimeType() tests #8449

annevk commented Nov 27, 2017 •

edited by wpt-pr-bot

Loading

ghost commented Nov 27, 2017 •

edited by ghost

Loading

annevk commented Nov 28, 2017 •

edited

Loading

annevk commented Apr 11, 2018

domenic left a comment

domenic Apr 12, 2018

annevk Apr 17, 2018

domenic Apr 12, 2018

annevk Apr 17, 2018

domenic Apr 12, 2018

domenic Apr 12, 2018

annevk commented Apr 16, 2018

ghost commented Apr 16, 2018 •

edited by ghost

Loading

thw0rted commented Sep 27, 2018 •

edited

Loading

wisniewskit commented Sep 27, 2018

thw0rted commented Sep 28, 2018

annevk commented Sep 28, 2018

thw0rted commented Oct 15, 2018

Add more XMLHttpRequest overrideMimeType() tests #8449

Add more XMLHttpRequest overrideMimeType() tests #8449

Conversation

annevk commented Nov 27, 2017 • edited by wpt-pr-bot Loading

ghost commented Nov 27, 2017 • edited by ghost Loading

Build PASSED

annevk commented Nov 28, 2017 • edited Loading

annevk commented Apr 11, 2018

domenic left a comment

Choose a reason for hiding this comment

domenic Apr 12, 2018

Choose a reason for hiding this comment

annevk Apr 17, 2018

Choose a reason for hiding this comment

domenic Apr 12, 2018

Choose a reason for hiding this comment

annevk Apr 17, 2018

Choose a reason for hiding this comment

domenic Apr 12, 2018

Choose a reason for hiding this comment

domenic Apr 12, 2018

Choose a reason for hiding this comment

annevk commented Apr 16, 2018

ghost commented Apr 16, 2018 • edited by ghost Loading

Build PASSED

thw0rted commented Sep 27, 2018 • edited Loading

wisniewskit commented Sep 27, 2018

thw0rted commented Sep 28, 2018

annevk commented Sep 28, 2018

thw0rted commented Oct 15, 2018

annevk commented Nov 27, 2017 •

edited by wpt-pr-bot

Loading

ghost commented Nov 27, 2017 •

edited by ghost

Loading

annevk commented Nov 28, 2017 •

edited

Loading

ghost commented Apr 16, 2018 •

edited by ghost

Loading

thw0rted commented Sep 27, 2018 •

edited

Loading