I moved the checks in SafeHtmlTemplatesImplMethodCreator to emitParameterExpression() as you suggested. It might be easier to review this file against trunk than against version 1.
http://gwt-code-reviews.appspot.com/1384801/diff/1/user/src/com/google/gwt/safecss/shared/SafeCss.java File user/src/com/google/gwt/safecss/shared/SafeCss.java (right): http://gwt-code-reviews.appspot.com/1384801/diff/1/user/src/com/google/gwt/safecss/shared/SafeCss.java#newcode72 user/src/com/google/gwt/safecss/shared/SafeCss.java:72: * facility for restricting deserialization on the Server only (thought this On 2011/03/11 17:48:30, rjrjr wrote:
though
Done. Fixed in SafeHtml as well. http://gwt-code-reviews.appspot.com/1384801/diff/1/user/src/com/google/gwt/safecss/shared/SafeCssString.java File user/src/com/google/gwt/safecss/shared/SafeCssString.java (right): http://gwt-code-reviews.appspot.com/1384801/diff/1/user/src/com/google/gwt/safecss/shared/SafeCssString.java#newcode24 user/src/com/google/gwt/safecss/shared/SafeCssString.java:24: * On 2011/03/11 17:48:30, rjrjr wrote:
<p>
Done. Fixed in SafeHtmlString as well. http://gwt-code-reviews.appspot.com/1384801/diff/1/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/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode186 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:186: * context. In a non-text context, the string is not guarenteed to be On 2011/03/11 17:48:30, rjrjr wrote:
guaranteed
Done. http://gwt-code-reviews.appspot.com/1384801/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode283 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:283: switch (htmlContext.getType()) { On 2011/03/11 17:48:30, rjrjr wrote:
Are you sure the SafeCss fragments make sense in both contexts? And
not sure I
saw test coverage of both paths.
I was being too hand wavy here. SafeCssProperties is safe in a CSS context if you are in a style tag, but not always. Now that we call it SafeCssPropeties and specify that its only safe for the style attribute, I put the original warning back. Eventually, we can add SafeCssRules, which contain the style rules found in a style tag. They can be built using SafeCssProperties. http://gwt-code-reviews.appspot.com/1384801/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode287 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:287: // safety via sub-formats that specify the in-css context. On 2011/03/11 17:48:30, rjrjr wrote:
drop the todo? Or update it?
Updated the comment. http://gwt-code-reviews.appspot.com/1384801/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode298 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:298: // TODO(xtof): Improve support for CSS. On 2011/03/11 17:48:30, rjrjr wrote:
ditto
Dropped the TODO here and added one to SafeCssPropertiesUtils to add context specific utility methods. http://gwt-code-reviews.appspot.com/1384801/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode354 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:354: private void emitTextContextParameterExpression(HtmlContext htmlContext, On 2011/03/11 17:48:30, rjrjr wrote:
It's odd that you're passing the context in here and checking its type
again,
rather than keeping all of the type-specific checks up on the switch
statement
that calls this method.
Done. http://gwt-code-reviews.appspot.com/1384801/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode368 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:368: throw new IllegalArgumentException(NON_TEXT_CONTEXT_ERROR); On 2011/03/11 17:48:30, rjrjr wrote:
Breaking change, yes? Seems like your users will see the warning, and
then get
this exception. Also, throwing an IllegalArgumentException for a user
error is
wrong.
It is a breaking change. I'll add a note to the issue tracker. Changed IllegalArgumentException to UnableToCompleteException. http://gwt-code-reviews.appspot.com/1384801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
