Last Comment Bug 775368 - Porting Websockets to WebIDL
: Porting Websockets to WebIDL
Status: RESOLVED FIXED
: addon-compat
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla18
Assigned To: Andrea Marchesini [:baku]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 777066 778044 785319
Blocks: ParisBindings 784686 504553 817669
  Show dependency treegraph
 
Reported: 2012-07-18 17:07 PDT by Andrea Marchesini [:baku]
Modified: 2012-12-04 05:22 PST (History)
10 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
websocket + webIDL - first patch (62.03 KB, patch)
2012-07-20 15:37 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
Bug 775368, Porting Websockets to WebIDL (64.01 KB, patch)
2012-07-24 14:02 PDT, Andrea Marchesini [:baku]
khuey: review-
Details | Diff | Splinter Review
Bug 775368, Porting Websockets to WebIDL (63.62 KB, patch)
2012-07-25 19:08 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
Bug 775368, Porting Websockets to WebIDL - delta from the previous patch (53.47 KB, patch)
2012-07-26 09:44 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
Bug 775368, Porting Websockets to WebIDL (78.45 KB, patch)
2012-07-26 09:46 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
Bug 775368, Porting Websockets to WebIDL (81.65 KB, patch)
2012-07-26 11:57 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
Bug 775368, Porting Websockets to WebIDL (81.61 KB, patch)
2012-07-26 14:17 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
Bug 775368, Porting Websockets to WebIDL - delta from the previous patch (1.78 KB, patch)
2012-07-26 14:18 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
Bug 775368, Porting Websockets to WebIDL (81.40 KB, patch)
2012-07-27 16:01 PDT, Andrea Marchesini [:baku]
bzbarsky: review+
Details | Diff | Splinter Review
Bug 775368, Porting Websockets to WebIDL (81.16 KB, text/plain)
2012-08-05 10:46 PDT, Andrea Marchesini [:baku]
no flags Details
Bug 775368, Porting Websockets to WebIDL (81.54 KB, patch)
2012-08-23 14:35 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 2 (81.35 KB, patch)
2012-08-28 02:26 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch 3 (97.07 KB, patch)
2012-09-05 06:27 PDT, Andrea Marchesini [:baku]
bzbarsky: review-
Details | Diff | Splinter Review
patch 4 (91.67 KB, patch)
2012-09-09 04:23 PDT, Andrea Marchesini [:baku]
bzbarsky: review+
Details | Diff | Splinter Review
patch 5 (92.11 KB, patch)
2012-09-10 00:57 PDT, Andrea Marchesini [:baku]
amarchesini: review+
amarchesini: checkin+
Details | Diff | Splinter Review
patch 6 (91.87 KB, patch)
2012-09-10 06:44 PDT, Andrea Marchesini [:baku]
amarchesini: review+
amarchesini: checkin+
Details | Diff | Splinter Review

Description Andrea Marchesini [:baku] 2012-07-18 17:07:33 PDT
Porting Websockets to WebIDL
Comment 1 Andrea Marchesini [:baku] 2012-07-20 15:37:33 PDT
Created attachment 644504 [details] [diff] [review]
websocket + webIDL - first patch
Comment 2 Andrea Marchesini [:baku] 2012-07-20 16:39:32 PDT
I'm going to remove the contract ID of the websocket ("@mozilla.org/websocket;1") because this is needed just for XPCOM and not for WebIDL.
I would like to know if, doing this, we are breaking some addons. Can someone check this?
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-07-20 18:00:14 PDT
I don't see any hits on that contract in the addons mxr, for what it's worth.
Comment 4 Jorge Villalobos [:jorgev] 2012-07-23 08:38:23 PDT
Yes, it doesn't look like anyone is using it.
Comment 5 Andrea Marchesini [:baku] 2012-07-24 14:02:17 PDT
Created attachment 645486 [details] [diff] [review]
Bug 775368, Porting Websockets to WebIDL

The constructor will be changed once the bug 775844 is closed.
Comment 6 Andrea Marchesini [:baku] 2012-07-24 14:08:41 PDT
777066 has been created just to remind me that I have to use sequences in the constructor.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-25 10:41:33 PDT
Comment on attachment 645486 [details] [diff] [review]
Bug 775368, Porting Websockets to WebIDL

Review of attachment 645486 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/WebSocket.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * The origin of this IDL file is
> + * www.w3.org/TR/2012/WD-XMLHttpRequest-20120117/

No it's not!

@@ +9,5 @@
> + * Copyright © 2012 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C
> + * liability, trademark and document use rules apply.
> + */
> +
> +[Constructor(DOMString url, optional any protocols)]

Please file a bug on changing this to not be 'any'.

@@ +28,5 @@
> +  readonly attribute unsigned long bufferedAmount;
> +
> +  // networking
> +
> +  [TreatNonCallableAsNull, GetterInfallible=MainThread]

