LGTM with minor nits.

http://gwt-code-reviews.appspot.com/128801/diff/1/3
File user/src/com/google/gwt/user/client/Cookies.java (right):

http://gwt-code-reviews.appspot.com/128801/diff/1/3#newcode112
Line 112: if (uriEncoding) {
Prettier and less duplicated code to do:

if (uriEncoding) {
   name = uriEncode(name);
}
removeCookieNative(name);

http://gwt-code-reviews.appspot.com/128801/diff/1/3#newcode115
Line 115: else {
} else {

on same line

http://gwt-code-reviews.appspot.com/128801/diff/1/3#newcode128
Line 128: if (uriEncoding) {
Ditto

http://gwt-code-reviews.appspot.com/128801/diff/1/3#newcode184
Line 184: throw new IllegalArgumentException("Illegal cookie format.");
Include name and value in exception message, maybe distinguish between
bad name and bad value to make debugging easier.

http://gwt-code-reviews.appspot.com/128801/diff/1/2
File user/test/com/google/gwt/user/client/CookieTest.java (right):

http://gwt-code-reviews.appspot.com/128801/diff/1/2#newcode166
Line 166: // Make sure cookie values are URI encoded
Probably best to add 'Cookies.setUriEncode(true);' here (redundantly) to
avoid dependency between sections of this method.

http://gwt-code-reviews.appspot.com/128801

-- 
http://groups.google.com/group/Google-Web-Toolkit-Contributors

Reply via email to