On Fri, Jul 2, 2010 at 2:37 AM, <j...@google.com> wrote: > http://gwt-code-reviews.appspot.com/646803/show
All in all, it really feels over-engineered to me. Sorry to be harsh, but let me explain: What's the purpose of RawWebSocketImpl vs. WebSocket? that you could do a new WebSocket() instead of WebSocket.newInstance() ? (I don't dispute the interface+implementation, which is more-than-useful for mockability). When you look at the WebSocketTest, it actually doesn't test anything really useful, only that a 'short' state is correctly converted into a 'State' enum value, and that the generic event-name+EventListener+event-handler-as-Object is correctly mapped to/from event-specific-method+event-specific-handler+HandlerRegistration (actually, it doesn't even test the HandlerRegistration behavior). What this dichotomy buys us? a cleaner API? why couldn't it be built at the interface level (currently named RawWebSocket)? Imagine a WebSocket implemented as the following (using Joel's proposed event API): public class NativeWebSocket extends JavaScriptObject implements WebSocket { public static native NativeWebSocket newInstance(String url, String subProtocol) /*-{ return new WebSocket(url, subProtocol); }-*/; public final State getReadyState() { return State.values()[nativeGetReadyState()]; } public final native String getUrl() /*-{ return this.url; }-*/; // we know "long" has a cost in GWT, so should we use it? // moreover given than Java's double exactly maps to JS's Number public final native double getBufferredAmount() /*-{ return this.bufferredAmount; }-*/; public final native boolean send(String data) /*-{ return this.send(data); }-*/; public final void addOpenListener(EventListener<OpenEvent> listener) { EventTarget.as(this).addEventListener("open", listener); } public final void addCloseListener(EventListener<CloseEvent> listener) { EventTarget.as(this).addEventListener("close", listener); } public final void addErrorListener(EventListener<ErrorEvent> listener) { EventTarget.as(this).addEventListener("error", listener); } public final void addMessageListener(EventListener<MessageEvent> listener) { EventTarget.as(this).addEventListener("message", listener); } public final void removeOpenListener(EventListener<OpenEvent> listener) { EventTarget.as(this).removeEventListener("open", listener); } public final void removeCloseListener(EventListener<CloseEvent> listener) { EventTarget.as(this).removeEventListener("close", listener); } public final void removeErrorListener(EventListener<ErrorEvent> listener) { EventTarget.as(this).removeEventListener("error", listener); } public final void removeMessageListener(EventListener<MessageEvent> listener) { EventTarget.as(this).removeEventListener("message", listener); } private native int nativeGetReadyState /*-{ return this.readyState; }-*/; protected NativeWebSocket() { } } It wouldn't make it impossible to use a non-native implementation of the interface (e.g. using Adobe AIR's sockets, or Flash/Java applet/Silverlight sockets in a browser) on environments/browsers where it's not supported (you'd just have to instantiate it different, either using a factory or dependency injection, and with deferred binding or a runtime supportsWebSocket check) without impacting testability either. If the goal of WebSocket vs. RawWebSocket is to provide a higher-level API (similar to RequestBuilder vs. XmlHttpRequest), then I think it could make use of c.g.g.event, but then I think it should use the whole GwtEvent+EventHandler+HandlerManager suite. But as it is done in this patch, there's not enough differentiation to make the extra-level of indirection worth it IMO. (actually, RawWebSocket (or the WebSocket above) could use setXxxListener methods instead of add/removeXxxListener (and possibly using onxxx instead of add/removeEventListener in the JSNI), as there's really not many reasons to have several listeners for a single event of a single WebSocket –similar to the setDelegate of the new Cell-based widgets) -- http://groups.google.com/group/Google-Web-Toolkit-Contributors