[gwt-contrib] Re: Add WebSocket API (issue646803)
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(EventListenerOpenEvent listener) {
EventTarget.as(this).addEventListener(open, listener);
}
public final void addCloseListener(EventListenerCloseEvent listener) {
EventTarget.as(this).addEventListener(close, listener);
}
public final void addErrorListener(EventListenerErrorEvent listener) {
EventTarget.as(this).addEventListener(error, listener);
}
public final void addMessageListener(EventListenerMessageEvent listener) {
EventTarget.as(this).addEventListener(message, listener);
}
public final void removeOpenListener(EventListenerOpenEvent listener) {
EventTarget.as(this).removeEventListener(open, listener);
}
public final void removeCloseListener(EventListenerCloseEvent listener) {
EventTarget.as(this).removeEventListener(close, listener);
}
public final void removeErrorListener(EventListenerErrorEvent listener) {
EventTarget.as(this).removeEventListener(error, listener);
}
public final void removeMessageListener(EventListenerMessageEvent
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
[gwt-contrib] Re: Add WebSocket API (issue646803)
Here's what I think... 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). Different browsers might have a slightly different WebSocket object implementation, so it seems to be necessary to be able to inject different implementations for different browsers. The other thing is that it feels much more natural to instantiate a java objet using syntax like new WebSocket() compared to WebSocket.newInstance(). Third, it adds to the testability. 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). I reckon more test will be added. This is not a final implementation. (Up in the thread it says that this is not ready to submit yet, this is a work in progrss. Right, John?) What this dichotomy buys us? a cleaner API? why couldn't it be built at the interface level (currently named RawWebSocket)? RawWebSocket is the contract that each RawWebSocket implementation needs to adhere to. I'm not sure what you would put here... http://gwt-code-reviews.appspot.com/646803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add WebSocket API (issue646803)
I don't have a particularly strong opinion on the RawWebSocket vs. RawWebSocketImpl dichotomy, but it might be a bit more abstracted that necessary. I'm perfectly fine with the WebSocket vs. RawWebSocket difference, though, because one's a native browser class, while the other provides a more Java-centric interface. Where I think there's a breakdown, though, is that the WebSocket*Handler interface, which seem to be meant to mimic the widget-level event handlers, actually pass event objects that are JSO subtypes. This is mixing up the DOM/Java levels of abstraction in a confusing way. FWIW, I think it might be worth trying to land this in two phases: 1. Native/DOM-only stuff. The rawest possible classes necessary to interface with WebSocket. 2. Java-friendly wrappers, possibly in a separate package. This distinction is much like that between XMLHttpRequest (raw native interface) and RequestBuilder (Java-friendly, built on top of it). http://gwt-code-reviews.appspot.com/646803/diff/16001/17013 File user/src/com/google/gwt/websocket/client/RawWebSocket.java (right): http://gwt-code-reviews.appspot.com/646803/diff/16001/17013#newcode35 user/src/com/google/gwt/websocket/client/RawWebSocket.java:35: boolean useCapture); An alternative to this approach would be to use the standard EventTarget.addEventListener() signature as described in http://gwt-code-reviews.appspot.com/623803 It's a little hairier to implement, because of the need to map EventListener instances back to the lambda that wraps them, but it's easier on the user. http://gwt-code-reviews.appspot.com/646803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Add WebSocket API (issue646803)
On 2 July 2010 15:37, markovuksano...@gmail.com wrote: Here's what I think... 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). Different browsers might have a slightly different WebSocket object implementation, so it seems to be necessary to be able to inject different implementations for different browsers. The other thing is that it feels much more natural to instantiate a java objet using syntax like new WebSocket() compared to WebSocket.newInstance(). Third, it adds to the testability. more natural ? =) The xx.newInstance concept is used in the JDK itself at many places, _because_ its a natural fit for many cases. A few examples: ByteBuffer.allocate SocketChannel.open -- regards gustav -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add WebSocket API (issue646803)
http://gwt-code-reviews.appspot.com/646803/diff/1/15 File user/src/com/google/gwt/dom/client/WebSocket.java (right): http://gwt-code-reviews.appspot.com/646803/diff/1/15#newcode48 user/src/com/google/gwt/dom/client/WebSocket.java:48: }; Is it really necessary to remove all the listeners once the connection is closed. I think the WebSocket specs says that a connection cannot be revived (reopened) - so, I think, that setting rawWebSocket to null (that is the underlying js object) when the connection is closed would let the GC collect that object with all the callbacks associates with it? Is that right? http://gwt-code-reviews.appspot.com/646803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add WebSocket API (issue646803)
http://gwt-code-reviews.appspot.com/646803/diff/1/10
File user/src/com/google/gwt/dom/client/Event.java (right):
http://gwt-code-reviews.appspot.com/646803/diff/1/10#newcode23
user/src/com/google/gwt/dom/client/Event.java:23: public class Event
extends JavaScriptObject {
Could somebody verify that correct methods are moved from NativeEvent
here?
http://gwt-code-reviews.appspot.com/646803/diff/1/12
File user/src/com/google/gwt/dom/client/MessageEvent.java (right):
http://gwt-code-reviews.appspot.com/646803/diff/1/12#newcode30
user/src/com/google/gwt/dom/client/MessageEvent.java:30: public final
String getData() {
Mayebe we could just retrieve the data using natve method and returning
this.data;
not that the result would be different, but just to keep retrieving
information from event in the same way as the other two properties.
http://gwt-code-reviews.appspot.com/646803/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add WebSocket API (issue646803)
http://gwt-code-reviews.appspot.com/646803/diff/1/15
File user/src/com/google/gwt/dom/client/WebSocket.java (right):
http://gwt-code-reviews.appspot.com/646803/diff/1/15#newcode48
user/src/com/google/gwt/dom/client/WebSocket.java:48: };
Just forget my last comment :) I misinterpreted the code here when first
looking at it...
http://gwt-code-reviews.appspot.com/646803/diff/1/20
File user/test/com/google/gwt/dom/client/WebSocketTest.java (right):
http://gwt-code-reviews.appspot.com/646803/diff/1/20#newcode174
user/test/com/google/gwt/dom/client/WebSocketTest.java:174: private
static MockRawWebSocket mockRawSocket;
What about adding a protected constructor into WebSocket which would
accept an additional parameter which would be the mockRawWebSocket
@SuppressWarnings(unused)
protected WebSocket(String url, String subProtocol, RawWebSocket
socket) {
rawWebSocket = socket;
}
and here in test you would have:
private static class MockWebSocket extends WebSocket {
public MockWebSocket(String url, String subProtocol,
MockRawWebSocket mockRawSocket) {
super(url, subProtocol, mockRawSocket);
}
public static MockWebSocket create(MockRawWebSocket mockRawSocket) {
return new MockWebSocket(mockRawSocket.getURL(), null,
mockRawSocket);
}
}
I think that hack would be gone - though haven't tried this yet.
http://gwt-code-reviews.appspot.com/646803/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add WebSocket API (issue646803)
http://gwt-code-reviews.appspot.com/646803/diff/1/12
File user/src/com/google/gwt/dom/client/MessageEvent.java (right):
http://gwt-code-reviews.appspot.com/646803/diff/1/12#newcode30
user/src/com/google/gwt/dom/client/MessageEvent.java:30: public final
String getData() {
On 2010/07/01 11:20:05, markovuksanovic wrote:
Mayebe we could just retrieve the data using natve method and
returning
this.data;
not that the result would be different, but just to keep retrieving
information
from event in the same way as the other two properties.
I suspect so -- I assumed you had a reason to go through DOMImpl, such
as some browsers might name the property something different. If that
isn't the case, then we can just directly reference it here.
http://gwt-code-reviews.appspot.com/646803/diff/1/20
File user/test/com/google/gwt/dom/client/WebSocketTest.java (right):
http://gwt-code-reviews.appspot.com/646803/diff/1/20#newcode174
user/test/com/google/gwt/dom/client/WebSocketTest.java:174: private
static MockRawWebSocket mockRawSocket;
On 2010/07/01 13:10:33, markovuksanovic wrote:
What about adding a protected constructor into WebSocket which would
accept an
additional parameter which would be the mockRawWebSocket
@SuppressWarnings(unused)
protected WebSocket(String url, String subProtocol, RawWebSocket
socket) {
rawWebSocket = socket;
}
and here in test you would have:
private static class MockWebSocket extends WebSocket {
public MockWebSocket(String url, String subProtocol,
MockRawWebSocket
mockRawSocket) {
super(url, subProtocol, mockRawSocket);
}
public static MockWebSocket create(MockRawWebSocket mockRawSocket)
{
return new MockWebSocket(mockRawSocket.getURL(), null,
mockRawSocket);
}
}
I think that hack would be gone - though haven't tried this yet.
Done.
http://gwt-code-reviews.appspot.com/646803/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Add WebSocket API (issue646803)
Hi there, Actually the open events might look broken but after looking carefully into W3C's spec, I have the feeling they aren't. At point 5 of the WebSocket constructor it says Return a new WebSockethttp://dev.w3.org/html5/websockets/#websocket object, and continue these steps in the background (without blocking scripts) which is followed by the connection procedure, and then later in the spec is says When the *WebSocket connection is established*, the user agent must queue a task to first change. So, to my understanding, if a user instantiates a WebSocket, and then adds a listener to it, it will only be at the ent of the task that onopen could be firing (due to the queing mechanism employed before firing), so the user would not miss the event. Hope I am not adding to the confusion ... just my two cents Andi Mullaraj On Wed, Jun 30, 2010 at 9:56 PM, j...@google.com wrote: This isn't ready to submit yet, just wanted to show the changes I was making based on discussions with Joel and JohnL about how the events should work. I think the open events are broken as designed (please correct me if I am wrong), and it would be pretty expensive for us to try and work around it (we could add our own wrapper with its own callbacks, and when we actually instantiate the underlying WebSocket object, we could check if it changed to OPEN before the callbacks were registered and fake it ourselves). http://gwt-code-reviews.appspot.com/646803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add WebSocket API (issue646803)
In a few words: everything's in c.g.g.dom, bringing a dependency from
c.g.g.dom to c.g.g.event and breaking the assumption that an
EventHandler is coupled with a GwtEvent (rather than a JSO-derivative),
and it starts to refactor event handling at the low-level, while Joel is
working on it (see
https://wave.google.com/wave/waveref/googlewave.com/w+ux7zL81XA ).
I'd rather:
- introduce a c.g.g.websocket module
- depend on what's being discussed in the Refactoring event handling
Wave (cast to EventTarget and use EventTarget.addEventListener), or at
least make sure it could be moved to that API/model later on (I haven't
looked closely but it might already be the case in this proposal, just
that I think EventListener and the like should probably be a distinct
review); WebSocket support shouldn't have any impact in itself on
c.g.g.dom
- use GwtEvent/EventHandler pairs (if performance is an issue –it has
been benchmarked as not being one for other very-frequent events such as
mousemove– then use callbacks instead)
My 2c.
http://gwt-code-reviews.appspot.com/646803/diff/1/3
File user/src/com/google/gwt/core/client/impl/RawWebSocketImpl.java
(right):
http://gwt-code-reviews.appspot.com/646803/diff/1/3#newcode46
user/src/com/google/gwt/core/client/impl/RawWebSocketImpl.java:46: var
callback = function(e) {
Shouldn't there be an $entry() here?
http://gwt-code-reviews.appspot.com/646803/diff/1/15
File user/src/com/google/gwt/dom/client/WebSocket.java (right):
http://gwt-code-reviews.appspot.com/646803/diff/1/15#newcode16
user/src/com/google/gwt/dom/client/WebSocket.java:16: package
com.google.gwt.dom.client;
Why isn't all of this on some new module? e.g. com.google.gwt.websocket?
http://gwt-code-reviews.appspot.com/646803/diff/1/16
File user/src/com/google/gwt/dom/client/WebSocketCloseHandler.java
(right):
http://gwt-code-reviews.appspot.com/646803/diff/1/16#newcode18
user/src/com/google/gwt/dom/client/WebSocketCloseHandler.java:18: import
com.google.gwt.event.shared.EventHandler;
This introduces a dependency from dom to event :-(
http://gwt-code-reviews.appspot.com/646803/diff/1/16#newcode22
user/src/com/google/gwt/dom/client/WebSocketCloseHandler.java:22: void
onClose(CloseEvent event);
CloseEvent (and related) should really be a kind of GwtEvent IMO.
http://gwt-code-reviews.appspot.com/646803/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add WebSocket API (issue646803)
http://gwt-code-reviews.appspot.com/646803/diff/1/16 File user/src/com/google/gwt/dom/client/WebSocketCloseHandler.java (right): http://gwt-code-reviews.appspot.com/646803/diff/1/16#newcode22 user/src/com/google/gwt/dom/client/WebSocketCloseHandler.java:22: void onClose(CloseEvent event); I must say that I wouldn't agree about this. This models DOM event and I don't think it should be a GwtEvent. http://gwt-code-reviews.appspot.com/646803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add WebSocket API (issue646803)
http://gwt-code-reviews.appspot.com/646803/diff/1/16 File user/src/com/google/gwt/dom/client/WebSocketCloseHandler.java (right): http://gwt-code-reviews.appspot.com/646803/diff/1/16#newcode22 user/src/com/google/gwt/dom/client/WebSocketCloseHandler.java:22: void onClose(CloseEvent event); I must say that I wouldn't agree about this. This models DOM event and I don't think it should be a GwtEvent. http://gwt-code-reviews.appspot.com/646803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add WebSocket API (issue646803)
http://gwt-code-reviews.appspot.com/646803/diff/1/15 File user/src/com/google/gwt/dom/client/WebSocket.java (right): http://gwt-code-reviews.appspot.com/646803/diff/1/15#newcode16 user/src/com/google/gwt/dom/client/WebSocket.java:16: package com.google.gwt.dom.client; +1 http://gwt-code-reviews.appspot.com/646803/diff/1/16 File user/src/com/google/gwt/dom/client/WebSocketCloseHandler.java (right): http://gwt-code-reviews.appspot.com/646803/diff/1/16#newcode22 user/src/com/google/gwt/dom/client/WebSocketCloseHandler.java:22: void onClose(CloseEvent event); +1/2 I though we said that we would have a parallel hierarchy of events. That being said, I'm not exactly sure what the logical wrapper buys us. We used logical wrappers around NativeEvent because NativeEvent is a dumping ground of methods, so the logical events limit the API to the applicable methods. Since the native web socket events are already limited, I'm not sure we need the logical event. We might need them to define the TYPE, which is required to use the EventHandler mechanism in GWT. http://gwt-code-reviews.appspot.com/646803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add WebSocket API (issue646803)
http://gwt-code-reviews.appspot.com/646803/diff/1/16 File user/src/com/google/gwt/dom/client/WebSocketCloseHandler.java (right): http://gwt-code-reviews.appspot.com/646803/diff/1/16#newcode22 user/src/com/google/gwt/dom/client/WebSocketCloseHandler.java:22: void onClose(CloseEvent event); I think CloseEvent specifically doesn't buy anything. I also think it could be easily modeled by just using nativeEvent but there are 3 other events - error, open, message. MessageEvent contains some specific information not found in other events. I think the model with 4 events (even though it might be almost dead code in the other 3 events) is much more intuitive and reflects much better the WebSockets browser API. http://gwt-code-reviews.appspot.com/646803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add WebSocket API (issue646803)
http://gwt-code-reviews.appspot.com/646803/diff/1/16 File user/src/com/google/gwt/dom/client/WebSocketCloseHandler.java (right): http://gwt-code-reviews.appspot.com/646803/diff/1/16#newcode18 user/src/com/google/gwt/dom/client/WebSocketCloseHandler.java:18: import com.google.gwt.event.shared.EventHandler; On 2010/07/01 16:04:46, tbroyer wrote: This introduces a dependency from dom to event :-( EventHandler is just a marker interface for any event handlers. Maybe if we aren't going through HandlerManager we don't need it. http://gwt-code-reviews.appspot.com/646803/diff/1/16#newcode22 user/src/com/google/gwt/dom/client/WebSocketCloseHandler.java:22: void onClose(CloseEvent event); On 2010/07/01 17:59:33, markovuksanovic wrote: I think CloseEvent specifically doesn't buy anything. I also think it could be easily modeled by just using nativeEvent but there are 3 other events - error, open, message. MessageEvent contains some specific information not found in other events. I think the model with 4 events (even though it might be almost dead code in the other 3 events) is much more intuitive and reflects much better the WebSockets browser API. I think the extra hierarchy doesn't buy us much, and it doesn't all optimize away. I think a reasonable compromise would be pure Java interfaces implemented by corresponding JSOs, so you get mockability for testing without extra wrappers. http://gwt-code-reviews.appspot.com/646803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add WebSocket API (issue646803)
http://gwt-code-reviews.appspot.com/646803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add WebSocket API (issue646803)
This isn't ready to submit yet, just wanted to show the changes I was making based on discussions with Joel and JohnL about how the events should work. I think the open events are broken as designed (please correct me if I am wrong), and it would be pretty expensive for us to try and work around it (we could add our own wrapper with its own callbacks, and when we actually instantiate the underlying WebSocket object, we could check if it changed to OPEN before the callbacks were registered and fake it ourselves). http://gwt-code-reviews.appspot.com/646803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
