mostly looks fine so far
https://codereview.appspot.com/10943044/diff/14001/src/com/google/caja/lang/css/CssPropertyPatterns.java
File src/com/google/caja/lang/css/CssPropertyPatterns.java (right):
https://codereview.appspot.com/10943044/diff/14001/src/com/google/caja/lang/css/CssPropertyPatterns.java#newcode199
src/com/google/caja/lang/css/CssPropertyPatterns.java:199: } else if
(sig instanceof CssPropertySignature.ProgIdSignature) {
I'm ok with dropping support for progid, it was really only for lack of
alpha support in old IE, and that's not an issue any more.
https://codereview.appspot.com/10943044/diff/14001/src/com/google/caja/plugin/CssRewriter.java
File src/com/google/caja/plugin/CssRewriter.java (right):
https://codereview.appspot.com/10943044/diff/14001/src/com/google/caja/plugin/CssRewriter.java#newcode429
src/com/google/caja/plugin/CssRewriter.java:429: if
(!CSS21_COLOR_NAMES.contains(colorName.getCanonicalForm())) {
Is there a reason to whitelist color names instead of just allowing any
identifier?
https://codereview.appspot.com/10943044/diff/14001/tests/com/google/caja/lang/css/CssPropertyPatternsTest.java
File tests/com/google/caja/lang/css/CssPropertyPatternsTest.java
(right):
https://codereview.appspot.com/10943044/diff/14001/tests/com/google/caja/lang/css/CssPropertyPatternsTest.java#newcode80
tests/com/google/caja/lang/css/CssPropertyPatternsTest.java:80: public
final void testKeywordPatternsOverlyLoose() throws ParseException {
FailureIsAnOption is for things that should eventually be fixed. I think
we aren't interested in making 'zoicks' mismatch 'zoicks zoicks', so
either this should be an assertMatches, or it should be deleted. and
similarly for all the other OverlyLoose tests.
https://codereview.appspot.com/10943044/diff/14001/tests/com/google/caja/lang/css/CssPropertyPatternsTest.java#newcode258
tests/com/google/caja/lang/css/CssPropertyPatternsTest.java:258: private
void assertMatch(
it seems strange to have all this matching logic in the test class. why
test a pattern-matching engine that only exists in test code?
https://codereview.appspot.com/10943044/diff/14001/tests/com/google/caja/lang/css/CssPropertyPatternsTest.java#newcode273
tests/com/google/caja/lang/css/CssPropertyPatternsTest.java:273: for
(boolean adjacent = false; lexer.hasNext();) {
this seems like an odd use of for(), how about change it to while()?
https://codereview.appspot.com/10943044/
--
---
You received this message because you are subscribed to the Google Groups "Google Caja Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