there's no need for the =MainThread bit, WebSockets only work on workers.

@@ +44,5 @@
> +  [Infallible]
> +  readonly attribute DOMString protocol;
> +
> +  [GetterInfallible=MainThread]
> +  void close(optional unsigned short code, optional DOMString reason);

GetterInfallible doesn't apply to methods.  This should either be 'Infallible' or empty.

We should finish my patches to implement [Clamp]!

@@ +51,5 @@
> +
> +  [TreatNonCallableAsNull, GetterInfallible=MainThread]
> +  attribute Function? onmessage;
> +
> +  attribute DOMString binaryType;

Why is this one fallible?

@@ +54,5 @@
> +
> +  attribute DOMString binaryType;
> +
> +  [GetterInfallible=MainThread]
> +  void send(any data);

GetterInfallible doesn't apply to methods.  This should either be 'Infallible' or empty.

Also there should be multiple signatures for the different types (just like there are in the spec), no?
Comment 8 Andrea Marchesini [:baku] 2012-07-25 19:08:09 PDT
Created attachment 645994 [details] [diff] [review]
Bug 775368, Porting Websockets to WebIDL

. It's green on tryserver
. binaryType is fallible because the SetBinaryType can fail
Thanks for your review!
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2012-07-26 02:21:34 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #7)
> @@ +44,5 @@
> > +  [Infallible]
> > +  readonly attribute DOMString protocol;
> > +
> > +  [GetterInfallible=MainThread]
> > +  void close(optional unsigned short code, optional DOMString reason);
> 
> GetterInfallible doesn't apply to methods.  This should either be
> 'Infallible' or empty.
> 
> We should finish my patches to implement [Clamp]!

I'll see if I can get that done.
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2012-07-26 02:35:14 PDT
Comment on attachment 645994 [details] [diff] [review]
Bug 775368, Porting Websockets to WebIDL

Review of attachment 645994 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsWebSocket.cpp
@@ +49,4 @@
>  #include "nsDOMEventTargetHelper.h"
>  #include "nsIObserverService.h"
>  #include "GeneratedEvents.h"
> +#include "XPCQuickStubs.h"

Why do you need this? If you don't, remove it, please.

@@ +621,5 @@
> +    }
> +  }
> +
> +  nsRefPtr<nsWebSocket> webSocket = new nsWebSocket();
> +  nsresult nv = webSocket->Init(principal, scriptContext, ownerWindow, aUrl, protocolArray, aRv);

nsresult rv

