[webkit-reviews] review granted: [Bug 15119] Non-UTF-8 query parameters not handled properly when not generated by the form code : [Attachment 19443] Patch with layout test

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Marvin Decker
<marv.decker at gmail.com>'s request for review:
Bug 15119: Non-UTF-8 query parameters not handled properly when not generated
by the form code
http://bugs.webkit.org/show_bug.cgi?id=15119

Attachment 19443: Patch with layout test
http://bugs.webkit.org/attachment.cgi?id=19443&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looks good.

It's too bad that there's so much repeated code in the various codecs. Is there
some way to share more of the code? More helper functions in the base class,
perhaps?

+// Invalid character handler when wriuting escaped entities for
unrepresentable

Typo here: "wriuting".

+	 char number[32];
+	 sprintf(number, "%%26%%23%d%%3B", codePoint);

Security screening tools are going to complain about our use of sprintf. Lets
use snprintf as we do in source files like String.cpp using the
<wtf/StringExtras.h> header. I know the existing code was using sprintf, but
lets not.

+	 UChar outChar;
+	 if ((outChar = getGbkEscape(codePoint))) {

If you're going to initialize the variable in the if statement, then please
define it inside the if statement too. Then you won't need the double
parentheses.

+	     return;
+	 } else {

Please don't use else after return.

+	 // allowEntities will cause characters not encodable in the output
+	 // character encoding to be escaped as in XML/HTML entities.
+	 // For example, U+06DE will be "&#1758;" (in octal). When unset, some
+	 // replacement character will be unsed for unencodable characters.
+	 //
+	 // When escapeEntities is set, non-alphanumeric characters in the
+	 // entity will be escaped using URL rules. In the above example, of
+	 // "&#1758;" it will generate "%26%231758#3B". This flag has no effect

+	 // when allowEntities is false.
+	 CString encode(const UChar*, size_t length, bool allowEntities =
false, bool escapeEntities = false) const;

I think that two booleans is an ugly way to pass the entity policy. How about
an enum instead? Or maybe separate virtual  encode functions rather than a
single function with a flag. Do we really need all three policies? 

Sorry to be so nitpicky, but I find the spacing in comments like this hard to
read. If there are spaces between the paragraphs, but not between  the last
paragraph and the function definition it makes it hard to read.

I'm going to say review+, but I'd prefer to see a new patch with the issues I
mention above fixed.