LGTM overall.
As a side note, where is the c.g.g.dom.client.EventListener defined? Could it be made generic? I like how the WebSocket proposal added a base Event class mimicking the DOM Event interface, with NativeEvent (and other WebSocket-specific events) extending it. EventListener could then be defined as interface EventListener<T extends Event>. It wouldn't be as type-safe as EventHandler (which "guards" you against adding a handler not matching the event's type) but would save us all doing casts to the specific Event subclass as the first line of all our EventListeners. http://gwt-code-reviews.appspot.com/623803/diff/1/3 File user/src/com/google/gwt/dom/client/DOMImplStandard.java (right): http://gwt-code-reviews.appspot.com/623803/diff/1/3#newcode115 user/src/com/google/gwt/dom/client/DOMImplStandard.java:115: public native void addEventListener(EventTarget target, String type, On 2010/07/02 15:39:30, jat wrote:
How about returning Object here as the opaque handle for remove? Then
you could
just return the callback fuction and avoid the map.
And then people will come complaining that they have to keep a reference to a second object to be able to removeEventListener. Remember the HandlerRegistration.removeHandler() vs. HandlerManager.removeHandler() debate? See http://code.google.com/p/google-web-toolkit/issues/detail?id=3102 and the related issue about now adding removeXxxHandler methods everywhere: http://code.google.com/p/google-web-toolkit/issues/detail?id=4159 There's also the floating idea of issuing warnings in DevMode if you forgot to remove all event handlers, which can cause memory leaks in IE. And finally, it's closer to the DOM-Events model where you pass the exact same object in both addEventListener and removeEventListener. http://gwt-code-reviews.appspot.com/623803/diff/1/4 File user/src/com/google/gwt/dom/client/DOMImplTrident.java (right): http://gwt-code-reviews.appspot.com/623803/diff/1/4#newcode161 user/src/com/google/gwt/dom/client/DOMImplTrident.java:161: // TODO: Hang on to enough information to implement dispose() properly. I guess the implementation would be pretty similar to the DOMImplStandard one re. setFn/getFn, just that setFn would probably also push on a Map<EventTarget,Set<EventListener>> (or use an expando on the EventTarget) which would be used by dispose() In other words, maybe setFn/getFn should be instance methods (non-static) at the DOMImpl level, with DOMImplTrident overriding setFn as explained above. http://gwt-code-reviews.appspot.com/623803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
