According to the latest XMLHttpRequest specification (http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#dom-xmlhttprequest-overridemimetype): The overrideMimeType(mime) throws an "InvalidStateError" exception if the state is LOADING or DONE. The current implementation never throws.
Created attachment 162015 [details] Patch
Comment on attachment 162015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162015&action=review > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-overridemimetype-invalid-state.html:15 > + finishJSTest(); Is it OK to finish the test here? (I'm not sure why this test proceeds to xhr.readyState == xhr.Done.) > Source/WebCore/ChangeLog:11 > + This behavior is according to the latest XMLHttpRequest specification: Would you check the behavior of other browsers (Firefox, IE, Opera)? You need to take care of compatibility when you try to throw an exception for the code that has not thrown an exception.
Comment on attachment 162015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162015&action=review >> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-overridemimetype-invalid-state.html:15 >> + finishJSTest(); > > Is it OK to finish the test here? (I'm not sure why this test proceeds to xhr.readyState == xhr.Done.) Yeah it could just return, or the next if could be an else if
(In reply to comment #3) > >> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-overridemimetype-invalid-state.html:15 > >> + finishJSTest(); > > > > Is it OK to finish the test here? (I'm not sure why this test proceeds to xhr.readyState == xhr.Done.) > > Yeah it could just return, or the next if could be an else if I'm sorry if I'm wrong: (1) xhr.onreadystatechange is invoked. (2) if (xhr.readyState == xhr.LOADING) calls finishJSTest(). (3) finishJSTest() calls testRunner.notifyDone(), which tells the testRunner that the asynchronous test has finished. (4) xhr.onreadystatechange is invoked. (5) if (xhr.readyState == xhr.DONE) calls finishJSTest(). My question is why (4) and (5) run, even though the test has already finished at the point of (3).
(In reply to comment #4) > (In reply to comment #3) > > >> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-overridemimetype-invalid-state.html:15 > > >> + finishJSTest(); > > > > > > Is it OK to finish the test here? (I'm not sure why this test proceeds to xhr.readyState == xhr.Done.) > > > > Yeah it could just return, or the next if could be an else if > > I'm sorry if I'm wrong: > > (1) xhr.onreadystatechange is invoked. > (2) if (xhr.readyState == xhr.LOADING) calls finishJSTest(). > (3) finishJSTest() calls testRunner.notifyDone(), which tells the testRunner that the asynchronous test has finished. > (4) xhr.onreadystatechange is invoked. > (5) if (xhr.readyState == xhr.DONE) calls finishJSTest(). > > My question is why (4) and (5) run, even though the test has already finished at the point of (3). You are right, of course it should not finish before everything has been tested.
(In reply to comment #4) > (In reply to comment #3) > > >> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-overridemimetype-invalid-state.html:15 > > >> + finishJSTest(); > > > > > > Is it OK to finish the test here? (I'm not sure why this test proceeds to xhr.readyState == xhr.Done.) > > > > Yeah it could just return, or the next if could be an else if > > I'm sorry if I'm wrong: > > (1) xhr.onreadystatechange is invoked. > (2) if (xhr.readyState == xhr.LOADING) calls finishJSTest(). > (3) finishJSTest() calls testRunner.notifyDone(), which tells the testRunner that the asynchronous test has finished. > (4) xhr.onreadystatechange is invoked. > (5) if (xhr.readyState == xhr.DONE) calls finishJSTest(). > > My question is why (4) and (5) run, even though the test has already finished at the point of (3). Yes, my bad, the finishJSTest() for LOADING case should not be there. I'll remove it. I have checked with Firefox and Opera. None of them throw for this case. I don't have access to IE to test it.
(In reply to comment #6) > I have checked with Firefox and Opera. None of them throw for this case. > I don't have access to IE to test it. Then you might want to fix the spec instead. Throwing an exception for the code that has not thrown an exception sounds dangerous. Let's hear opinions of XHR experts.
Created attachment 162027 [details] Patch Fixed the LayoutTest as reported by Haraken.
For your information: The old spec (http://www.w3.org/TR/2010/WD-XMLHttpRequest2-20100907/#the-overridemimetype-method) did not indicate that we should throw for this case. This was corrected later in: http://www.w3.org/TR/2011/WD-XMLHttpRequest2-20110816/#the-overridemimetype-method And the latest working draft still indicates we should throw: http://www.w3.org/TR/2012/WD-XMLHttpRequest-20120117/#the-overridemimetype-method
Can you find working group discussion that led to this change? We need to somehow verify that this change would be good for WebKit users.
I added James Robinson in CC since it seems he proposed the throwing behavior in: http://lists.w3.org/Archives/Public/public-webapps/2010OctDec/0022.html This thread seems related as well: http://lists.w3.org/Archives/Public/public-webapps/2011AprJun/0261.html
Third related thread: http://lists.w3.org/Archives/Public/public-webapps/2011OctDec/0883.html This relates to the following change in the latest spec: client . overrideMimeType(mime) Sets the Content-Type header for the response to mime. - Throws an INVALID_STATE_ERR exception if the state is not OPENED or HEADERS_RECEIVED. + Throws an "InvalidStateError" exception if the state is LOADING or DONE.
It's curious that the discussion seems to have revolved around charset only. That case is undoubtedly unfortunate, but what about changing MIME type in order to use responseXML? That should be fine to do later.
Comment on attachment 162027 [details] Patch Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.
This was fixed in https://bugs.webkit.org/show_bug.cgi?id=136699.
As stated in Comment 15, I think this is fixed.