http://gwt-code-reviews.appspot.com/33827/diff/1/12 File user/src/com/google/gwt/resources/client/CssResource.java (right):
http://gwt-code-reviews.appspot.com/33827/diff/1/12#newcode229 Line 229: * recommended as the default behavior for CssResources. <hindsight>Should have made it the default behavior, and provided @Lenient</hindsight> http://gwt-code-reviews.appspot.com/33827/diff/1/10 File user/src/com/google/gwt/resources/css/GenerateCssAst.java (right): http://gwt-code-reviews.appspot.com/33827/diff/1/10#newcode420 Line 420: Collections.addAll(externals.getClasses(), parts); It's creepy that CssExternalSelectors's classes list is externally modifiable. Any reason not to give it a Collection<String> constructor arg? http://gwt-code-reviews.appspot.com/33827/diff/1/9 File user/src/com/google/gwt/resources/css/ast/CssExternalSelectors.java (right): http://gwt-code-reviews.appspot.com/33827/diff/1/9#newcode24 Line 24: * subject to obfuscation requirements. @see CssResource.External http://gwt-code-reviews.appspot.com/33827/diff/1/7 File user/src/com/google/gwt/resources/css/ast/CssNodeCloner.java (right): http://gwt-code-reviews.appspot.com/33827/diff/1/7#newcode135 Line 135: public boolean visit(CssExternalSelectors x, Context ctx) { I loves me a good Visitor. http://gwt-code-reviews.appspot.com/33827/diff/1/8 File user/src/com/google/gwt/resources/css/ast/CssVisitor.java (right): http://gwt-code-reviews.appspot.com/33827/diff/1/8#newcode21 Line 21: * The base class for visiting a CSS tree. Please document what happens when the visit methods return true or false. (Up here is fine, copy / paste on each and every seems silly.) Also document how to kick off a traversal. Are these only used for traversal, or will you sometimes just visit a single node for double dispatch purposes? If both, doc for both would be nice. Not asking for a novel, but a recipe line or two. If that doc already exists somewhere else, this would be a good place to have a link to it. http://gwt-code-reviews.appspot.com/33827/diff/1/11 File user/src/com/google/gwt/resources/rg/CssResourceGenerator.java (right): http://gwt-code-reviews.appspot.com/33827/diff/1/11#newcode105 Line 105: static class ClassRenamer extends CssVisitor { Should really pull this out into ClassRenamer.java. (Not in this patch.) http://gwt-code-reviews.appspot.com/33827/diff/1/11#newcode107 Line 107: private final Map<String, Map<JMethod, String>> classReplacementsWithPrefix; It would be nice to have doc explaining this map. Tell us that the outer key is the prefix, the inner key the replacement name. And tell us what the prefix is, I can't figure out where it comes from. http://gwt-code-reviews.appspot.com/33827/diff/1/11#newcode120 Line 120: this.classReplacementsWithPrefix = classReplacementsWithPrefix; defensive copy would be wise. http://gwt-code-reviews.appspot.com/33827/diff/1/11#newcode1511 Line 1511: + method.getName() + "() is missing the @Strict annotation."); You're going to scold me if I set a system wide force-strict flag, but don't provide a redundant annotation on every method? What did I ever do to you? Also, comment above implies the opposite warning "does already have the @Strict". http://gwt-code-reviews.appspot.com/33827/diff/1/4 File user/test/com/google/gwt/resources/client/test.css (right): http://gwt-code-reviews.appspot.com/33827/diff/1/4#newcode212 Line 212: /* The @external parser attempts to be flexible */ Mad man. So I could do @external able baker; .able, .baker, .charlie { border: green; } and only able and baker will be externalized? Is that documented? Is that particular case worth testing? http://gwt-code-reviews.appspot.com/33827 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
