http://gwt-code-reviews.appspot.com/1454808/diff/4001/user/src/com/google/gwt/safecss/shared/SafeStylesBuilder.java File user/src/com/google/gwt/safecss/shared/SafeStylesBuilder.java (right):
http://gwt-code-reviews.appspot.com/1454808/diff/4001/user/src/com/google/gwt/safecss/shared/SafeStylesBuilder.java#newcode166 user/src/com/google/gwt/safecss/shared/SafeStylesBuilder.java:166: public SafeStylesBuilder floatprop(Float value) { On 2011/06/17 23:25:06, skybrian wrote:
Oh well. I can't think of anything better.
cssFloat? to mimic the name of the DOM property: http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSS2Properties-cssFloat http://gwt-code-reviews.appspot.com/1454808/diff/4001/user/src/com/google/gwt/safecss/shared/SafeStylesHostedModeUtils.java File user/src/com/google/gwt/safecss/shared/SafeStylesHostedModeUtils.java (right): http://gwt-code-reviews.appspot.com/1454808/diff/4001/user/src/com/google/gwt/safecss/shared/SafeStylesHostedModeUtils.java#newcode39 user/src/com/google/gwt/safecss/shared/SafeStylesHostedModeUtils.java:39: * If true, perform checks in server-side code. We were asked to make the javadoc clearer for the similar constant in SafeUriHostedModelUtils: http://www.google.com/codesearch#A1edwVHBClQ/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java&l=50 (as a side note, SafeHtmlHostedModeUtils hasn't been updated) http://gwt-code-reviews.appspot.com/1454808/diff/4001/user/src/com/google/gwt/safecss/shared/SafeStylesHostedModeUtils.java#newcode251 user/src/com/google/gwt/safecss/shared/SafeStylesHostedModeUtils.java:251: assert errorText == null : errorText; It would be better to "assert isValidStyleName(name) == null : isValidStyleName(name)". That way, isValidStyleName would *not* be called when assertions are disabled (this code might be used at server-side, not necessarily in DevMode only). When they're enabled, the method is only called twice in case of error, which is a good compromise IMO (if you enable assertions, you won't catch the AssertionError, it'd be a Very Bad Thing(tm)). Or we could just use a simple error message in the "assert" case, following the "best practice": > Note that the detail message is not a user-level error > message, so it is generally unnecessary to make these > messages understandable in isolation, or to > internationalize them. The detail message is meant to > be interpreted in the context of a full stack trace, in > conjunction with the source code containing the failed > assertion. http://download.oracle.com/javase/1.4.2/docs/guide/lang/assert.html http://gwt-code-reviews.appspot.com/1454808/diff/4001/user/src/com/google/gwt/safecss/shared/SafeStylesHostedModeUtils.java#newcode267 user/src/com/google/gwt/safecss/shared/SafeStylesHostedModeUtils.java:267: assert errorText == null : errorText; Similar here: try to optimize out the "assertions disabled" case. http://gwt-code-reviews.appspot.com/1454808/diff/4001/user/src/com/google/gwt/user/client/ui/impl/ClippedImageImpl.java File user/src/com/google/gwt/user/client/ui/impl/ClippedImageImpl.java (right): http://gwt-code-reviews.appspot.com/1454808/diff/4001/user/src/com/google/gwt/user/client/ui/impl/ClippedImageImpl.java#newcode70 user/src/com/google/gwt/user/client/ui/impl/ClippedImageImpl.java:70: "url(" + url.asString() + ") " + "no-repeat " + (-left + "px ") + (-top + "px")); Should be "url(\"" + url.asString() + "\") ". Should there be a SafeStylesUtils.urlprop(SafeUri) method to make this concatenation for us so we don't forget the quotes? http://gwt-code-reviews.appspot.com/1454808/diff/4001/user/test/com/google/gwt/safecss/shared/GwtSafeStylesHostedModeUtilsTest.java File user/test/com/google/gwt/safecss/shared/GwtSafeStylesHostedModeUtilsTest.java (right): http://gwt-code-reviews.appspot.com/1454808/diff/4001/user/test/com/google/gwt/safecss/shared/GwtSafeStylesHostedModeUtilsTest.java#newcode82 user/test/com/google/gwt/safecss/shared/GwtSafeStylesHostedModeUtilsTest.java:82: fail("Expected an exception for invalid style name."); On 2011/06/17 23:25:06, skybrian wrote:
This doesn't look right. If fail() is called, it will throw
AssertionError,
which gets silently ignored. If we're actually expecting a different AssertionError, it would be safer to set a boolean and check it
outside the try
block. (Similarly in the next test.)
+1 I recently fixed the same error in the SafeHtml and SafeUri tests. http://gwt-code-reviews.appspot.com/1454808/diff/4001/user/test/com/google/gwt/safecss/shared/GwtSafeStylesHostedModeUtilsTest.java#newcode83 user/test/com/google/gwt/safecss/shared/GwtSafeStylesHostedModeUtilsTest.java:83: } catch (IllegalArgumentException e) { On 2011/06/17 23:25:06, skybrian wrote:
Which exception do we actually expect? Should we check the message
string for a
reasonable error?
The maybeCheckXxx methods either use Preconditions.checkArgument(...) which throws an IllegalArgumentException, or an assert, which throws an AssertionError. We don't check for specific messages in SafeHtml/SafeUri tests, while the message used is a constant value; but I agree it'd be better to do it! Or, to make it easier to maintain (no need to synchronize the error message), we could first assertNotNull(isValidStyleName) and then try/catch on the maybeCheckValidStylename. That way, if isValidStyleName throws an IllegalArgumentException or AssertionError, we don't hide it here. http://gwt-code-reviews.appspot.com/1454808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
