I haven't looked at every class, but this should be enough for now.
http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/client/DomBuilderFactory.java File user/src/com/google/gwt/dom/builder/client/DomBuilderFactory.java (right): http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/client/DomBuilderFactory.java#newcode57 user/src/com/google/gwt/dom/builder/client/DomBuilderFactory.java:57: public DivElementBuilder createDivElementBuilder() { "createDivBuilder"? http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/client/DomBuilderImpl.java File user/src/com/google/gwt/dom/builder/client/DomBuilderImpl.java (right): http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/client/DomBuilderImpl.java#newcode145 user/src/com/google/gwt/dom/builder/client/DomBuilderImpl.java:145: Element getCurrentElement() { It's unlikely to matter, but perhaps more efficient to keep the current element in a field, so we don't access the list here. Then it's just a field access and can be inlined. List<Element> parentStack = new ArrayList<Element>(); Element currentElement; http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/DivElementBuilder.java File user/src/com/google/gwt/dom/builder/shared/DivElementBuilder.java (right): http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/DivElementBuilder.java#newcode21 user/src/com/google/gwt/dom/builder/shared/DivElementBuilder.java:21: public interface DivElementBuilder extends ElementBuilderBase<DivElementBuilder> { "DivBuilder"? http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java File user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java (right): http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java#newcode40 user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java:40: Element asElement(); Perhaps "finishElement()" or just "finish()" since "as" implies it's just a cast. http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java#newcode47 user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java:47: * @return this builder I'd remove the @param since they don't actually tell us anything. http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java#newcode83 user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java:83: * @param draggable a String constants "constant". http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java#newcode106 user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java:106: <B extends ElementBuilderBase<?>> B end(Class<B> clazz); I'm wondering if we need the variations on end() that take a class. In particular, this looks like a mismatch: out.startDiv().startH1().text("Heading").end(DivBuilder.class) It also doubles the number of end() method variants. Though it's ugly, perhaps this would be better: out.startDiv() .startH1().text("Heading").end().backTo(DivBuilder.class) .text("Some text") .startH2().text("Hello").endH2() .end() Although, my personal preference is to not chain so many methods. Perhaps: out.startDiv(); out.asDiv().startH1().text("hello").endH1(); out.asDiv().text("hello"); out.asDiv().startH2().text("hello").endH2(); out.endDiv(); If you squint at it right, the "asDiv()" call looks sort of like an indent. Or perhaps: DivBuilder __div = out.asDiv(); out.startDiv(); __div.startH1().text("Heading").end(); __div.text("Some text"); __div.startH2().text("Hello").end(); out.end(); Normally I'd be wary of mixing up local variables, but since they all point to the same instance, it seems fairly harmless. http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java#newcode269 user/src/com/google/gwt/dom/builder/shared/ElementBuilderBase.java:269: * Once you append text to the element, you can no longer set attributes. Perhaps say that the text will be escaped. http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderImpl.java File user/src/com/google/gwt/dom/builder/shared/ElementBuilderImpl.java (right): http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/ElementBuilderImpl.java#newcode54 user/src/com/google/gwt/dom/builder/shared/ElementBuilderImpl.java:54: /** oops http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/HtmlBuilderImpl.java File user/src/com/google/gwt/dom/builder/shared/HtmlBuilderImpl.java (right): http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/HtmlBuilderImpl.java#newcode160 user/src/com/google/gwt/dom/builder/shared/HtmlBuilderImpl.java:160: sb.append("<").append(escape(tagName)); Perhaps have trustedStart() to use internally, which skips escaping the tag. Actually, escaping it seems like the wrong thing; really we want to reject it if it needs escaping. http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/StyleBuilder.java File user/src/com/google/gwt/dom/builder/shared/StyleBuilder.java (right): http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/StyleBuilder.java#newcode228 user/src/com/google/gwt/dom/builder/shared/StyleBuilder.java:228: * Set the trusted background color, i.e., without escaping the value. No Possible rewording to be a bit less boilerplatey: Appends the equivalent of "background-color:{value};" to the style tag's contents. Does not check or escape the color string. The calling code should be carefully reviewed to ensure that the provided color string won't cause a security issue if appended to innerHtml. For details, see {link}. Also, the last paragraph of the boilerplate was garbled a bit using copy-and-paste. I'd suggest putting it in one place and linking to it. http://gwt-code-reviews.appspot.com/1455802/diff/5002/user/src/com/google/gwt/dom/builder/shared/StyleBuilder.java#newcode243 user/src/com/google/gwt/dom/builder/shared/StyleBuilder.java:243: * @return a SafeStylescom.google.gwt.safecss.shared.SafeStyles instance oops http://gwt-code-reviews.appspot.com/1455802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
