LGTM
http://gwt-code-reviews.appspot.com/77805/diff/1/4 File user/src/com/google/gwt/uibinder/rebind/BundleWriter.java (right): http://gwt-code-reviews.appspot.com/77805/diff/1/4#newcode41 Line 41: private final JClassType strictAnnotationType; Not strictly necessary after I integrate the spate of CssResource patches. http://gwt-code-reviews.appspot.com/77805/diff/1/4#newcode82 Line 82: writer.write("import %s;", strictAnnotationType.getQualifiedSourceName()); If you were to add additional imports, it would probably be cleaner to have an array or Arrays.asList() over which you iterate to set up the imports. http://gwt-code-reviews.appspot.com/77805/diff/1/4#newcode111 Line 111: writeImageOptionsAnnotation(image.getFlipRtl(), image.getRepeatStyle()); Is there a way to make this more resilient to adding new image options? I was thinking about adding compile-time scaling to ImageOptions, for instance. http://gwt-code-reviews.appspot.com/77805/diff/1/5 File user/src/com/google/gwt/uibinder/rebind/GetEscapedInnerTextVisitor.java (right): http://gwt-code-reviews.appspot.com/77805/diff/1/5#newcode52 Line 52: // TODO(jgw): write this back just as it came in. Is this TODO important? http://gwt-code-reviews.appspot.com/77805/diff/1/5#newcode75 Line 75: parent != null && "pre".equals(parent.getTagName()); Would this ever operate on a script element? http://gwt-code-reviews.appspot.com/77805/diff/1/8 File user/src/com/google/gwt/uibinder/rebind/UiBinderParser.java (right): http://gwt-code-reviews.appspot.com/77805/diff/1/8#newcode121 Line 121: if (elem.hasAttribute(FLIP_RTL_ATTRIBUTE)) { Can this extraction be made dynamic? http://gwt-code-reviews.appspot.com/77805/diff/1/9 File user/src/com/google/gwt/uibinder/rebind/XMLElement.java (right): http://gwt-code-reviews.appspot.com/77805/diff/1/9#newcode347 Line 347: writer.die("%s must contain only text", this); Is it possible to have a degenerate case with multiple sibling text nodes? http://gwt-code-reviews.appspot.com/77805/diff/1/10 File user/src/com/google/gwt/uibinder/rebind/model/ImplicitClientBundle.java (right): http://gwt-code-reviews.appspot.com/77805/diff/1/10#newcode73 Line 73: * Called to declare a new CssResource accessor on this bundle. CssResource? http://gwt-code-reviews.appspot.com/77805/diff/1/14 File user/src/com/google/gwt/uibinder/sample/client/WidgetBasedUi.ui.xml (right): http://gwt-code-reviews.appspot.com/77805/diff/1/14#newcode187 Line 187: Well of course you do. Who wouldn't? Creepy. http://gwt-code-reviews.appspot.com/77805/diff/1/16 File user/test/com/google/gwt/uibinder/rebind/XMLElementTest.java (right): http://gwt-code-reviews.appspot.com/77805/diff/1/16#newcode73 Line 73: // TODO(rjrjr) Can't test this until die() is factored out of UiBinderWriter Didn't I see you use .die() elsewhere in this patch? http://gwt-code-reviews.appspot.com/77805 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
