[webkit-reviews] review denied: [Bug 34653] Charset parameter set by setRequestHeader as "KOI8-R" is NOT changed to UTF-8. : [Attachment 50520] fix patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Chang Shu
<Chang.Shu at nokia.com>'s request for review:
Bug 34653: Charset parameter set by setRequestHeader as "KOI8-R" is NOT changed
to UTF-8.
https://bugs.webkit.org/show_bug.cgi?id=34653

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> KIO8-R can be found in the attached htm file around line 54:
>client.setRequestHeader("Content-Type", "text/plain;charset=KOI8-R");

Sorry, I was probably looking at some other test.

This bug has all Qt flags set on it, but the change is in cross-platform code.
Please correct the bug's component and keywords.

Whether we want to make this change depends on what Firefox and IE do, not just
on what the spec says. Please research their behavior, and post the results in
the bug.

+	 }
+	 else {

The should be on the same line.

+	     contentType = contentType.stripWhiteSpace();
+	     int start = contentType.find("charset", 0);

HTTP header modification should abstracted out into a separate function. We
don't have a great place to put it currently - HTTPParsers.{h,cpp} is the
closest, but it's only about parsing. Maybe you should just make it a static
funciton in XMLHttpRequest.cpp for now.

-		 setRequestHeaderInternal("Content-Type", "application/xml");
+		 setRequestHeaderInternal("Content-Type",
"text/plain;charset=UTF-8");

This is a change that is not asked for in this bug, and one that is not
explained in ChangeLog. Is there a reason you are making it in the same patch?
It will need to be discussed on its own.

+	 No new tests have been created. The orginal test case, 
+	
http://waplabdc.nokia-boston.com/browser/users/akancher/sf_XHR/Send/Send_strDat
a_Charset_Not_UTF8.htm,
+	 requires ALL-HTTP echoing back from an asp server. However, cgi does
not support this.

It is certainly possible to access a request Content-Type from a cgi, see e.g.
LayoutTests/http/tests/xmlhttprequest/print-content-type.cgi.