http://gwt-code-reviews.appspot.com/1384801/diff/3003/user/src/com/google/gwt/safecss/shared/SafeCssProperties.java File user/src/com/google/gwt/safecss/shared/SafeCssProperties.java (right):
http://gwt-code-reviews.appspot.com/1384801/diff/3003/user/src/com/google/gwt/safecss/shared/SafeCssProperties.java#newcode37 user/src/com/google/gwt/safecss/shared/SafeCssProperties.java:37: * All implementing classes must maintain the class invariant (by design and On 2011/03/11 23:02:45, skybrian wrote:
This description of the class invariant is rather abstract. I think
users would
find it helpful if we gave them some examples of valid and invalid
strings.
As I understand it, the empty string, "width: 1em;" and "width: 1em;
height:
1em;" are safe, but just "1em" is not. It's also an error to leave off
the
trailing semicolon.
Done. http://gwt-code-reviews.appspot.com/1384801/diff/3003/user/src/com/google/gwt/safecss/shared/SafeCssPropertiesString.java File user/src/com/google/gwt/safecss/shared/SafeCssPropertiesString.java (right): http://gwt-code-reviews.appspot.com/1384801/diff/3003/user/src/com/google/gwt/safecss/shared/SafeCssPropertiesString.java#newcode40 user/src/com/google/gwt/safecss/shared/SafeCssPropertiesString.java:40: SafeCssPropertiesString(String css) { On 2011/03/11 23:02:45, skybrian wrote:
perhaps have an assert that the string is either empty or ends with a
semicolon,
just to catch blatantly wrong usage?
Done. http://gwt-code-reviews.appspot.com/1384801/diff/3003/user/src/com/google/gwt/safecss/shared/SafeCssPropertiesUtils.java File user/src/com/google/gwt/safecss/shared/SafeCssPropertiesUtils.java (right): http://gwt-code-reviews.appspot.com/1384801/diff/3003/user/src/com/google/gwt/safecss/shared/SafeCssPropertiesUtils.java#newcode31 user/src/com/google/gwt/safecss/shared/SafeCssPropertiesUtils.java:31: * code should be carefully reviewed to ensure the argument meets the On 2011/03/11 23:02:45, skybrian wrote:
As a convenience to the user, maybe put some concrete examples here
too. (I
suspect this JavaDoc would be the first documentation seen by someone
reading
user code in an IDE, and concrete examples will make it blindingly
obvious if
someone's doing it wrong.)
Done. http://gwt-code-reviews.appspot.com/1384801/diff/3003/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/3003/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode146 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:146: * <li>If the parameter is not of type {@link String}, it is first converted On 2011/03/11 23:02:45, skybrian wrote:
need to update javadoc
Done. http://gwt-code-reviews.appspot.com/1384801/diff/3003/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode185 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:185: doEscape = false; I think escaping at this point would defeat the purpose of using SafeCss. Css property strings sometimes contain quotes, so we don't want to escape them by default. For example: background: url('http://example.com/marble.png'); I added a note in the JavaDoc that SafeCssProperties should only use single quotes, and that the style attribute should use double quotes to wrap the value. Eventually, we could update SafeHtmlTemplates to verify that the style attribute wraps the value in double quotes. http://gwt-code-reviews.appspot.com/1384801/diff/3003/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/3003/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java#newcode80 user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java:80: public void testTemplateWithCssAttribute() { I added a some asserts to SafeCss to check for an end semi-colon, double quotes, and brackets. The & character is valid because it can be used in a URL: background:url(http://url?image=123112&size=1024) I added tests to SafeCssPropertiesString to test the assertions. http://gwt-code-reviews.appspot.com/1384801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
