I agree with most of the comments except passing values as separate arguments from the Context because it doesn't seem to make any difference. Do you have a convincing argument why it should be separated?
http://gwt-code-reviews.appspot.com/1129801/diff/4001/5008 File samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/ContactTreeViewModel.java (right): http://gwt-code-reviews.appspot.com/1129801/diff/4001/5008#newcode199 samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/ContactTreeViewModel.java:199: On 2010/11/24 19:50:06, pdr wrote:
Extra newlines
Done. http://gwt-code-reviews.appspot.com/1129801/diff/4001/5024 File user/src/com/google/gwt/cell/client/Cell.java (right): http://gwt-code-reviews.appspot.com/1129801/diff/4001/5024#newcode65 user/src/com/google/gwt/cell/client/Cell.java:65: context.key = key; It seems sort of arbitrary to exclude the value from the Context, and I don't see what value it adds. Also, the key is paired to the value, so that would have to be a separate arg as well. The Cell is a flyweight, so it shouldn't maintain any state unless absolutely necessary. IMHO, for a flyweight like Cell, the value is tightly bound to the Context where the value is being rendered. http://gwt-code-reviews.appspot.com/1129801/diff/4001/5024#newcode180 user/src/com/google/gwt/cell/client/Cell.java:180: * may chooses to pass keystrokes directly to the cell rather than using them On 2010/11/24 19:50:06, pdr wrote:
chooses -> choose
Done. http://gwt-code-reviews.appspot.com/1129801/diff/4001/5038 File user/src/com/google/gwt/cell/client/TextCell.java (right): http://gwt-code-reviews.appspot.com/1129801/diff/4001/5038#newcode47 user/src/com/google/gwt/cell/client/TextCell.java:47: public void render(Context<String> context, SafeHtml value, SafeHtmlBuilder sb) { The Context has the String value, but "value" has been converted to SafeHtml in this method. TextCell extends AbstractSafeHtmlCell and uses a SafeHtmlRenderer to convert the String value to a SafeHtml value. This is a convenience method for users to override. It might make sense to just revert it to its old form and skip the context. If users want to use the method, they can always override the main render() method. http://gwt-code-reviews.appspot.com/1129801/diff/4001/5041 File user/src/com/google/gwt/user/cellview/client/CellList.java (right): http://gwt-code-reviews.appspot.com/1129801/diff/4001/5041#newcode325 user/src/com/google/gwt/user/cellview/client/CellList.java:325: * I think I convinced myself that you'd never have a conflict in practice, but that probably isn't a safe assumption because you could call getCellContext for two different row indexes from within the same method. I'm also probably being to aggressive about this. We can create one Context per event (user time) and one at the top of the render loop, and there should be any performance impact. http://gwt-code-reviews.appspot.com/1129801/diff/4001/5042 File user/src/com/google/gwt/user/cellview/client/CellTable.java (right): http://gwt-code-reviews.appspot.com/1129801/diff/4001/5042#newcode342 user/src/com/google/gwt/user/cellview/client/CellTable.java:342: final T rowValue, NativeEvent event) { I was being overly aggressive about avoid Context creation. I'll change these to take a Context. This is a private inner class, but the protected version of fireEventToCell should take a Context. http://gwt-code-reviews.appspot.com/1129801/diff/4001/5042#newcode353 user/src/com/google/gwt/user/cellview/client/CellTable.java:353: * Agreed. I'll get ride of the singleton. http://gwt-code-reviews.appspot.com/1129801/diff/4001/5042#newcode1152 user/src/com/google/gwt/user/cellview/client/CellTable.java:1152: CellPreviewEvent<T> previewEvent = CellPreviewEvent.fire(this, event, Because it wasn't committed in when I uploaded this patch. http://gwt-code-reviews.appspot.com/1129801/diff/4001/5043 File user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java (right): http://gwt-code-reviews.appspot.com/1129801/diff/4001/5043#newcode67 user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java:67: // TODO(jlabanca): Convert this to be the type of the child and create lazily. This is a package protected class, so nobody should be referring to it. http://gwt-code-reviews.appspot.com/1129801/diff/4001/5051 File user/test/com/google/gwt/cell/client/CompositeCellTest.java (right): http://gwt-code-reviews.appspot.com/1129801/diff/4001/5051#newcode115 user/test/com/google/gwt/cell/client/CompositeCellTest.java:115: cell.onBrowserEvent(context, event, null); Done. I added more checks to make sure the context has the correct values when passed to the child Cell. http://gwt-code-reviews.appspot.com/1129801/diff/4001/5055 File user/test/com/google/gwt/cell/client/ImageLoadingCellTest.java (right): http://gwt-code-reviews.appspot.com/1129801/diff/4001/5055#newcode61 user/test/com/google/gwt/cell/client/ImageLoadingCellTest.java:61: // Verify the image. It isn't specifically defined, but our Cells should handle it unless we specifically note that they don't. http://gwt-code-reviews.appspot.com/1129801/diff/4001/5061 File user/test/com/google/gwt/user/cellview/client/ColumnTest.java (right): http://gwt-code-reviews.appspot.com/1129801/diff/4001/5061#newcode26 user/test/com/google/gwt/user/cellview/client/ColumnTest.java:26: import com.google.gwt.emultest.java.lang.C; On 2010/11/24 19:50:06, pdr wrote:
Unneeded import
Done. http://gwt-code-reviews.appspot.com/1129801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
