I'm only half through the review, but a number of its links are broken. E.g. I can't see CellBrowser. Maybe try re-uploading?
I'm a bit uncomfortable doing this review without a clearer understanding of the ViewData mechanism, which really isn't documented at all. Can you punch up the design doc, and explain better just what the new behavior is that you're introducing? http://gwt-code-reviews.appspot.com/689801/diff/1001/2001 File bikeshed/src/com/google/gwt/sample/bikeshed/cookbook/client/CellSamplerRecipe.java (right): http://gwt-code-reviews.appspot.com/689801/diff/1001/2001#newcode105 bikeshed/src/com/google/gwt/sample/bikeshed/cookbook/client/CellSamplerRecipe.java:105: this.status = "Active"; Why not use the constant you defined above? For that matter, why not use a status enum and get these display strings from a method on it? http://gwt-code-reviews.appspot.com/689801/diff/1001/2008 File user/src/com/google/gwt/cell/client/Cell.java (right): http://gwt-code-reviews.appspot.com/689801/diff/1001/2008#newcode34 user/src/com/google/gwt/cell/client/Cell.java:34: * associated with a cell value. Passed to the cell by whom? What actually happens when updateViewData is called? If this isn't the place to document that, where is? Don't forget to update the design doc wave with this info. http://gwt-code-reviews.appspot.com/689801/diff/1001/2008#newcode69 user/src/com/google/gwt/cell/client/Cell.java:69: * @param viewData the view data associated with the cell, or null When will this be null? http://gwt-code-reviews.appspot.com/689801/diff/1001/2008#newcode71 user/src/com/google/gwt/cell/client/Cell.java:71: * @param valueUpdater a {...@link ValueUpdater}, or null Ditto http://gwt-code-reviews.appspot.com/689801/diff/1001/2008#newcode72 user/src/com/google/gwt/cell/client/Cell.java:72: * @param viewDataUpdater a {...@link ViewDataUpdater} used to save view data Can it be null? When? http://gwt-code-reviews.appspot.com/689801/diff/1001/2017 File user/src/com/google/gwt/cell/client/ViewDataManager.java (right): http://gwt-code-reviews.appspot.com/689801/diff/1001/2017#newcode25 user/src/com/google/gwt/cell/client/ViewDataManager.java:25: * Handles view data objects. Who uses this class? Is it for cell authors, or are they unlikely to need it? Does it tend to get customized? If so, should it accept a ViewDataUpdaterFactory as an optional constructor arg rather than requiring subclassing? http://gwt-code-reviews.appspot.com/689801/diff/1001/2017#newcode57 user/src/com/google/gwt/cell/client/ViewDataManager.java:57: public ViewDataUpdater createViewDataUpdater(Object key) { Surprising that creating one doesn't also put it in the map. http://gwt-code-reviews.appspot.com/689801/diff/1001/2017#newcode65 user/src/com/google/gwt/cell/client/ViewDataManager.java:65: public void setViewData(Object key, Object viewData) { Should javadoc the null behavior. This api seems to imply that I need to call set after existing view data changes, but you're not doing anything to protect instances returned by get, no defensive copy. If you don't want people to edit the things in place, you need to enforce it. If editing in place is okay, why have create, set and get as separate calls when one would do just fine? http://gwt-code-reviews.appspot.com/689801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
