I'll update the change with your recommendations.
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)) {
There are constants defined within the GWT logical events:
BlurEvent.getType().getName()
It feels a little heavyweight though.
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.
On 2010/07/28 20:53:37, Ray Ryan wrote:
Perhaps note that most GWT apps will be better served by using
ImageResourceCell
if they can?
Done.
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 {
On 2010/07/28 20:53:37, Ray Ryan wrote:
Interfaces are implicitly static.
Done.
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) {
I can change this to use Renderers.
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 {
How about CellBasedWidgetImpl?
We may want to add more than just event handling at some point. I'd
call it CellWidgetImpl, but I'm working on a CellWidget (wraps a Cell)
in another CL, so that won't work either.
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.
On 2010/07/28 20:53:37, Ray Ryan wrote:
Please add: Used by {...@link #initEventSystem}
Done.
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);
Its pretty unlikely. The first thing we do in onBrowserEvent() is
remove the event listener, before user code has a chance to throw an
exception.
If a user subclasses a Cell based widget, they could potentially throw
an error, and the event handler won't be removed. But then, a user can
override onAttach/Detach and throw an error, which has the same effect.
Making it more bullet proof wouldn't be easy. We could keep a list of
Elements that have a listeners and remove the listeners onDetach(), but
we'd be adding/removing from the list on every event. Personally, I
think the risk of a leak is pretty small.
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) {
We can't be sure that the Cell is the only reason that the event was
sunk in the first place. For example, the user might use a ButtonCell
that consumes click events, but might also add a ClickHandler to the
Widget, which also requires the click event to be sunk. If we unsink
the click event when we remove the Column with the ButtonCell, it will
cause the ClickHandler to stop firing too.
Most of the performance cost is sinking the event in the first place, so
we only want to sink events we need. There really isn't any penalty in
keeping an event sunk (except maybe if you sink mousemove events). Even
then, its a relatively rare use case to remove columns.
There is a comment at the end of the method explaining why we don't sink
events.
http://gwt-code-reviews.appspot.com/720801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
