As illustrated in tet http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/overridemimetype-headers-received-state-force-shiftjis.htm, it may be convenient to use overrideMimeType at the time headers are received.
Created attachment 224504 [details] Patch
Comment on attachment 224504 [details] Patch Attachment 224504 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5856344070422528 New failing tests: http/tests/xmlhttprequest/xmlhttprequest-overridemimetype-content-type-header.html
Created attachment 224509 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 224504 [details] Patch Attachment 224504 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6037382042222592 New failing tests: http/tests/xmlhttprequest/xmlhttprequest-overridemimetype-content-type-header.html
Created attachment 224513 [details] Archive of layout-test-results from webkit-ews-08 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 224516 [details] Removing lowercasing of mimeTypeOverride
Created attachment 234404 [details] Rebasing patch
Comment on attachment 234404 [details] Rebasing patch View in context: https://bugs.webkit.org/attachment.cgi?id=234404&action=review Seems generally OK, please address the comments. > Source/WebCore/ChangeLog:9 > + Moved response encoding computation from didReceiveResponse to didReceiveData, just before the decoder is instantiated. > + This allows overrideMimeType to be changed within readystatechange event callback and have an impact on selected encoding. What does the spec say, and what do other browsers do? This bug only says that this would be convenient. > Source/WebCore/xml/XMLHttpRequest.cpp:1131 > + // FIXME: should we tie response "Content-Type" header and m_mimeTypeOverride no matter when is set m_mimeTypeOverride? I don't understand this FIXME. Could you please elaborate? > LayoutTests/ChangeLog:11 > + * http/tests/resources/status.php: Added. You add this script to root level resources directory, meaning that it should be useful for many tests. Can this script have a more descriptive name? But also, do we really need it? I find a custom .php script or an .asis file easier to read than something like ../resources/status.php?type="+encodeURIComponent('text/html;charset=UTF-8')+'&content=%83%65%83%58%83%67'). And it may even be possible to reuse one or our existing resources used for overrideMimeType testing. > LayoutTests/http/tests/resources/status.php:2 > + // Hack for PHP What does this hack achieve? Is there an explanation somewhere? Perhaps copying a comment from http/tests/eventsource/resources/response-content-type-charset.php would be helpful. > LayoutTests/http/tests/resources/status.php:14 > + $text = isset($_GET['text']) && $_GET["text"] ? $_GET["text"] : "OMG"; $text should probably be $status_text. > LayoutTests/http/tests/resources/status.php:19 > + header("X-Request-Method: " . (isset($_SERVER["REQUEST_METHOD"]) ? $_SERVER["REQUEST_METHOD"] : "NO")); When is $_SERVER["REQUEST_METHOD"] not set?
Thanks for the review. Please find some answers below. (In reply to comment #8) > (From update of attachment 234404 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=234404&action=review > > Seems generally OK, please address the comments. > > > Source/WebCore/ChangeLog:9 > > + Moved response encoding computation from didReceiveResponse to didReceiveData, just before the decoder is instantiated. > > + This allows overrideMimeType to be changed within readystatechange event callback and have an impact on selected encoding. > > What does the spec say, and what do other browsers do? Spec aligns with the patch (overrideMimeType can be changed in HEADERS status). Firefox is aligned with the spec. Chrome has the same code path as WebKit currently. > > Source/WebCore/xml/XMLHttpRequest.cpp:1131 > > + // FIXME: should we tie response "Content-Type" header and m_mimeTypeOverride no matter when is set m_mimeTypeOverride? > > I don't understand this FIXME. Could you please elaborate? The "Content-Type" header is updated with the overrideMimeType value in didReceiveResponse method. We may want to update its value in didReceiveData method as well, maybe in overrideMimeType method whenever we are in HEADERS_RECEIVED state. The FIXME is probably not very explicit and would be better placed in didReceiveData. > > LayoutTests/ChangeLog:11 > > + * http/tests/resources/status.php: Added. > > You add this script to root level resources directory, meaning that it should be useful for many tests. Can this script have a more descriptive name? > > But also, do we really need it? I find a custom .php script or an .asis file easier to read than something like ../resources/status.php?type="+encodeURIComponent('text/html;charset=UTF-8')+'&content=%83%65%83%58%83%67'). And it may even be possible to reuse one or our existing resources used for overrideMimeType testing. This test is a copy of a past W3C test. I will simplify the php script.
Created attachment 234482 [details] Updated php script and FIXME
Comment on attachment 234482 [details] Updated php script and FIXME View in context: https://bugs.webkit.org/attachment.cgi?id=234482&action=review > Source/WebCore/xml/XMLHttpRequest.cpp:1143 > + // FIXME: should we update "Content-Type" header with m_mimeTypeOverride value in case it has changed since didReceiveResponse? Please replace "header" with "header field", to make terminology correct. Also, we usually start FIXME comment with an upper case letter (so it should be "FIXME: Should...")
Created attachment 234655 [details] updating fixme
Comment on attachment 234655 [details] updating fixme Clearing flags on attachment: 234655 Committed r170960: <http://trac.webkit.org/changeset/170960>
All reviewed patches have been landed. Closing bug.