Thanks for posting the ShowCase url. Looks slick!
http://gwt-code-reviews.appspot.com/1499808/diff/1/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java File samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/1/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java#newcode185 samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java:185: tr.startTH().colSpan(2).rowSpan(2).className(headerStyle + " " + firstColumnStyle); Didn't you already prepend this space to firstColumnStyle in the constructor? http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java File user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java#newcode44 user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java:44: public interface StylesBuilder { I know this isn't the right patch for this conversation, but have you consider the api evolution issue here? At the very least these interfaces need comments warning that they are subject to change, and that custom implementations should expect to get broken, yes? http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/dom/client/Style.java File user/src/com/google/gwt/dom/client/Style.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/dom/client/Style.java#newcode798 user/src/com/google/gwt/dom/client/Style.java:798: private static final String STYLE_LINE_HEIGHT = "lineHeight"; Huge amount of redundancy between these strings and those in c.g.g.dom.client.Style, etc. Do I not worry about that because they all get inlined? Also, Style is missing this lineHeight attribute. Should it be added there at the same time? Is there anything we can do to tie the maintenance of the two packages together? Confused by camel case here but hyphens in SafeStyleUtils (third redundant bag-o-strings). Are they actually different in css and js? http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java File user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java#newcode368 user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:368: * columns. Should be in its own file. AbstractCellTable is huge. http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java#newcode428 user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:428: // Cache the styles for faster access. Here and in the showcase app, did you actually measure improved speed from this caching? http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java#newcode826 user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:826: private class HeaderUtilityImpl extends CellTableHeaderBuilder.Utility<T> { Does Utility really need the param type T? What happens if it deals with Column<?, ?> instead of Column<T, ?>? http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java#newcode896 user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:896: while (section.getDepth() > 0) { Should this loop be added to the builders as endAll()? http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java#newcode1296 user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:1296: // Strip the table section tags off of the tbody. It seems like you have to do this kind of stripping a lot. Can the builders learn to do this for you? Render the inner build only? http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java#newcode2972 user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:2972: if (elem == null) { You sure you want this to mask NPEs? http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java File user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode34 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:34: public abstract class Utility<T> { "Utility" is right up there with "Misc" in the pantheon of non-descriptive names. Even "Helper" would be better. Or Worker? Really, it seems like this thing is the builder, and CellTableHeaderBuilder is the CellTableHeaderDelegate. Can you document just where implementations and instances of this are found? Why is this an abstract class while the builder is an interface? How are you making these choices? http://gwt-code-reviews.appspot.com/1499808/diff/1/user/test/com/google/gwt/user/cellview/client/CellTableTest.java File user/test/com/google/gwt/user/cellview/client/CellTableTest.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/1/user/test/com/google/gwt/user/cellview/client/CellTableTest.java#newcode195 user/test/com/google/gwt/user/cellview/client/CellTableTest.java:195: table.getPresenter().flush(); Why are these calls becoming necessary? http://gwt-code-reviews.appspot.com/1499808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
