Where is UiSafeHtmlInterpreter? I imagine it's a copy / paste clone of UiTextInterpreter, and will have the same bugs that one does. Instead, please make it a subclass of UiTextInterpreter, overriding a protected createComputedAttributeInstructor() method.
Please add the unit tests for both interpreters which would have uncovered these bugs. It maybe simplest to do that by having the test make its own ElementParser that uses one of them and driving it with ElementParserTester http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/elementparsers/ComputedAttributeInterpreter.java File user/src/com/google/gwt/uibinder/elementparsers/ComputedAttributeInterpreter.java (right): http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/elementparsers/ComputedAttributeInterpreter.java#newcode36 user/src/com/google/gwt/uibinder/elementparsers/ComputedAttributeInterpreter.java:36: String getAttributeToken(UiBinderWriter writer, XMLAttribute attribute) Just give the delegate the attribute. It can provide its own writer. http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/elementparsers/ComputedAttributeInterpreter.java#newcode89 user/src/com/google/gwt/uibinder/elementparsers/ComputedAttributeInterpreter.java:89: throws UnableToCompleteException { Remember what I said about too much copy paste? It is never okay to have this much identical code. • Make a new constructor, ComputedAttributeInterpreter(Delegate) • Make ComputedAttributeInterpreter(UiBinderWriter) a convenience wrapper for it that provides a default delegate implementation with the behavior in this method, and have this method always hand out to the delegate. • Get rid of the other interpretElement call. http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/elementparsers/ComputedAttributeInterpreter.java#newcode107 user/src/com/google/gwt/uibinder/elementparsers/ComputedAttributeInterpreter.java:107: String attToken = writer.tokenForStringExpression(att.consumeStringValue()); Isn't this exactly what the delegate in UiTextInterpreter is doing? So it doesn't need to do a delegate at all. http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/elementparsers/UiTextInterpreter.java File user/src/com/google/gwt/uibinder/elementparsers/UiTextInterpreter.java (right): http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/elementparsers/UiTextInterpreter.java#newcode33 user/src/com/google/gwt/uibinder/elementparsers/UiTextInterpreter.java:33: public final class UiTextDelegate implements ComputedAttributeInterpreter.Delegate { Why public? Why final? http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/elementparsers/UiTextInterpreter.java#newcode56 user/src/com/google/gwt/uibinder/elementparsers/UiTextInterpreter.java:56: if (!elem.getAttribute("from").hasComputedValue()) { NPE if there is no from attribute. http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/elementparsers/UiTextInterpreter.java#newcode64 user/src/com/google/gwt/uibinder/elementparsers/UiTextInterpreter.java:64: if (fieldRef == null) { Too late. Should also check that from is the only attribute. http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java File user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java (right): http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode809 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:809: public String tokenForSafeHtmlMethod(String expression) { Delete this method. Move the javadoc to Rafa's tokenForExpression, above, and use that. http://gwt-code-reviews.appspot.com/1422812/diff/1/user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java File user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java (right): http://gwt-code-reviews.appspot.com/1422812/diff/1/user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java#newcode632 user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java:632: assertEquals(widgetUi.mySafeHtml.getHTML(), "<b>This text should be bold!</b>"); Also need tests that ui:text escapes markup. Should test that in both a <gwt:Label> and a <gwt:HTML> http://gwt-code-reviews.appspot.com/1422812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
