[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