http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safecss/shared/SafeStyles.java File user/src/com/google/gwt/safecss/shared/SafeStyles.java (right):
http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safecss/shared/SafeStyles.java#newcode71 user/src/com/google/gwt/safecss/shared/SafeStyles.java:71: * attribute: Maybe instead say, "comply with this type's contract" (here and elsewhere) -- these examples are not inherently unsafe, they just don't comply with the type contract (specifically, the composability requirement). With that in mind, perhaps move the examples after the paragraph about composability? http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safecss/shared/SafeStyles.java#newcode88 user/src/com/google/gwt/safecss/shared/SafeStyles.java:88: * {@code http://trackingurl.com)"} is appended to {@code background:url("}, the Hmm, so generally the SafeHtml contracts don't guarantee that the resulting HTML is side-effect free; in particular when we sanitize untrusted URIs we'll leave them alone as long as they have a whitelisted scheme. I.e. we'd allow <img src='{0}'> where the value of {0} is http://trackingurl.com/ Complete side effect freedom in this sense would be much harder to enforce (we'd have to somehow have a way to configure filters that know which domains are "trusted" and which ones are third-party). Perhaps a better example would be to use "javascript:evil()" as a url? http://gwt-code-reviews.appspot.com/1384801/diff/8001/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/1384801/diff/8001/user/src/com/google/gwt/safecss/shared/SafeStylesBuilder.java#newcode42 user/src/com/google/gwt/safecss/shared/SafeStylesBuilder.java:42: * any escaping to it. Perhaps, "any escaping or sanitization"? http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java File user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java (right): http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode131 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:131: * <li>If the parameter is not of type {@link SafeStyles}, the result is then I think this comment should remain unchanged; we always HTML escape the attribute value even if it came from a SafeStyles. http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode176 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:176: // TODO(xtof): Throw an exception if using SafeHtml within an attribute. I think you can remove this TODO, isn't this handled now in your new checks at the start of emitParameterExpression? http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode280 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:280: * definitely isn't what the user intended to do. Maybe "user" -> "developer"? http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode315 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:315: logger.log(TreeLogger.WARN, I find the code is a bit hard to understand with the checks related to CSS_ATTRIBUTE contexts partially done here, and partially done up above in line 270ff. Might it be more readable if you move the checks from above to here (possibly splitting the two cases)? Not sure on this; might make the code more verbose... Otherwise, a comment noting the precondition established by the check in 270ff might help. http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/user/cellview/client/CellBrowser.java File user/src/com/google/gwt/user/cellview/client/CellBrowser.java (right): http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/user/cellview/client/CellBrowser.java#newcode728 user/src/com/google/gwt/user/cellview/client/CellBrowser.java:728: private static final SafeHtml LEAF_IMAGE = SafeHtmlUtils Maybe break after = ? http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java File user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java (right): http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java#newcode60 user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java:60: SafeHtml templateWithCssAttribute(int height /* generates a compiled time warning */, "compile time" http://gwt-code-reviews.appspot.com/1384801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
