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/18 00:39:07, tbroyer wrote:
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
Either one works. I preferred floatprop because it will autocomplete
when you start typing "float", whereas with cssFloat you have to look
through the methods in the class to find the correct method name.
I added a JavaDoc explaining whey its named floatprop instead of float.
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.
On 2011/06/18 00:39:07, tbroyer wrote:
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)
Done.
http://gwt-code-reviews.appspot.com/1454808/diff/4001/user/src/com/google/gwt/safecss/shared/SafeStylesHostedModeUtils.java#newcode214
user/src/com/google/gwt/safecss/shared/SafeStylesHostedModeUtils.java:214:
}
On 2011/06/17 23:25:06, skybrian wrote:
If we exit the loop with ignoreNext = true, I think that's an error
and perhaps
a security issue. (There should not be a backslash at the end of the
string,
because it would escape whatever comes next outside the string.)
Done.
I added a check and a test case for this case.
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;
On 2011/06/18 00:39:07, tbroyer wrote:
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
Done.
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;
On 2011/06/18 00:39:07, tbroyer wrote:
Similar here: try to optimize out the "assertions disabled" case.
Done.
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"));
On 2011/06/18 00:39:07, tbroyer wrote:
Should be "url(\"" + url.asString() + "\") ".
According to the spec, the quote is optional:
http://www.w3.org/TR/CSS21/syndata.html#uri
I added them here anyway.
Should there be a SafeStylesUtils.urlprop(SafeUri) method to make this
concatenation for us so we don't forget the quotes?
That might cause some confusion because you end up with a url wrapped in
a "url()", which is still just a fragment.
We could add backgroundPosition and backgroundRepeat methods in the
future, but I'll save that for another time.
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/18 00:39:07, tbroyer wrote:
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.
Done.
Good catch. I used the same pattern in other tests, which I also fixed.
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/18 00:39:07, tbroyer wrote:
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.
Done.
http://gwt-code-reviews.appspot.com/1454808/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
