Closed Bug 775368 Opened 12 years ago Closed 12 years ago

Porting Websockets to WebIDL

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: baku, Assigned: baku)

References

(Blocks 2 open bugs)

Details

(Keywords: addon-compat)

Attachments

(1 file, 15 obsolete files)

91.87 KB, patch
baku
: review+
baku
: checkin+
Details | Diff | Splinter Review
Porting Websockets to WebIDL
Assignee: nobody → amarchesini
Attached patch websocket + webIDL - first patch (obsolete) — Splinter Review
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?
Keywords: addon-compat
I don't see any hits on that contract in the addons mxr, for what it's worth.
Yes, it doesn't look like anyone is using it.
The constructor will be changed once the bug 775844 is closed.
Attachment #645486 - Flags: review?(khuey)
777066 has been created just to remind me that I have to use sequences in the constructor.
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?
Attachment #645486 - Flags: review?(khuey) → review-
. It's green on tryserver
. binaryType is fallible because the SetBinaryType can fail
Thanks for your review!
Attachment #644504 - Attachment is obsolete: true
Attachment #645486 - Attachment is obsolete: true
Attachment #645994 - Flags: review?(khuey)
(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 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.
Do we want to remove the old interface? How should binary stuff use websocket without it?
(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!
> @@ +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?
this is just the latest changes on top of my previous patch.
Attachment #646181 - Flags: review?(khuey)
Attachment #646181 - Flags: review?(Ms2ger)
full patch
Attachment #645994 - Attachment is obsolete: true
Attachment #645994 - Flags: review?(khuey)
Attachment #646182 - Flags: review?(khuey)
Attachment #646182 - Flags: review?(Ms2ger)
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...
And 8 lines of context is nice...
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!
I still don't know if I nsContentUtils::GetDocumentFromScriptContext(sc) is equal to window-> GetExtantDocument().
Attachment #646182 - Attachment is obsolete: true
Attachment #646182 - Flags: review?(khuey)
Attachment #646182 - Flags: review?(Ms2ger)
Attachment #646240 - Flags: review?(khuey)
Attachment #646240 - Flags: review?(Ms2ger)
(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?
> 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 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? :)
Attachment #646240 - Flags: review?(Ms2ger)
> 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?
> 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();
(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.
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 :)
(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);
Attachment #646240 - Attachment is obsolete: true
Attachment #646240 - Flags: review?(khuey)
Attachment #646333 - Flags: review?(Ms2ger)
just a short diff.
Attachment #646181 - Attachment is obsolete: true
Attachment #646181 - Flags: review?(khuey)
Attachment #646181 - Flags: review?(Ms2ger)
> I don't have the faintest of ideas.

Well, please file a bug on sorting it out?
(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 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.
Attachment #646333 - Flags: review?(Ms2ger)
ok, so if we want to have a separated patch for this preference network.websocket.enabled check, I think this patch is "almost" ready.
Blocks: 504553
Depends on: 778044
With this patch the constructor uses Sequence<DOMString>.
Attachment #646333 - Attachment is obsolete: true
Attachment #646334 - Attachment is obsolete: true
Attachment #646747 - Flags: review?(bzbarsky)
Status: NEW → UNCONFIRMED
Depends on: 777066
Ever confirmed: false
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....
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Attachment #646747 - Flags: review?(bzbarsky) → review+
patch updated in order to be applied on top of the central repository.
Attachment #646747 - Attachment is obsolete: true
rebased
Attachment #649112 - Attachment is obsolete: true
Depends on: 785319
Attached patch patch 2 (obsolete) — Splinter Review
rebased on 785319
Attachment #654798 - Attachment is obsolete: true
Attached patch patch 3 (obsolete) — Splinter Review
Preferences checked
Attachment #655924 - Attachment is obsolete: true
Attachment #658464 - Flags: review?(bzbarsky)
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.
Attachment #658464 - Flags: review?(bzbarsky) → review-
(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.
Attached patch patch 4 (obsolete) — Splinter Review
I changed the order of the methods. Maybe this is easier to review.
Attachment #658464 - Attachment is obsolete: true
Attachment #659561 - Flags: review?(bzbarsky)
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!
Attachment #659561 - Flags: review?(bzbarsky) → review+
Attached patch patch 5 (obsolete) — Splinter Review
Patch is ready. When I see the try server green, I ask to have it checked in.
Attachment #659561 - Attachment is obsolete: true
Keywords: checkin-needed
You shouldn't need the switch in SetBinaryType anymore, right?
Attached patch patch 6Splinter Review
Attachment #659642 - Attachment is obsolete: true
Attachment #659693 - Flags: review+
Attachment #659693 - Flags: checkin+
> Flags: checkin+
"checkin?" if you don't land the patch yourself. The commiter will update the flag to "checkin+".
Thanks for the quick update!

https://hg.mozilla.org/integration/mozilla-inbound/rev/b913d21dd208
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/b913d21dd208
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.