http://gwt-code-reviews.appspot.com/720801/diff/1/4 File user/src/com/google/gwt/cell/client/EditTextCell.java (right):
http://gwt-code-reviews.appspot.com/720801/diff/1/4#newcode231 user/src/com/google/gwt/cell/client/EditTextCell.java:231: } else if ("blur".equals(type)) { Are there really no constants for these strings? http://gwt-code-reviews.appspot.com/720801/diff/1/5 File user/src/com/google/gwt/cell/client/ImageCell.java (right): http://gwt-code-reviews.appspot.com/720801/diff/1/5#newcode20 user/src/com/google/gwt/cell/client/ImageCell.java:20: * of the image. Perhaps note that most GWT apps will be better served by using ImageResourceCell if they can? http://gwt-code-reviews.appspot.com/720801/diff/1/6 File user/src/com/google/gwt/cell/client/ImageLoadingCell.java (right): http://gwt-code-reviews.appspot.com/720801/diff/1/6#newcode36 user/src/com/google/gwt/cell/client/ImageLoadingCell.java:36: static interface Images extends ClientBundle { Interfaces are implicitly static. http://gwt-code-reviews.appspot.com/720801/diff/1/6#newcode50 user/src/com/google/gwt/cell/client/ImageLoadingCell.java:50: } Make Images public and provide a constructor that allows an alternative implementation. (The public Images interface probably shouldn't extend ClientBundle, though.) GWT.create the default one here in the zero args constructor. If there are other cells or widgets with client bundles they should do the same. This is the general scheme I want to see us take for getting ClientBundle integrated into the stock widget set. If you use that pattern here in M3, we have a chance to vet it. http://gwt-code-reviews.appspot.com/720801/diff/1/6#newcode96 user/src/com/google/gwt/cell/client/ImageLoadingCell.java:96: protected void renderErrorHtml(String value, Object key, StringBuilder sb) { Having to subclass to customize things like this kind of sucks. Instead, rename your Images bundle to Resources, and have it include include a few Renderer<String> that are used here and in the other protected methods, and make them private. In fact, if you do that the Resources interface wouldn't need to have that ImageResource on it at all. They could provide any old loading implementation they want, not necessarily an image based one. http://gwt-code-reviews.appspot.com/720801/diff/1/10 File user/src/com/google/gwt/user/cellview/client/CellImpl.java (right): http://gwt-code-reviews.appspot.com/720801/diff/1/10#newcode27 user/src/com/google/gwt/user/cellview/client/CellImpl.java:27: abstract class CellImpl { This name confused me. I thought I was reading code that would be used in various cells, not in the widgets themselves. CellEventDispatchImpl? http://gwt-code-reviews.appspot.com/720801/diff/1/11 File user/src/com/google/gwt/user/cellview/client/CellImplStandard.java (right): http://gwt-code-reviews.appspot.com/720801/diff/1/11#newcode47 user/src/com/google/gwt/user/cellview/client/CellImplStandard.java:47: * Handle an event from a cell. Please add: Used by {...@link #initEventSystem} http://gwt-code-reviews.appspot.com/720801/diff/1/12 File user/src/com/google/gwt/user/cellview/client/CellImplTrident.java (right): http://gwt-code-reviews.appspot.com/720801/diff/1/12#newcode83 user/src/com/google/gwt/user/cellview/client/CellImplTrident.java:83: DOM.sinkEvents(target, eventBits); Could stray uncaught exceptions or something break your clean up scheme? Anything you can do to make it more bullet proof? http://gwt-code-reviews.appspot.com/720801/diff/1/14 File user/src/com/google/gwt/user/cellview/client/CellTable.java (right): http://gwt-code-reviews.appspot.com/720801/diff/1/14#newcode769 user/src/com/google/gwt/user/cellview/client/CellTable.java:769: public void removeColumn(int index) { Shouldn't there be something going on here to unsink events you no longer need? http://gwt-code-reviews.appspot.com/720801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
