[webkit-reviews] review denied: [Bug 128968] [XHR] overrideMimeType() should be able to change encoding in HEADERS RECEIVED state : [Attachment 234404] Rebasing patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied youenn fablet
<youennf at gmail.com>'s request for review:
Bug 128968: [XHR] overrideMimeType() should be able to change encoding in
HEADERS RECEIVED state
https://bugs.webkit.org/show_bug.cgi?id=128968

Attachment 234404: Rebasing patch
https://bugs.webkit.org/attachment.cgi?id=234404&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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?