On Fri, Jul 2, 2010 at 2:37 AM, <[email protected]> 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
