[webkit-reviews] review denied: [Bug 34653] Charset parameter set by setRequestHeader as "KOI8-R" is NOT changed to UTF-8 : [Attachment 51079] fix patch 4
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Apr 1 23:53:23 PDT 2010
Darin Fisher (:fishd, Google) <fishd at chromium.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 51079: fix patch 4
https://bugs.webkit.org/attachment.cgi?id=51079&action=review
------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
Sorry for the super slow turnaround on this code review.
> Index: WebCore/platform/network/HTTPParsers.cpp
...
> +void findCharsetInMediaType(const String& mediaType, int& charsetStart, int&
charsetLen)
> +{
> + charsetStart = 0;
> + charsetLen = 0;
> +
> int pos = 0;
> int length = (int)mediaType.length();
>
> while (pos < length) {
> pos = mediaType.find("charset", pos, false);
> + if (pos <= 0) {
> + charsetLen = 0;
> + return;
> + }
>
> // is what we found a beginning of a word?
> if (mediaType[pos-1] > ' ' && mediaType[pos-1] != ';') {
> @@ -214,10 +226,12 @@ String extractCharsetFromMediaType(const
> int endpos = pos;
> while (pos != length && mediaType[endpos] > ' ' && mediaType[endpos]
!= '"' && mediaType[endpos] != '\'' && mediaType[endpos] != ';')
> ++endpos;
> +
> + charsetStart = pos;
> + charsetLen = endpos - pos;
> + return;
> }
> +
> + charsetLen = 0;
> }
^^^ it looks likes it is unnecessary to assign 0 here since contentLen is
initialized to 0 at the top of the function.
> Index: WebCore/xml/XMLHttpRequest.cpp
> ===================================================================
> --- WebCore/xml/XMLHttpRequest.cpp (revision 56125)
> +++ WebCore/xml/XMLHttpRequest.cpp (working copy)
> @@ -128,6 +128,31 @@ static bool isSetCookieHeader(const Atom
> return equalIgnoringCase(name, "set-cookie") || equalIgnoringCase(name,
"set-cookie2");
> }
>
> +static void setCharsetInMediaType(String& mediaType, const String&
charsetValue)
> +{
> + int start, len;
> + findCharsetInMediaType(mediaType, start, len);
> +
> + if (!len) {
> + // When no charset found, append new charset.
> + mediaType.stripWhiteSpace();
> + if (mediaType[mediaType.length() - 1] != ';')
> + mediaType.append(";");
> + mediaType.append(" charset=");
> + mediaType.append(charsetValue);
> + } else {
> + // Found existing charset, replace with new charset.
> + String newMediaType = mediaType.substring(0, start);
> + newMediaType.append(charsetValue);
> +
> + if (start + len < (int)mediaType.length()) {
^^^ nit: please use static_cast<int>(...) instead of the C-style cast.
> + newMediaType.append("; ");
> + newMediaType.append(mediaType.substring(start + len));
maybe it would be better to use the String::replace method?
another approach here might be to strip out all occurrences of a charset
attribute and then append the desired charset attribute.
> Index: LayoutTests/http/tests/xmlhttprequest/request-encoding2.html
...
> + function test1()
> + {
> + req = new XMLHttpRequest;
> + req.open("POST", "print-content-type.cgi", false);
> +
> + req.send("");
> +
> + document.getElementById("result1").firstChild.data = "Test1
(setRequestHeader was not called):";
> + if (req.responseText == "application/xml\n")
Can you explain why this case does not have an implicit charset?
More information about the webkit-reviews
mailing list