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) {
On 2013/07/11 22:33:36, felix8a wrote:
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.
Added a comment to that effect, but I'd rather not excise existing
progid support in the schema parser in this CL.
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())) {
On 2013/07/11 22:33:36, felix8a wrote:
Is there a reason to whitelist color names instead of just allowing
any
identifier?
I'd rather not white-list any identifier, though I don't see any reason
not to whitelist any in the CSS3 set now that more browsers are
supporting it.
Some properties can contain "/" as a token. "font" comes to mind. I
wouldn't want that combined with an unrecognized color to masquerade as
an unquoted URL.
Should I get rid of this CSS2/CSS3 distinction here or in a follow-on
URL?
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 {
On 2013/07/11 22:33:36, felix8a wrote:
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.
Deleted all.
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(
On 2013/07/11 22:33:36, felix8a wrote:
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?
Yeah. Being able to test patterns that don't appear in the CSS schema
helps explore corner-cases in CssPropertyPatterns, but it is duplicating
some portion of sanitizecss's logic.
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();) {
On 2013/07/11 22:33:36, felix8a wrote:
this seems like an odd use of for(), how about change it to while()?
Done.
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.
