http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/src/com/google/gwt/resources/css/RtlVisitor.java File user/src/com/google/gwt/resources/css/RtlVisitor.java (right):
http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/src/com/google/gwt/resources/css/RtlVisitor.java#newcode46 user/src/com/google/gwt/resources/css/RtlVisitor.java:46: * Negates offset-x, assuming it's the first number found in the list. example? http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/src/com/google/gwt/resources/css/RtlVisitor.java#newcode51 user/src/com/google/gwt/resources/css/RtlVisitor.java:51: int offsetXIndex = indexOfFirstNumber(slice); It seems like a regular for loop would be more straightforward? for (int i = 0; i<slice.size(); i++) { NumberValue v = slice.get(i).isNumberValue(); if (v != null) { slice.set(i, ...); return; } } http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/src/com/google/gwt/resources/css/RtlVisitor.java#newcode160 user/src/com/google/gwt/resources/css/RtlVisitor.java:160: void propertyHandlerBorderImage(List<Value> values) { add javadoc with an example http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/src/com/google/gwt/resources/css/RtlVisitor.java#newcode173 user/src/com/google/gwt/resources/css/RtlVisitor.java:173: String ident = value.isIdentValue().getIdent(); Maybe we could add a convenience method to CssProperty.Value? /** * Returns true if this Value is one of the given * identifiers. */ boolean isIdentifier(String candidate, String... moreCandidates) Then this all simplifies to: int end = start; while (end < values.size() && (values.get(end).isNumberValue() != null || values.get(end).isIdentifier("/", "auto", "fill"))) { end++; } http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/src/com/google/gwt/resources/css/RtlVisitor.java#newcode304 user/src/com/google/gwt/resources/css/RtlVisitor.java:304: private void applyToSlices(List<Value> values, String delim, It seems this would be a bit simpler if we rid of the ValueListAction and had a split method instead: private List<List<Value>> split(List<Value>, String delim) { ... } Then the callers can just have a regular for loop, possibly keeping track of sliceNum if needed. http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/src/com/google/gwt/resources/css/RtlVisitor.java#newcode319 user/src/com/google/gwt/resources/css/RtlVisitor.java:319: private List<Value> excludeFillForBorderImageSlice(List<Value> values) { add javadoc with example? http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/src/com/google/gwt/resources/css/RtlVisitor.java#newcode341 user/src/com/google/gwt/resources/css/RtlVisitor.java:341: for (int i = 0; it.hasNext(); i++) { Nit: this for loop is kind of weird. How about: int i = 0; for (Value value : values) { ... i++; } http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/src/com/google/gwt/resources/css/RtlVisitor.java#newcode359 user/src/com/google/gwt/resources/css/RtlVisitor.java:359: StringBuilder methodName = new StringBuilder("propertyHandler"); maybe extract a function? String getHandlerName(String propertyName) It might be a good thing to have tests for. http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/src/com/google/gwt/resources/css/VendorPrefixes.java File user/src/com/google/gwt/resources/css/VendorPrefixes.java (right): http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/src/com/google/gwt/resources/css/VendorPrefixes.java#newcode65 user/src/com/google/gwt/resources/css/VendorPrefixes.java:65: if (identifier.length() == 0 || identifier.charAt(0) != '-') { Seems like the length has to be at least 4. The shortest string that should match is something like: "-a-b" http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/src/com/google/gwt/resources/css/VendorPrefixes.java#newcode69 user/src/com/google/gwt/resources/css/VendorPrefixes.java:69: if (nextHyphen < 0) { < 2, since "--" probably shouldn't match Maybe add a VendorPrefixesTest to make sure we're not too easily fooled. http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/test/com/google/gwt/resources/css/CssRtlTest.java File user/test/com/google/gwt/resources/css/CssRtlTest.java (right): http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/test/com/google/gwt/resources/css/CssRtlTest.java#newcode31 user/test/com/google/gwt/resources/css/CssRtlTest.java:31: public void testBorderImage() throws UnableToCompleteException { Nit: I realize you're just following what's there already, and it's not too bad, so maybe you don't want to take this on, but this sort of test is kind of hard to read, because the test data being compared is in two different files and these files are kind of big. It's better to have a bunch of small tests with all the test data right there in the test. So it seems like we should have a different version of test() that compares two strings? Perhaps something like this: public void testBorderImageSizes() { checkTransform( ".borderImageSizes {", " border-image-slice: 11px 12px 13px 14px fill;", " border-image-width: 22px 24px 26px 28px;", " border-image-outset: 1px 2px 3px 4px;", "}" ).into( ".borderImageSizes {", " border-image-slice: 11px 14px 13px 12px fill;", " border-image-width: 22px 28px 26px 24px;", " border-image-outset: 1px 4px 3px 2px;", "}" ).andReversed(); } (Along the lines of Testing on the Toilet episode 258.) http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/test/com/google/gwt/resources/css/borderRadiusRtl_test.css File user/test/com/google/gwt/resources/css/borderRadiusRtl_test.css (right): http://gwt-code-reviews.appspot.com/1727803/diff/4003/user/test/com/google/gwt/resources/css/borderRadiusRtl_test.css#newcode27 user/test/com/google/gwt/resources/css/borderRadiusRtl_test.css:27: .borderRadiusShorthand4 { Add some tests for vendor prefixes? http://gwt-code-reviews.appspot.com/1727803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
