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

[bugzilla-daemon at webkit.org]

Marvin Decker <marv.decker at gmail.com> has asked  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 19514: Revised patch x2
http://bugs.webkit.org/attachment.cgi?id=19514&action=edit

------- Additional Comments from Marvin Decker <marv.decker at gmail.com>
> 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.

Ugh, I've got way too many coding styles in my head nowadays.


> 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 assume you mean specified at every call site? I was initially very resistant
to this because I didn't want to change 1000 files, but there are only a couple
places it's used. I agree, don't think the default value is very clear here.


> 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.

I used a buffer and returned the length (since I don't use the return value for
anything) which I think is more clear than an out param.

I changed XMLHTTPRequest code to specify entities rather than substitution. It
looks like it's currently hard-coded for UTF-8, so this won't be an issue, but
I'm pretty sure when you correctly support the document encoding, entities will
be correct here.