Bug 128968 - [XHR] overrideMimeType() should be able to change encoding in HEADERS RECEIVED state
Summary: [XHR] overrideMimeType() should be able to change encoding in HEADERS RECEIVE...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-18 06:11 PST by youenn fablet
Modified: 2014-07-10 03:10 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.36 KB, patch)
2014-02-18 06:44 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (452.16 KB, application/zip)
2014-02-18 07:33 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (486.99 KB, application/zip)
2014-02-18 08:03 PST, Build Bot
no flags Details
Removing lowercasing of mimeTypeOverride (7.39 KB, patch)
2014-02-18 08:45 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing patch (7.53 KB, patch)
2014-07-04 05:08 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Updated php script and FIXME (6.99 KB, patch)
2014-07-06 23:41 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
updating fixme (7.03 KB, patch)
2014-07-09 14:17 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2014-02-18 06:11:03 PST
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.
Comment 1 youenn fablet 2014-02-18 06:44:30 PST
Created attachment 224504 [details]
Patch
Comment 2 Build Bot 2014-02-18 07:33:30 PST
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
Comment 3 Build Bot 2014-02-18 07:33:31 PST
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 4 Build Bot 2014-02-18 08:03:53 PST
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
Comment 5 Build Bot 2014-02-18 08:03:55 PST
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
Comment 6 youenn fablet 2014-02-18 08:45:30 PST
Created attachment 224516 [details]
Removing lowercasing of mimeTypeOverride
Comment 7 youenn fablet 2014-07-04 05:08:56 PDT
Created attachment 234404 [details]
Rebasing patch
Comment 8 Alexey Proskuryakov 2014-07-04 07:10:45 PDT
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?
Comment 9 youenn fablet 2014-07-06 13:53:22 PDT
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.
Comment 10 youenn fablet 2014-07-06 23:41:45 PDT
Created attachment 234482 [details]
Updated php script and FIXME
Comment 11 Alexey Proskuryakov 2014-07-09 00:48:55 PDT
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...")
Comment 12 youenn fablet 2014-07-09 14:17:38 PDT
Created attachment 234655 [details]
updating fixme
Comment 13 WebKit Commit Bot 2014-07-10 03:10:02 PDT
Comment on attachment 234655 [details]
updating fixme

Clearing flags on attachment: 234655

Committed r170960: <http://trac.webkit.org/changeset/170960>
Comment 14 WebKit Commit Bot 2014-07-10 03:10:06 PDT
All reviewed patches have been landed.  Closing bug.