[webkit-reviews] review denied: [Bug 15119] Non-UTF-8 query parameters not handled properly when not generated by the form code : [Attachment 19468] Revised patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 19468: Revised patch
http://bugs.webkit.org/attachment.cgi?id=19468&action=edit

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

As documented in http://webkit.org/coding/coding-style.html, we don't use all
capitals for enums. It should be Substitute, Entity and EscapedEntity rather
than SUBSTIUTE, ENTITY and ESCAPED_ENTITY. I also would suggest making
UnencodableHandling a top level member of WebCore and naming the enum values
QuestionMarksForUnencodables, EntitiesForUnencodables, and
URLEncodedEntitiesForUnencodables.

(The all capital style for macros was invented to keep the macros from
colliding with other uses of the same identifier and it's not needed for things
that aren't macros.)

I don't think the encode function needs a default value for
UnencodableHandling. I'd prefer to see it capitalized at every call site.

I'm slightly uncomfortable with the non-thread-safe implementation of
unencodeableCharReplacement. How about having the function take a reference to
an array of the right size instead? Then the caller can supply the buffer. I'd
also suggest using int& rather than int* for the length out pointer.

 284					  UChar32 codePoint,
UConverterCallbackReason reason, UErrorCode* err) {

We put braces on separate lines not on the same line as the declaration.

 295 // Substutites special GBK characters, escaping all other unassigned
entities.

Typo here in the word "Substutites".

 309 // Combines both gbkUrlEscapedEntityCallack with GBK character
substitution.

Strange wording here: Both/with.

 79		int replLen;
 80		const char* repl = TextCodec::unencodableCharReplacement(c,
unencodable, &replLen);

I prefer actual words rather than fragments like "repl" for variable names.

I'm going to say review- this time because I'd like at least some of these
style issues to be fixed.