[XHR] overrideMimeType() should be able to change encoding in HEADERS RECEIVED state

• Component: XML
• Status: RESOLVED FIXED

[youenn fablet]

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.

[youenn fablet]

Created attachment 224504 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[youenn fablet]

Created attachment 224516 [details]
Removing lowercasing of mimeTypeOverride

[youenn fablet]

Created attachment 234404 [details]
Rebasing patch

[Alexey Proskuryakov]

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?

[youenn fablet]

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.

[youenn fablet]

Created attachment 234482 [details]
Updated php script and FIXME

[Alexey Proskuryakov]

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...")

[youenn fablet]

Created attachment 234655 [details]
updating fixme

[WebKit Commit Bot]

Comment on attachment 234655 [details]
updating fixme

Clearing flags on attachment: 234655

Committed r170960: <http://trac.webkit.org/changeset/170960>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.