http://gwt-code-reviews.appspot.com/1466809/diff/1/tools/api-checker/config/gwt23_24userApi.conf File tools/api-checker/config/gwt23_24userApi.conf (right):
http://gwt-code-reviews.appspot.com/1466809/diff/1/tools/api-checker/config/gwt23_24userApi.conf#newcode58 tools/api-checker/config/gwt23_24userApi.conf:58: :user/src/com/google/gwt/uibinder/client/UiRendererUtils.java\ This doesn't match the other lines. It shouldn't be necessary to exclude new classes from the excludedFiles_old list. http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java File user/src/com/google/gwt/uibinder/client/UiRendererUtils.java (right): http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode25 user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:25: public class UiRendererUtils { If this is an impl class, you should make it package protected or at least append "Impl" to the class name so nobody uses it. http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode47 user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:47: return null; Should this throw an IllegalArgumentException? If root is null, it indicates that an invalid parent was passed into the method. http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode55 user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:55: Element elementById = Document.get().getElementById(renderedId); You can move the assertion below this line, and only run it if elementById is null. That way, you can throw a Runtime exception in prod mode, but you only walk up the DOM in the error case. if (elementById == null) { if (!isAttachedToDom(root)) { throw new RuntimeException(""UiRendered element is not attached to DOM while getting \"" + fieldName + "\""); } else { throw new IllegalArgumentException("fieldName not found within rendered element"); } } http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode88 user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:88: : "Parent Element of previously rendered element contains more than one child"; This assertion should only be executed if ret is the first child of the parent. If the parent itself is the rendered Element, you don't need the check. http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode117 user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:117: public static SafeHtml stampUiRendererAttribute(SafeHtml safeHtml, String attributeName, Can you JavaDoc explaining what this is doing and what it returns. http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode120 user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:120: int endOfFirstTag = html.indexOf('>'); What if the SafeHtml does not contain any HTML, or if it is self closing (ends in />): <div id="placeholder" /> If UiBinder controls the inputs and can guarantee this doesn't happen, you should note that. An assertion in dev mode would be good too. http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode121 user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:121: html = html.substring(0, endOfFirstTag) + " " + attributeName + "='" + attributeValue + "'" We generally use double quotes instead of quotes. I'm not sure if it matters a lot. http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/client/UiRendererUtils.java#newcode123 user/src/com/google/gwt/uibinder/client/UiRendererUtils.java:123: return new SafeHtmlBuilder().appendHtmlConstant(html).toSafeHtml(); This is simpler: return SafeHtmlUtils.fromTrustedString(html); http://gwt-code-reviews.appspot.com/1466809/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/1466809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1732 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1732: List<JMethod> getters = findGetterNames(owner); What happens if there are other methods in the interface that aren't supported by UiRenderer? Do we rely on javac to trigger a compiler exception? http://gwt-code-reviews.appspot.com/1466809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1739 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1739: String elementParameter = getter.getParameters()[0].getName(); We should check that there is exactly 1 parameter, and its type is assignable from Element. http://gwt-code-reviews.appspot.com/1466809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
