Sync the WebSocket interface with the spec

Source/modules/websockets/WebSocket.idl

 /*
  * Copyright (C) 2009 Google Inc.  All rights reserved.
  * Copyright (C) 2010, 2011 Apple Inc. All Rights Reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
@@ skipping 11 matching lines @@
  * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
  * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
  * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
  * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
  * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 // http://dev.w3.org/html5/websockets/#websocket

[philipj_slow, 2015/02/26 03:02:53]

I'm pretty sure this copy of the spec is abandoned, see the date. You can also
tell from the indentation of the IDL block that it isn't being kept in sync with
the source document. In other words, drop this link?

[jsbell, 2015/02/26 19:56:54]

Done.

-// http://www.whatwg.org/specs/web-apps/current-work/multipage/network.html#websocket
+// https://html.spec.whatwg.org/multipage/comms.html#websocket

[philipj_slow, 2015/02/26 03:02:53]

Can you link to
https://html.spec.whatwg.org/multipage/comms.html#the-websocket-interface
instead? Not all interfaces have such a section handy, but when there is one, it
means you get to see the constructor and any enums without scrolling.

[jsbell, 2015/02/26 19:56:53]

Done.

 
 // FIXME: use BinaryType in binaryType

[jsbell, 2015/02/25 18:51:11]

See https://codereview.chromium.org/871013007/ by Paritosh Kumar for a fix for
this

 enum BinaryType { "blob", "arraybuffer" };
 
 [
     ActiveDOMObject,
-    // FIXME: should be optional union type http://crbug.com/240176
-    Constructor(DOMString url, optional DOMString protocol),
-    Constructor(DOMString url, sequence<DOMString> protocols),
+    Constructor(DOMString url, optional (DOMString or sequence<DOMString>) protocols),

[jsbell, 2015/02/25 18:51:11]

Spec has (DOMString or DOMString[]) but it's unclear why T[] would be used
instead of sequence<T>.

[jsbell, 2015/02/25 23:18:55]

I filed this against the HTML spec:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=28102

[philipj_slow, 2015/02/26 03:02:53]

Can you add a FIXME here linking to that spec bug, just in case it should be
rejected? It can just be removed if Hixie does what you ask.

[jsbell, 2015/02/26 19:56:54]

Done.

     ConstructorCallWith=ExecutionContext,
     Exposed=(Window,Worker),
     RaisesException=Constructor,
     GarbageCollected,
     ImplementedAs=DOMWebSocket,
 ] interface WebSocket : EventTarget {
     [DeprecateAs=WebSocketURL] readonly attribute DOMString URL; // Lowercased .url is the one in the spec, but leaving .URL for compatibility reasons.

[philipj_slow, 2015/02/26 03:02:53]

So that it's easy to find all the differences, can you convert this to a FIXME?
I would also LGTM a Intent to Remove it, since it's been deprecated for a long
time and has acceptably low usage:
https://www.chromestatus.com/metrics/feature/timeline/popularity/255

[jsbell, 2015/02/26 19:56:53]

Done.

     readonly attribute DOMString url;
 
     // ready state
     const unsigned short CONNECTING = 0;
     const unsigned short OPEN = 1;
     const unsigned short CLOSING = 2;
     const unsigned short CLOSED = 3;
     readonly attribute unsigned short readyState;
     readonly attribute unsigned long bufferedAmount;
 
     // networking
-             attribute EventHandler onopen;
-             attribute EventHandler onerror;
-             attribute EventHandler onclose;
+    attribute EventHandler onopen;
+    attribute EventHandler onerror;
+    attribute EventHandler onclose;
     [TreatReturnedNullStringAs=Undefined] readonly attribute DOMString extensions;

[philipj_slow, 2015/02/26 03:02:53]

Add FIXMEs for these two [TreatReturnedNullStringAs=Undefined], per spec it
looks like these should always be strings. If there are cases where a null
string is returned internally, we should probably just return an empty string.

[jsbell, 2015/02/26 19:56:54]

Done.

I traced the code back through from Chromium, and they should never be null. But
left the FIXMEs in there for now.

     [TreatReturnedNullStringAs=Undefined] readonly attribute DOMString protocol;
-    [RaisesException] void close([Clamp] optional unsigned short code, optional DOMString reason);
+    [RaisesException] void close([Clamp] optional unsigned short code, optional USVString reason);
 
     // messaging
-             attribute EventHandler onmessage;
-             attribute DOMString binaryType;
-    [RaisesException] void send(DOMString data);
+    attribute EventHandler onmessage;
+    attribute DOMString binaryType;
+    [RaisesException] void send(USVString data);
     [RaisesException] void send(Blob data);

[philipj_slow, 2015/02/26 03:02:53]

Note that without [TypeChecking=Interface], some non-Blob object or null will
result in null being seen on the C++ side. That should never happen per the
spec's IDL. There's some risk to that change, but if you want to add it in a
separate (easily reverted) CL that would be great!

[philipj_slow, 2015/02/26 03:05:59]

Hmm, I'm actually not sure what happens when there are same-name methods with
argument type overloading, there's no single C++ function to call with a null
argument. Maybe I'm just wrong, but it would be worth checking if
[TypeChecking=Interface] changes the generated code.

[jsbell, 2015/02/26 05:45:18]

I'll take a look at the generated code and report back.

[jsbell, 2015/02/26 19:56:54]

The generated code does change, but there's no behavior difference.

With the overload, the type checks are done before dispatching using
V8$TYPE$::hasInstance(), falling back to the string overload. Then in the
dispatched calls, the conversion is done with data =
V8$TYPE$::toImplWithTypeCheck(). If [TypeChecking=Interface] is specified, this
is followed by `if (!data) { ... throwTypeError ... }`. But that should never be
true.

I can add in [TypeChecking=Interface] since we ideally would have that
everywhere, fix the cases where it causes problems, then make it the default. Or
leave it for now to avoid clutter - I have no strong preference.

[philipj_slow, 2015/02/27 04:23:09]

Oh, overloading also changes things a bit. [TypeChecking=Interface] on the
interface would be great, in this CL or later as you see fit.

     [RaisesException] void send(ArrayBuffer data);
     [RaisesException] void send(ArrayBufferView data);
 };