@@ +622,5 @@
> +  }
> +
> +  nsRefPtr<nsWebSocket> webSocket = new nsWebSocket();
> +  nsresult nv = webSocket->Init(principal, scriptContext, ownerWindow, aUrl, protocolArray, aRv);
> +  if (nv != NS_OK) {

if (NS_FAILED(rv)) {

@@ +635,5 @@
> +                           nsIScriptContext* aScriptContext,
> +                           nsPIDOMWindow* aOwnerWindow,
> +                           const nsAString& aURL,
> +                           nsTArray<nsString>& aProtocolArray,
> +                           ErrorResult& aRv)

Don't pass aRv; you're not using it.

@@ +670,5 @@
> +
> +  nsCOMPtr<nsIJSContextStack> stack =
> +    do_GetService("@mozilla.org/js/xpc/ContextStack;1");
> +  JSContext* cx = nsnull;
> +  if (stack && NS_SUCCEEDED(stack->Peek(&cx)) && cx) {

Do we push the context on the stack with the new bindings?

::: content/base/src/nsWebSocket.h
@@ +16,5 @@
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/dom/TypedArray.h"
> +
> +#include "jsfriendapi.h"
> +#include "nsContentUtils.h"

Are you using ContentUtils in the header? If not, don't include it.

@@ +37,5 @@
>  #define DEFAULT_WS_SCHEME_PORT  80
>  #define DEFAULT_WSS_SCHEME_PORT 443
>  
> +namespace mozilla {
> +namespace dom {

If you're going to do this, call the class mozilla::dom::WebSocket, not mozilla::dom::nsWebSocket.

::: dom/webidl/WebSocket.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * The origin of this IDL file is
> + * http://dev.w3.org/html5/websockets/

http://www.whatwg.org/html/#network

@@ +6,5 @@
> + * The origin of this IDL file is
> + * http://dev.w3.org/html5/websockets/
> + *
> + * Copyright © 2012 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C
> + * liability, trademark and document use rules apply.

© Copyright 2004-2011 Apple Computer, Inc., Mozilla Foundation, and Opera Software ASA.

You are granted a license to use, reproduce and create derivative works of this document.
Comment 11 Olli Pettay [:smaug] 2012-07-26 02:46:29 PDT
Do we want to remove the old interface? How should binary stuff use websocket without it?
Comment 12 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-26 07:03:14 PDT
(In reply to Olli Pettay [:smaug] (limited connectivity July 26-29) from comment #11)
> Do we want to remove the old interface? How should binary stuff use
> websocket without it?

It shouldn't!
Comment 13 Olli Pettay [:smaug] 2012-07-26 07:52:10 PDT
Why not?
Comment 14 Andrea Marchesini [:baku] 2012-07-26 09:41:21 PDT
> @@ +670,5 @@
> > +
> > +  nsCOMPtr<nsIJSContextStack> stack =
> > +    do_GetService("@mozilla.org/js/xpc/ContextStack;1");
> > +  JSContext* cx = nsnull;
> > +  if (stack && NS_SUCCEEDED(stack->Peek(&cx)) && cx) {
> 
> Do we push the context on the stack with the new bindings?

I don't have any answer about that. Kyle?
Comment 15 Andrea Marchesini [:baku] 2012-07-26 09:44:32 PDT
Created attachment 646181 [details] [diff] [review]
Bug 775368, Porting Websockets to WebIDL - delta from the previous patch

this is just the latest changes on top of my previous patch.
Comment 16 Andrea Marchesini [:baku] 2012-07-26 09:46:19 PDT
Created attachment 646182 [details] [diff] [review]
Bug 775368, Porting Websockets to WebIDL

full patch
Comment 17 :Ms2ger (⌚ UTC+1/+2) 2012-07-26 10:21:50 PDT
Comment on attachment 646182 [details] [diff] [review]
Bug 775368, Porting Websockets to WebIDL

Review of attachment 646182 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsWebSocket.cpp
@@ +51,4 @@
>  #include "GeneratedEvents.h"
>  
>  using namespace mozilla;
> +using namespace mozilla::dom;

Just wrap the entire file in namespace{}s and remove those two 'using's.

@@ +466,5 @@
> +                         mReadyState(WebSocket::CONNECTING),
> +                         mOutgoingBufferedAmount(0),
> +                         mBinaryType(WS_BINARY_TYPE_BLOB),
> +                         mScriptLine(0),
> +                         mInnerWindowID(0)

If you're touching all this...

WebSocket::WebSocket()
  : mKeepingAlive(false)
  , mCheckMustKeepAlive(true)
...

@@ +486,4 @@
>    nsLayoutStatics::Release();
>  }
>  
> +nsISupports* WebSocket::GetParentObject()

Put this in the header

@@ +502,5 @@
> +// WebIDL
> +//---------------------------------------------------------------------------
> +
> +// Constructor:
> +already_AddRefed<WebSocket> WebSocket::Constructor(JSContext *aCx,

already_AddRefed<WebSocket> on a line of its own

@@ +513,5 @@
> +    aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
> +    return NULL;
> +  }
> +
> +  nsCOMPtr<nsIScriptObjectPrincipal> scriptPrincipal(do_QueryInterface(aGlobal));

...foo = do_Q...

@@ +629,5 @@
> +
> +  return webSocket.forget();
> +}
> +
> +nsresult WebSocket::Init(nsIPrincipal* aPrincipal,

nsresult on a line of its own

@@ +636,5 @@
> +                         const nsAString& aURL,
> +                         nsTArray<nsString>& aProtocolArray)
> +{
> +  NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread");
> +  nsresult rv;

Declare on first use

@@ +638,5 @@
> +{
> +  NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread");
> +  nsresult rv;
> +
> +  NS_ENSURE_ARG(aPrincipal);

You can assert that, right?

@@ +644,5 @@
> +  if (!PrefEnabled()) {
> +    return NS_ERROR_DOM_SECURITY_ERR;
> +  }
> +
> +  mPrincipal = aPrincipal;

Assert it is non-null

@@ +645,5 @@
> +    return NS_ERROR_DOM_SECURITY_ERR;
> +  }
> +
> +  mPrincipal = aPrincipal;
> +  if (aOwnerWindow) {

Assert this is non-null and remove the branch

@@ +668,5 @@
> +
> +  nsCOMPtr<nsIJSContextStack> stack =
> +    do_GetService("@mozilla.org/js/xpc/ContextStack;1");
> +  JSContext* cx = nsnull;
> +  if (stack && NS_SUCCEEDED(stack->Peek(&cx)) && cx) {

Pass cx from Constructor instead

@@ +684,5 @@
> +  // parses the url
> +  rv = ParseURL(PromiseFlatString(aURL));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsIScriptContext* sc = GetContextForEventHandlers(&rv);

NS_ENSURE_SUCCESS(rv, rv);

@@ +686,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsIScriptContext* sc = GetContextForEventHandlers(&rv);
> +  nsCOMPtr<nsIDocument> originDoc =
> +    nsContentUtils::GetDocumentFromScriptContext(sc);

Can't you get this from the window?

@@ +696,5 @@
> +    // Confirmed we are opening plain ws:// and want to prevent this from a
> +    // secure context (e.g. https). Check the security context of the document
> +    // associated with this script, which is the same as associated with mOwner.
> +    if (originDoc && originDoc->GetSecurityInfo())
> +      return NS_ERROR_DOM_SECURITY_ERR;

{}

@@ +708,5 @@
> +        return NS_ERROR_DOM_SYNTAX_ERR;
> +    }
> +
> +    if (!mRequestedProtocolList.IsEmpty())
> +      mRequestedProtocolList.Append(NS_LITERAL_CSTRING(", "));

{}

@@ +787,5 @@
> +
> +// webIDL: readonly attribute unsigned long bufferedAmount;
> +uint32_t WebSocket::GetBufferedAmount()
> +{
> +  return (uint32_t)mOutgoingBufferedAmount;

These can be inlined in the header

@@ +791,5 @@
> +  return (uint32_t)mOutgoingBufferedAmount;
> +}
> +
> +// webIDL: attribute Function? onopen;
> +JSObject *WebSocket::GetOnopen(JSContext *aCx)

* to the left everywhere

@@ +874,5 @@
> +    FailConnection(closeCode, closeReason);
> +    return;
> +  }
> +
> +  // mReadyState == WebSocket::OPEN

MOZ_ASSERT instead of comment

@@ +902,5 @@
> +    aBinaryType.AssignLiteral("blob");
> +    break;
> +  default:
> +    NS_ERROR("Should not happen");
> +  }

Looks like binaryType needs to be [GetterInfallible].

@@ +930,5 @@
> +  nsCString msgString = NS_ConvertUTF16toUTF8(aData);
> +  PRUint64 msgLength = msgString.Length();
> +  nsCOMPtr<nsIInputStream> msgStream;
> +
> +  Send(msgStream, msgString, msgLength, false, aRv);

s/msgStream/nullptr/

@@ +962,5 @@
> +    return;
> +  }
> +
> +  nsCString msgString;
> +  Send(msgStream, msgString, msgLength, true, aRv);

EmptyCString()?

@@ +979,5 @@
> +  char* data = reinterpret_cast<char*>(aData.mData);
> +
> +  nsCOMPtr<nsIInputStream> msgStream;
> +  nsCString msgString;
> +  msgString.Assign(data, len);

nsDependentCString, I think

@@ +984,5 @@
> +
> +  Send(msgStream, msgString, (PRInt64)len, true, aRv);
> +}
> +
> +void WebSocket::Send(nsCOMPtr<nsIInputStream>& aMsgStream,

void on the previous line, and aMsgStream should just be a nsIInputStream*

@@ +986,5 @@
> +}
> +
> +void WebSocket::Send(nsCOMPtr<nsIInputStream>& aMsgStream,
> +                       const nsACString& aMsgString,
> +                       PRUint32 aMsgLength,

PRUint32? Why do you cast to 64-bit ints in the callers, then?

@@ +990,5 @@
> +                       PRUint32 aMsgLength,
> +                       bool aIsBinary,
> +                       ErrorResult &aRv)
> +{
> +  nsresult rv;

Declare lower

::: content/base/src/nsWebSocket.h
@@ +153,5 @@
> +  void Send(const nsAString& aData,
> +            ErrorResult &aRv);
> +  void Send(nsIDOMBlob* aData,
> +            ErrorResult &aRv);
> +  void Send(::mozilla::dom::ArrayBuffer& aData,

This can now just be ArrayBuffer&, right?

@@ +287,5 @@
>    PRUint64 mInnerWindowID;
>  
>  private:
> +  WebSocket(const WebSocket& x);   // prevent bad usage
> +  WebSocket& operator=(const WebSocket& x);

MOZ_DELETE, please

::: dom/base/nsDOMClassInfo.cpp
@@ -6697,5 @@
> -  // For now don't expose web sockets unless user has explicitly enabled them
> -  if (aStruct->mDOMClassInfoID == eDOMClassInfo_WebSocket_id) {
> -    if (!nsWebSocket::PrefEnabled()) {
> -      return false;
> -    }

We'll need to decide if we want to keep this pref...
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2012-07-26 10:46:36 PDT
And 8 lines of context is nice...
Comment 19 Andrea Marchesini [:baku] 2012-07-26 11:51:53 PDT
wow Tnx for this review :)

> WebSocket::WebSocket()
>   : mKeepingAlive(false)
>   , mCheckMustKeepAlive(true)
> ...

What about:
WebSocket::WebSocket()
: mKeepingAlive(false),
  mCheckMustKeepAlive(true)
...

> > +  nsIScriptContext* sc = GetContextForEventHandlers(&rv);
> > +  nsCOMPtr<nsIDocument> originDoc =
> > +    nsContentUtils::GetDocumentFromScriptContext(sc);
> 
> Can't you get this from the window?

...  maybe.

> ::: dom/base/nsDOMClassInfo.cpp
> @@ -6697,5 @@
> > -  // For now don't expose web sockets unless user has explicitly enabled them
> > -  if (aStruct->mDOMClassInfoID == eDOMClassInfo_WebSocket_id) {
> > -    if (!nsWebSocket::PrefEnabled()) {
> > -      return false;
> > -    }
> 
> We'll need to decide if we want to keep this pref...

This check is also done in the WebSocket::Init(). I'm attaching a new path.
Thanks again for this review!
Comment 20 Andrea Marchesini [:baku] 2012-07-26 11:57:13 PDT
Created attachment 646240 [details] [diff] [review]
Bug 775368, Porting Websockets to WebIDL

I still don't know if I nsContentUtils::GetDocumentFromScriptContext(sc) is equal to window-> GetExtantDocument().
Comment 21 :Ms2ger (⌚ UTC+1/+2) 2012-07-26 12:09:34 PDT
(In reply to amarchesini from comment #19)
> wow Tnx for this review :)
> 
> > WebSocket::WebSocket()
> >   : mKeepingAlive(false)
> >   , mCheckMustKeepAlive(true)
> > ...
> 
> What about:
> WebSocket::WebSocket()
> : mKeepingAlive(false),
>   mCheckMustKeepAlive(true)
> ...

I don't like it ;)

> > ::: dom/base/nsDOMClassInfo.cpp
> > @@ -6697,5 @@
> > > -  // For now don't expose web sockets unless user has explicitly enabled them
> > > -  if (aStruct->mDOMClassInfoID == eDOMClassInfo_WebSocket_id) {
> > > -    if (!nsWebSocket::PrefEnabled()) {
> > > -      return false;
> > > -    }
> > 
> > We'll need to decide if we want to keep this pref...
> 
> This check is also done in the WebSocket::Init(). I'm attaching a new path.
> Thanks again for this review!

The question here is what |'WebSocket' in window| returns. Currently, if you toggle the pref, it will be false. That means you can detect whether WebSockets are enabled without creating a socket. It would be nice if we could keep that property.

(In reply to amarchesini from comment #20)
> Created attachment 646240 [details] [diff] [review]
> Bug 775368, Porting Websockets to WebIDL
> 
> I still don't know if I nsContentUtils::GetDocumentFromScriptContext(sc) is
> equal to window-> GetExtantDocument().

I'd like to hope so... Boris, maybe you could shed a little light on this?
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2012-07-26 12:17:06 PDT
> Boris, maybe you could shed a little light on this?

It depends on how "sc" and "window" are related.  How are they related in this case?

(Note that for windows that don't have a document _yet_ they're obviously different: the GetDocumentFromScriptContext() will create the document while GetExtantDocument() will not.  But I doubt that such windows are an issue here.)
Comment 23 :Ms2ger (⌚ UTC+1/+2) 2012-07-26 12:36:10 PDT
Comment on attachment 646240 [details] [diff] [review]
Bug 775368, Porting Websockets to WebIDL

Review of attachment 646240 [details] [diff] [review]:
-----------------------------------------------------------------

Some nits and another question for Boris:

::: content/base/src/nsWebSocket.cpp
@@ +636,5 @@
> +  }
> +
> +  mPrincipal = aPrincipal;
> +  BindToOwner(aOwnerWindow->IsOuterWindow() ?
> +              aOwnerWindow->GetCurrentInnerWindow() : aOwnerWindow);

The XHR code claims you totally have an inner window here. Please assert that and remove another branch :)

@@ +656,5 @@
> +  unsigned lineno;
> +  JSScript* script;
> +  if (JS_DescribeScriptedCaller(aCx, &script, &lineno)) {
> +      mScriptFile = JS_GetScriptFilename(aCx, script);
> +      mScriptLine = lineno;

Indentation

@@ +659,5 @@
> +      mScriptFile = JS_GetScriptFilename(aCx, script);
> +      mScriptLine = lineno;
> +  }
> +
> +  mInnerWindowID = nsJSUtils::GetCurrentlyRunningCodeInnerWindowID(aCx);

I hope this is equivalent to aOwnerWindow->WindowID(). Boris? :)
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2012-07-26 12:56:11 PDT
> I hope this is equivalent to aOwnerWindow->WindowID(). Boris? 

It sure doesn't have to be.  Consider script running in window A which has a reference to window B in "win" and does:

  var websock = new win.WebSocket();

Are you looking for the window ID of A or B?
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2012-07-26 13:13:06 PDT
> But I doubt that such windows are an issue here.

I take that back.  Who knows whether that happens.  Should test with:

  var i = document.createElement("iframe");
  document.body.appendChild(i);
  new i.contentWindow.WebSocket();
Comment 26 :Ms2ger (⌚ UTC+1/+2) 2012-07-26 14:05:52 PDT
(In reply to Boris Zbarsky (:bz) from comment #24)
> Are you looking for the window ID of A or B?

I don't have the faintest of ideas.

Leave the code alone for now, I guess.
Comment 27 Andrea Marchesini [:baku] 2012-07-26 14:10:30 PDT
MOZ_ASSERT(aOwnerWindow->IsOuterWindow()) fails in the tests.
but maybe I misunderstood what you meant.

> > +  mPrincipal = aPrincipal;
> > +  BindToOwner(aOwnerWindow->IsOuterWindow() ?
> > +              aOwnerWindow->GetCurrentInnerWindow() : aOwnerWindow);
> 
> The XHR code claims you totally have an inner window here. Please assert
> that and remove another branch :)
Comment 28 :Ms2ger (⌚ UTC+1/+2) 2012-07-26 14:12:26 PDT
(In reply to amarchesini from comment #27)
> MOZ_ASSERT(aOwnerWindow->IsOuterWindow()) fails in the tests.
> but maybe I misunderstood what you meant.
> 
> > > +  mPrincipal = aPrincipal;
> > > +  BindToOwner(aOwnerWindow->IsOuterWindow() ?
> > > +              aOwnerWindow->GetCurrentInnerWindow() : BindToOwner);
> > 
> > The XHR code claims you totally have an inner window here. Please assert
> > that and remove another branch :)

You did misunderstand me; you should assert aOwnerWindow->IsInnerWindow(), not  aOwnerWindow->IsOuterWindow(), and call BindToOwner(BindToOwner);
Comment 29 Andrea Marchesini [:baku] 2012-07-26 14:17:08 PDT
Created attachment 646333 [details] [diff] [review]
Bug 775368, Porting Websockets to WebIDL
Comment 30 Andrea Marchesini [:baku] 2012-07-26 14:18:05 PDT
Created attachment 646334 [details] [diff] [review]
Bug 775368, Porting Websockets to WebIDL - delta from the previous patch

just a short diff.
Comment 31 Boris Zbarsky [:bz] (still a bit busy) 2012-07-26 14:25:26 PDT
> I don't have the faintest of ideas.

Well, please file a bug on sorting it out?
Comment 32 :Ms2ger (⌚ UTC+1/+2) 2012-07-26 14:37:56 PDT
(In reply to :Ms2ger (Gone until 20120801) from comment #21)
> > > ::: dom/base/nsDOMClassInfo.cpp
> > > @@ -6697,5 @@
> > > > -  // For now don't expose web sockets unless user has explicitly enabled them
> > > > -  if (aStruct->mDOMClassInfoID == eDOMClassInfo_WebSocket_id) {
> > > > -    if (!nsWebSocket::PrefEnabled()) {
> > > > -      return false;
> > > > -    }
> > > 
> > > We'll need to decide if we want to keep this pref...
> > 
> > This check is also done in the WebSocket::Init(). I'm attaching a new path.
> > Thanks again for this review!
> 
> The question here is what |'WebSocket' in window| returns. Currently, if you
> toggle the pref, it will be false. That means you can detect whether
> WebSockets are enabled without creating a socket. It would be nice if we
> could keep that property.

I think we want some extended attribute on the interface for the pref, and then probably check in DefineDOMInterface (see <http://mxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py#1231>); something like

if (!Preferences::GetBool("network.websocket.enabled", true /* or false? */)) {
  *aEnabled = false;
  return false;
}

I think that would do the trick. If we don't have a test for this yet... I'd appreciate it if one was written :)

Separate patch for this change, if you don't mind.
Comment 33 :Ms2ger (⌚ UTC+1/+2) 2012-07-26 14:38:48 PDT
Comment on attachment 646333 [details] [diff] [review]
Bug 775368, Porting Websockets to WebIDL

I won't be around to look at this patch for the next week or so.
Comment 34 Andrea Marchesini [:baku] 2012-07-26 15:26:46 PDT
ok, so if we want to have a separated patch for this preference network.websocket.enabled check, I think this patch is "almost" ready.
Comment 35 Andrea Marchesini [:baku] 2012-07-27 16:01:22 PDT
Created attachment 646747 [details] [diff] [review]
Bug 775368, Porting Websockets to WebIDL

With this patch the constructor uses Sequence<DOMString>.
Comment 36 Boris Zbarsky [:bz] (still a bit busy) 2012-07-27 16:06:50 PDT
It looks like a bunch of methods that otherwise didn't change much were moved around in the file.  Was there a reason for that?  Seems like it would be better to preserve blame if possible (and incidentally make the diff simpler to review) by keeping them at the old locations....
Comment 37 Boris Zbarsky [:bz] (still a bit busy) 2012-07-27 16:22:55 PDT
Comment on attachment 646747 [details] [diff] [review]
Bug 775368, Porting Websockets to WebIDL

Document that you need the extra copy into an array to do the uniqueness checking?

>+    nsString protocolElement = aProtocols[index];

const nsString&, please.  No need for the extra copy there.

You don't use the aScriptContext argument to Init().  Just get rid of it?

Other than that, this looks fine as far as the constructor bits go.
Comment 38 Andrea Marchesini [:baku] 2012-08-05 10:46:06 PDT
Created attachment 649112 [details]
Bug 775368, Porting Websockets to WebIDL

patch updated in order to be applied on top of the central repository.
Comment 39 Andrea Marchesini [:baku] 2012-08-23 14:35:10 PDT
Created attachment 654798 [details] [diff] [review]
Bug 775368, Porting Websockets to WebIDL

rebased
Comment 40 Andrea Marchesini [:baku] 2012-08-28 02:26:12 PDT
Created attachment 655924 [details] [diff] [review]
patch 2

rebased on 785319
Comment 41 Andrea Marchesini [:baku] 2012-09-05 06:27:29 PDT
Created attachment 658464 [details] [diff] [review]
patch 3

Preferences checked
Comment 42 Boris Zbarsky [:bz] (still a bit busy) 2012-09-06 12:47:43 PDT
Comment on attachment 658464 [details] [diff] [review]
patch 3

So first of all, this is awesome stuff!

>+++ b/content/base/src/WebSocket.cpp

For future reference, breaking this up into two patches, where the first one does the rename and the second one makes the substantive changes, would have made the review much easier.

I still wish the code had not moved in the file so that more of the blame would be preserved.  And again incidentally so it would be simpler to review.  :(  Why _did_ the code move?

>+WebSocket::Constructor(JSContext* aCx,
...
>+    return NULL;

Maybe "nullptr"?  And same elsewhere in this constructor.

>+WebSocket::Init(JSContext* aCx,
>+  // Get WindowID

Move this comment down to where you actually get it?

>+WebSocket::GetListenerAsJSObject(nsDOMEventListenerWrapper* aWrapper)

It looks like you're effectively backing out the websocket parts of bug 687332.  Please don't do that.

If you merge to that change, then you can just use something like the IMPL_EVENT_HANDLER macro in content/base/src/nsXMLHttpRequest.h to implement this stuff.

So for now I'm skipping the GetListenerAsJSObject and SetJSObjectListener bits and all the Get/SetOn* bits.

>+// webIDL: void close(optional unsigned short code, optional DOMString reason):
>+WebSocket::Send(const nsAString& aData,
>+  if (mReadyState == WebSocket::CONNECTING) {

Can we push this check down into the shared Send() method instead of doing it in several different places?

>+  nsCString msgString = NS_ConvertUTF16toUTF8(aData);

  NS_ConvertUTF1toUTF8 msgString(aData);

>+WebSocket::Send(ArrayBuffer& aData,
>+  PRInt32 len = aData.Length() * sizeof(*aData.Data());

ArrayBuffer.Data() returns a uint8_t*. So the sizeof will be 1, no?  I suppose you could MOZ_ASSERT that if desired, but no need to do the multiplication thing.

>+  nsCOMPtr<nsIInputStream> msgStream;

Why do you need that?  Just pass nullptr to the shared Send() explicitly.

>+  nsDependentCString msgString;
>+  msgString.Assign(data, len);

This part doesn't make sense to me.  If you're trying to make a copy, then why use an nsDependentCString.  If you're not trying to make a copy, why is this code copying?

What you probably want here is something more like this:

  nsDependentCSubstring msgString(data, len);

>+++ b/content/base/src/WebSocket.h
>+  nsISupports* GetParentObject() { return GetOwner(); }

Why not return an nsPIDOMWindow* here?

>+++ b/dom/bindings/Bindings.conf
>+    'nativeType': 'mozilla::dom::WebSocket',
>+    'headerFile': 'WebSocket.h',

Given the rename of the class, you shouldn't need the nativeType.  The bindings assume that the type is mozilla::dom::interfacename.

You do need 'headerFile' unless you export the header to mozilla/dom, in which case you won't need it.

>+    'prefable': True

This shouldn't be here.  The reason is that this patch rips out the non-WebIDL implementation of websocket, so if you leave it prefable and then the binding pref is flipped that will completely break websocket in the process....

If you do want this to be prefable, you'll need to leave the XPCOM implementation in for now.  I don't think it's worth it, honestly.

>+++ b/dom/webidl/WebIDL.mk
>   XMLHttpRequest.webidl \
>   XMLHttpRequestEventTarget.webidl \
>   XMLHttpRequestUpload.webidl \
>+  WebSocket.webidl \

This should come before the XHR entries in the alphabetical list here.

>+++ b/dom/webidl/WebSocket.webidl

>+[PrefControlled,
>+  [Infallible]

The way fallibility is annotated got changed recently.  You no longer need the [Infallible] annotations.  You now need [Throws] on the things that can throw.

>+  [TreatNonCallableAsNull, GetterInfallible]
>+  attribute Function? onopen;

So that would need a [TreatNonCallableAsNull, SetterThrows] instead.

>+  void close(optional unsigned short code, optional DOMString reason);

This should have [Clamp] on the first argument.  I believe [Clamp] support landed this morning.  ;)

>+  [Infallible]
>+  attribute DOMString binaryType;

This should be an enumerated type, as in the spec.  Why isn't it?

>+  void send(DOMString data);
>+  void send(Blob data);
>+  void send(ArrayBuffer data);

Spec also has:

  void send(ArrayBufferView data);

r- pending the fixes to the on* stuf above.
Comment 43 :Ms2ger (⌚ UTC+1/+2) 2012-09-06 12:56:38 PDT
(In reply to Boris Zbarsky (:bz) from comment #42)
> >+  void close(optional unsigned short code, optional DOMString reason);
> 
> This should have [Clamp] on the first argument.  I believe [Clamp] support
> landed this morning.  ;)

It did!

> >+  [Infallible]
> >+  attribute DOMString binaryType;
> 
> This should be an enumerated type, as in the spec.  Why isn't it?

Because it wasn't in the spec until after Andrea started on this patch. But yes, that would be a righteous change.
Comment 44 Andrea Marchesini [:baku] 2012-09-09 04:23:55 PDT
Created attachment 659561 [details] [diff] [review]
patch 4

I changed the order of the methods. Maybe this is easier to review.
Comment 45 Boris Zbarsky [:bz] (still a bit busy) 2012-09-09 18:50:46 PDT
Comment on attachment 659561 [details] [diff] [review]
patch 4

I just noticed that this is still using the PR* integer types.  I believe we've switched to stdint types at this point.  There's a script at https://bugzilla.mozilla.org/attachment.cgi?id=650572 that you can run on your patch queue to auto-convert stuff for you before you check in.

In the ArrayBuffer and ArrayBufferView Send() methods, why is len a signed int that you then cast?  aData.Length() is uint32_t already...

Your GetParentObject is still returning nsISupports, not nsPIDOMWindow.  If there's a good reason for that, please document what that is.  If not, return nsPIDOMWindow?

In the Bindings.conf, you shouldn't need a nativeType line.  Why is that still there?

>+  [Clamp, Throws]
>+  void close(optional unsigned short code, optional DOMString reason);

That should be:

  [Throws]
  void close([Clamp] optional unsigned short code, optional DOMString reason);

>+  attribute DOMString binaryType;

That still needs to be an enumerated type.  A followup bug is OK for this, especially if there is a good reason it's not at the moment, but then please add a comment here pointing to that bug.

r=me with the above issues fixed.  Thank you again for doing this!
Comment 46 Andrea Marchesini [:baku] 2012-09-10 00:57:17 PDT
Created attachment 659642 [details] [diff] [review]
patch 5

Patch is ready. When I see the try server green, I ask to have it checked in.
Comment 47 Andrea Marchesini [:baku] 2012-09-10 01:51:21 PDT
Comment on attachment 659642 [details] [diff] [review]
patch 5

https://tbpl.mozilla.org/?tree=Try&rev=d2e5c9fffa76 green on try
Comment 48 Boris Zbarsky [:bz] (still a bit busy) 2012-09-10 05:40:48 PDT
You shouldn't need the switch in SetBinaryType anymore, right?
Comment 49 Andrea Marchesini [:baku] 2012-09-10 06:44:30 PDT
Created attachment 659693 [details] [diff] [review]
patch 6
Comment 50 Masatoshi Kimura [:emk] 2012-09-10 06:54:32 PDT
> Flags: checkin+
"checkin?" if you don't land the patch yourself. The commiter will update the flag to "checkin+".
Comment 51 Boris Zbarsky [:bz] (still a bit busy) 2012-09-10 07:22:49 PDT
Thanks for the quick update!

https://hg.mozilla.org/integration/mozilla-inbound/rev/b913d21dd208
Comment 52 Ryan VanderMeulen [:RyanVM] 2012-09-10 18:46:39 PDT
https://hg.mozilla.org/mozilla-central/rev/b913d21dd208

Note You need to log in before you can comment on or make changes to this bug.