[webkit-reviews] review granted: [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
bugzilla-daemon at webkit.org
Fri Mar 7 12:28:56 PST 2008
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 19514: Revised patch x2
http://bugs.webkit.org/attachment.cgi?id=19514&action=edit
------- Additional Comments from Darin Adler <darin at apple.com>
Looks great.
59 switch (unencodable) {
60 case QuestionMarksForUnencodables:
61 replacement[0] = '?';
62 replacement[1] = 0;
63 return 1;
64 case EntitiesForUnencodables:
65 snprintf(replacement, 32, "&#%u;", codePoint);
66 break;
67 case URLEncodedEntitiesForUnencodables:
68 snprintf(replacement, 32, "%%26%%23%u%%3B", codePoint);
69 break;
70 default:
71 ASSERT_NOT_REACHED();
72 replacement[0] = 0;
73 }
74 return static_cast<int>(strlen(replacement));
I normally like to leave out the default: case so that GCC will warn if we ever
add a new value for the enum. That requires a little bit of reorganization,
though. Not very important.
I noticed that at some other call sites, you did not include a default:
ASSERT_NOT_REACHED. It would be better to be consistent.
44 // Substitues the replacement character "?".
Typo "Substitutes" is spelled wrong.
It's a little annoying that the value "32" is repeated at all the various
sites. Maybe it should have been named. Maybe a typedef for char[32] would have
been the best way to not repeat the value in multiple places; could use
sizeof(type) in the few places where the number is needed (passing in to
snprintf).
r=me as is, but there is still a little room for improvement.
More information about the webkit-reviews
mailing list