[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]

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?