http://gwt-code-reviews.appspot.com/1449814/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java File user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java (right):
http://gwt-code-reviews.appspot.com/1449814/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java#newcode117 user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java:117: if (Character.isHighSurrogate((char) codePoint) || Character.isLowSurrogate((char) codePoint)) { On 2011/06/08 16:41:02, xtof wrote:
Long lines, here and below.
Huh, right. I formatted the file and it was totally messed up as a result (for instance, but not only, it also reformatted the license header with 100-chars long lines, is it expected?) so I reverted it... and forgot to format the changes "locally". http://gwt-code-reviews.appspot.com/1449814/diff/1/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java File user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java (right): http://gwt-code-reviews.appspot.com/1449814/diff/1/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java#newcode96 user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java:96: assertEquals(CONSTANT_URL, UriUtils.fromTrustedString(CONSTANT_URL).asString()); On 2011/06/08 16:41:02, xtof wrote:
While you're here, could you change fromTrustedString to
fromSafeConstant where
the argument is a constant? In keeping with the desire that
fromTrustedString
should be used as little as possible...
As I said in the other review, these are unit-tests for fromTrustedString. Maybe we should duplicate them for fromSafeConstant then? (IIRC, initially, fromSafeConstant didn't validate the value; that would explain the lack of unit test) http://gwt-code-reviews.appspot.com/1449814/diff/1/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java#newcode109 user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java:109: public void testFromTrustedString_withInvalidUrl() { On 2011/06/08 16:41:02, xtof wrote:
Given that this test passed even with the original regex check
commented out,
would it make sense to to split the character set check of isValidUri
out and
test it separately?
Yes, there should probably be a SafeUriHostedModeUtilsTest. I'll try to do it tomorrow morning (UTC+2) http://gwt-code-reviews.appspot.com/1449814/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
