Just gave it a quick look.

 - There should be overloads with SafeUri for (at least)
from/appendTrustedNameAndValue and for/appendBackgroundImage adding
"url(" and ")" around the SafeUri#asString() value.

 - There's a TODO in ClippedImageImplSomething about using these methods
once they're available; shouldn't it be addressed at the same time?


http://gwt-code-reviews.appspot.com/1454808/diff/1/user/src/com/google/gwt/safecss/SafeCss.gwt.xml
File user/src/com/google/gwt/safecss/SafeCss.gwt.xml (right):

http://gwt-code-reviews.appspot.com/1454808/diff/1/user/src/com/google/gwt/safecss/SafeCss.gwt.xml#newcode21
user/src/com/google/gwt/safecss/SafeCss.gwt.xml:21: <inherits
name="com.google.gwt.user.UserAgent"/>
Shouldn't you inherit c.g.g.dom.Dom?

http://gwt-code-reviews.appspot.com/1454808/diff/1/user/src/com/google/gwt/safecss/SafeCss.gwt.xml#newcode24
user/src/com/google/gwt/safecss/SafeCss.gwt.xml:24: <when-type-is
class="com.google.gwt.safecss.shared.SafeStylesUtils.ImplTrident" />
Did you mean SafeStylesUtils.Impl instead?
Also, if it doesn't apply to IE9, maybe it needs a better name than
"trident"; something like Ie6To8 maybe?

http://gwt-code-reviews.appspot.com/1454808/diff/1/user/src/com/google/gwt/safecss/shared/SafeStylesUtils.java
File user/src/com/google/gwt/safecss/shared/SafeStylesUtils.java
(right):

http://gwt-code-reviews.appspot.com/1454808/diff/1/user/src/com/google/gwt/safecss/shared/SafeStylesUtils.java#newcode53
user/src/com/google/gwt/safecss/shared/SafeStylesUtils.java:53: * The
server doesn't know necessarily know the user agent of the client, so
duplicated "know"

http://gwt-code-reviews.appspot.com/1454808/diff/1/user/src/com/google/gwt/safecss/shared/SafeStylesUtils.java#newcode494
user/src/com/google/gwt/safecss/shared/SafeStylesUtils.java:494: return
fromTrustedString(name + ":" + SafeHtmlUtils.htmlEscape(value) + ";");
If the SafeStyles is passed to a SafeHtmlTemplates, won't it be
double-escaped then?

There should also be an 'assert value.indexOf(';') < 0' (maybe a few
other checks, maybe a SafeStylesHostedModeUtils, or at least a TODO for
it)

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

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

Reply via email to