Implemented W3C complient web socket client interface

Implemented W3C complient web socket client interface

Fixed a bug where onMessage events could happen before the onOpen event.

R=ager@google.com, ajohnsen@google.com, vsm@google.com, jacobr@google.com

BUG=dart:2001
TEST=tests/standalone/io/web_socket_test.dart

Committed: https://code.google.com/p/dart/source/detail?r=7440

[Mads Ager (google), 2012-05-07 11:46:57]

I'd like to proof-read the doc comments for the public interface. The
implementation looks good!

https://chromiumcodereview.appspot.com/10383039/diff/1/runtime/bin/websocket....
File runtime/bin/websocket.dart (right):

https://chromiumcodereview.appspot.com/10383039/diff/1/runtime/bin/websocket....
runtime/bin/websocket.dart:162: WebSocket(String url, [protocols]);
What is protocols? Is there a type that it makes sense to use in this interface?
List<String>?

Also, this is the public API, please add doc comments on everything.

https://chromiumcodereview.appspot.com/10383039/diff/1/runtime/bin/websocket_...
File runtime/bin/websocket_impl.dart (right):

https://chromiumcodereview.appspot.com/10383039/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:766: client.shutdown();
Maybe do 'client = null' as well?

https://chromiumcodereview.appspot.com/10383039/diff/1/runtime/bin/websocket_...
runtime/bin/websocket_impl.dart:795: void set onerror(Function callback) {
I would add a blank line before each multi-line method.

[Søren Gjesse, 2012-05-07 12:55:24]

Addressed comments from ager@

Fixed a bug related to the onOpen event.

http://codereview.chromium.org/10383039/diff/1/runtime/bin/websocket.dart
File runtime/bin/websocket.dart (right):

http://codereview.chromium.org/10383039/diff/1/runtime/bin/websocket.dart#new...
runtime/bin/websocket.dart:162: WebSocket(String url, [protocols]);
On 2012/05/07 11:46:57, Mads Ager wrote:
> What is protocols? Is there a type that it makes sense to use in this
interface?
> List<String>?
> 
> Also, this is the public API, please add doc comments on everything.

Sorry abut that. I totally forgot the comments on the public API. Added.

http://codereview.chromium.org/10383039/diff/1/runtime/bin/websocket_impl.dart
File runtime/bin/websocket_impl.dart (right):

http://codereview.chromium.org/10383039/diff/1/runtime/bin/websocket_impl.dar...
runtime/bin/websocket_impl.dart:766: client.shutdown();
On 2012/05/07 11:46:57, Mads Ager wrote:
> Maybe do 'client = null' as well?

Done.

http://codereview.chromium.org/10383039/diff/1/runtime/bin/websocket_impl.dar...
runtime/bin/websocket_impl.dart:795: void set onerror(Function callback) {
On 2012/05/07 11:46:57, Mads Ager wrote:
> I would add a blank line before each multi-line method.

Done.

[Mads Ager (google), 2012-05-07 13:01:35]

lgtm

https://chromiumcodereview.appspot.com/10383039/diff/1006/runtime/bin/websock...
File runtime/bin/websocket.dart (right):

https://chromiumcodereview.appspot.com/10383039/diff/1006/runtime/bin/websock...
runtime/bin/websocket.dart:161: bool get wasClean();
Document the getters?

https://chromiumcodereview.appspot.com/10383039/diff/1006/runtime/bin/websock...
runtime/bin/websocket.dart:184: * [:String:] or [:List<String>:] specifying the
subprotocols the
Should we just make this List<String> to simplify the interface or do you find
['one_protocol'] too annoying to write?

[Søren Gjesse, 2012-05-07 13:59:20]

https://chromiumcodereview.appspot.com/10383039/diff/1006/runtime/bin/websock...
File runtime/bin/websocket.dart (right):

https://chromiumcodereview.appspot.com/10383039/diff/1006/runtime/bin/websock...
runtime/bin/websocket.dart:161: bool get wasClean();
On 2012/05/07 13:01:35, Mads Ager wrote:
> Document the getters?

Done.

https://chromiumcodereview.appspot.com/10383039/diff/1006/runtime/bin/websock...
runtime/bin/websocket.dart:184: * [:String:] or [:List<String>:] specifying the
subprotocols the
On 2012/05/07 13:01:35, Mads Ager wrote:
> Should we just make this List<String> to simplify the interface or do you find
> ['one_protocol'] too annoying to write?

The constructor defined in http://dev.w3.org/html5/websockets/ is

[Constructor(DOMString url, optional (DOMString or DOMString[]) protocols)]
interface WebSocket : EventTarget {
  ...
}

so to match that we need to support both.

[vsm, 2012-05-07 15:16:07]

LGTM

At some point, we need to reconcile this with the version in dart:html.  It'd be
nice to factor this out of dart:io/html into a common lib.

https://chromiumcodereview.appspot.com/10383039/diff/1007/runtime/bin/websock...
File runtime/bin/websocket.dart (right):

https://chromiumcodereview.appspot.com/10383039/diff/1007/runtime/bin/websock...
runtime/bin/websocket.dart:147: interface WebSocketDataEvent default
_WebSocketDataEvent {
Should this correspond to an HTML MessageEvent?

https://chromiumcodereview.appspot.com/10383039/diff/1007/runtime/bin/websock...
runtime/bin/websocket.dart:200: WebSocket(String url, [protocols]);
Just an fyi - it looks like protocols is now supported in WebKit but not
surfaced in the Dart dom/html API.  I'll file a bug.

[Jacob, 2012-05-07 16:31:51]

LGTM

https://chromiumcodereview.appspot.com/10383039/diff/1007/runtime/bin/websock...
File runtime/bin/websocket.dart (right):

https://chromiumcodereview.appspot.com/10383039/diff/1007/runtime/bin/websock...
runtime/bin/websocket.dart:160: interface WebSocketCloseEvent default
_WebSocketCloseEvent {
Is there a reason you don't want to have a base event interface?

https://chromiumcodereview.appspot.com/10383039/diff/1007/runtime/bin/websock...
runtime/bin/websocket.dart:222: void set onerror(Function callback);
can these callbacks have a type?

[Søren Gjesse, 2012-05-09 07:53:43]

I have opened http://code.google.com/p/dart/issues/detail?id=2958 to track
making the interfaces match.

https://chromiumcodereview.appspot.com/10383039/diff/1007/runtime/bin/websock...
File runtime/bin/websocket.dart (right):

https://chromiumcodereview.appspot.com/10383039/diff/1007/runtime/bin/websock...
runtime/bin/websocket.dart:147: interface WebSocketDataEvent default
_WebSocketDataEvent {
On 2012/05/07 15:16:07, vsm wrote:
> Should this correspond to an HTML MessageEvent?

Yes it should. Renamed to MessageEvent. It still only has the data property, as
I am not sure what to return for the other properties. We can iterate on this.

https://chromiumcodereview.appspot.com/10383039/diff/1007/runtime/bin/websock...
runtime/bin/websocket.dart:160: interface WebSocketCloseEvent default
_WebSocketCloseEvent {
On 2012/05/07 16:31:51, Jacob wrote:
> Is there a reason you don't want to have a base event interface?

No - I have added an Event interface.

https://chromiumcodereview.appspot.com/10383039/diff/1007/runtime/bin/websock...
runtime/bin/websocket.dart:200: WebSocket(String url, [protocols]);
On 2012/05/07 15:16:07, vsm wrote:
> Just an fyi - it looks like protocols is now supported in WebKit but not
> surfaced in the Dart dom/html API.  I'll file a bug.

OK - I also still have to handle protocols in the standalone VM implementation.

https://chromiumcodereview.appspot.com/10383039/diff/1007/runtime/bin/websock...
runtime/bin/websocket.dart:222: void set onerror(Function callback);
On 2012/05/07 16:31:51, Jacob wrote:
> can these callbacks have a type?

Sure, added type.

[Jacob, 2012-05-09 17:06:43]

lgtm

https://chromiumcodereview.appspot.com/10383039/diff/4002/runtime/bin/websock...
File runtime/bin/websocket.dart (right):

https://chromiumcodereview.appspot.com/10383039/diff/4002/runtime/bin/websock...
runtime/bin/websocket.dart:148: interface Event { }
It would be nice if this event interface could be more consistent with w3c.  For
example, expose an event type as a string.