Patrick McManus [:mcmanus] Reporter
	Description • 13 years ago

Clearly - that means "void send(in ArrayBuffer data);"

and delivering on onMessage as "then set event's data attribute to a new read-only ArrayBuffer object whose contents are data. [TYPEDARRAY]"

and reading the binaryType attribute as "arraybuffer".

I'm not sure what the behavior should be if the JS sets the binaryType attribute to be "blob". SYNTAX_ERROR?  I don't know what crhrome does. Jonas?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 1 • 13 years ago

Ouch! The spec shouldn't call for a readonly buffer. That just removes the ability to use that buffer directly for data manipulation and forces the user to make a copy if that is needed. It also doesn't match what FileReader or XMLHttpRequest does.

And we don't have readonly buffers implemented. I'll file a bug on the spec.

	Jason Duell Assignee
	Comment 2 • 13 years ago

Comments starting with ???? are for review feedback only (I'll delete before landing).

	Jason Duell Assignee
	Comment 4 • 13 years ago

Test code:  I'm assigning to Patrick out of some vague sense that he's more familiar with the WS spec at this point, and also might be less busy (? hmm probably not...) than smaug.  Feel free to reassign as y'all see fit.

The tests in the 'websocket_hybi" directory are mochi-fied versions of some chromium tests from their 'hybi' test suite.  I've stuck them in a separate directory both because content/base/test is getting full, and because Gerv suggested we put them in a separate directory to handle any licensing issues. There are actually a lot more of these tests we can add, though they sometimes require non-trivial tweaking to make mochi-friendly.

	Jason Duell Assignee
	Comment 5 • 13 years ago

Found small bug in v1: OutboundMessage(stream) need to be properly deleted if never sent.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 13 years ago

Comment on attachment 576112 [details] [diff] [review]
v1: Websocket Binary Message support: DOM changes

>+  void send(in nsIVariant data);
Could be perhaps
[implicit_context] send(in jsval data);
But using nsIVariant for now is ok.


> nsWebSocket::OnMessageAvailable(nsISupports *aContext, const nsACString & aMsg)
> {
>   NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread");
>-
>   NS_ABORT_IF_FALSE(!mDisconnected, "Received message after disconnecting");
> 
>   // Dispatch New Message
>-  nsresult rv = CreateAndDispatchMessageEvent(aMsg);
>-  if (NS_FAILED(rv)) {
>+  nsresult rv = CreateAndDispatchMessageEvent(aMsg, false);
>+  if (NS_FAILED(rv))
>     NS_WARNING("Failed to dispatch the message event");
>-  }
>+
Coding style is
if (expr) {
  stmt;
}
So don't remove {}
Use the same coding style also elsewhere.


>+nsWebSocket::CreateAndDispatchMessageEvent(const nsACString& aData,
>+                                           bool isBinary)
> {
>   NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread");
>   nsresult rv;
> 
>   rv = CheckInnerWindowCorrectness();
>-  if (NS_FAILED(rv)) {
>+  if (NS_FAILED(rv))
>     return NS_OK;
>-  }
ditto. happens also elsewhere in the patch


>+// ???? This is total voodoo to me: copied from XHR
>+void
>+nsWebSocket::RootResultArrayBuffer()
>+{
>+  nsContentUtils::PreserveWrapper(
>+    static_cast<nsIDOMEventTarget*>(
>+      static_cast<nsDOMEventTargetHelper*>(this)), this);
>+}
>+
There is no need for this magic here. It was added for XHR in Bug 658683, where
the array should be kept alive for a long time.
With websocket it is the message event which keeps the data alive.

>+// ????: lifted from nsXMLHttpRequest::CreateResponseArrayBuffer.  Centralize?
Yes please. Move to nsContentUtils, and add a parameter to whether or not call PreserveWrapper.


>+// TODO: Initial implementation: only stores to RAM, not file, so won't behave
>+// well with large files.
Need to file a follow up to fix that.


>+    if (NS_SUCCEEDED(rv) && !JSVAL_IS_PRIMITIVE(realVal) &&
>+        (obj = JSVAL_TO_OBJECT(realVal)) &&
>+        (js_IsArrayBuffer(obj)))
>+    {
{ should be in the previous line


>+// ???? Any reason we can't just use NS_ConvertUTF16toUTF8?
>+//
Yes. Websocket defines error handling for invalid utf.
Or at least some version of the protocol did.


>+  enum {
{ should be on its own line

	Patrick McManus [:mcmanus] Reporter
	Comment 7 • 13 years ago

Comment on attachment 576116 [details] [diff] [review]
v1.1: Websocket Binary Message support: necko changes

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

nsnetmodule.cpp appears to be whitespace changes unrelated to binary messaging. please save them for another patch.

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +326,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    mMsg.pStream->Close();
> +    mMsg.pStream->Release();
> +    mMsg.pString = new nsCString(temp);

is this going to copy all the bytes of temp or is it just going to add a reference to them? I always need to look that up. Obviously a copy is to be avoided here (as this is probably large).

@@ +503,5 @@
>    {
>      PR_ATOMIC_DECREMENT(&mConnectedCount);
>    }
>  
> +  PRUint32 ConnectedCount()

why this change? PR_ATOMIC is defined on a signed int. I'm pretty sure 31 bits is enough :)

@@ -1088,5 @@
>  
>  void
>  WebSocketChannel::GeneratePing()
>  {
> -  LOG(("WebSocketChannel::GeneratePing() %p\n", this));

why?

@@ -1100,5 @@
>  void
>  WebSocketChannel::GeneratePong(PRUint8 *payload, PRUint32 len)
>  {
> -  LOG(("WebSocketChannel::GeneratePong() %p [%p %u]\n", this, payload, len));
> -

why?

::: netwerk/protocol/websocket/WebSocketChannel.h
@@ +195,5 @@
>    PRUint32                        mPingTimeout;  /* milliseconds */
>    PRUint32                        mPingResponseTimeout;  /* milliseconds */
>  
>    nsCOMPtr<nsITimer>              mLingeringCloseTimer;
> +  const static PRUint32           kLingeringCloseTimeout   = 1000;

is there a reason for changing this in the binary message patch?

@@ +196,5 @@
>    PRUint32                        mPingResponseTimeout;  /* milliseconds */
>  
>    nsCOMPtr<nsITimer>              mLingeringCloseTimer;
> +  const static PRUint32           kLingeringCloseTimeout   = 1000;
> +  const static PRUint32           kLingeringCloseThreshold = 50;

is there a reason for changing this in the binary message patch?

::: netwerk/protocol/websocket/nsIWebSocketChannel.idl
@@ +45,5 @@
>  
>  #include "nsISupports.idl"
>  
> +/** This interface is deprecated and will be removed.  Don't use it.
> + *  You probably want nsI{Moz}WebSocket.idl

I disagree with this statement - the channel is potentially a useful interface. I don't mind the pointer to nsIWebSocket.idl

	Patrick McManus [:mcmanus] Reporter
	Comment 8 • 13 years ago

Comment on attachment 576114 [details] [diff] [review]
v1: Websocket Binary Message support: Mochitests

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

r=mcmanus with necko change removed from this patch

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +2200,5 @@
>  // nsIRequestObserver (from nsIStreamListener)
>  
>  NS_IMETHODIMP
>  WebSocketChannel::OnStartRequest(nsIRequest *aRequest,
> +                                 nsISupports *aContext)

I'm pretty sure you didn't mean for this diff to be in the test patch.

	Jason Duell Assignee
	Comment 9 • 13 years ago

Smaug:  here's a V1->V2 interdiff in case you find that easier to review than going over the whole patch again.

	Jason Duell Assignee
	Comment 10 • 13 years ago

All changes from 1st review made (I found it more natural to have the XHR keep its own RootResultArray function instead of doing that logic within CreateArrayBuffer: I can change it if you like).

	Jason Duell Assignee
	Comment 11 • 13 years ago

Patrick:  here's a V1->V2 interdiff in case you find that easier to review than going over the whole patch again.

	Jason Duell Assignee
	Comment 12 • 13 years ago

> > +    mMsg.pString = new nsCString(temp);
>
> is this going to copy all the bytes of temp or is it just going to add a
reference?

Just adds a reference.  But to make it clearer I've changed it to use a single CString with an autoptr (in case we hit failure and need to delete it).

> > +  PRUint32 ConnectedCount()
> > +  const static PRUint32           kLingeringCloseTimeout   = 1000;
> > +  const static PRUint32           kLingeringCloseThreshold = 50;
>
> why this change? PR_ATOMIC is defined on a signed int. I'm pretty sure 31 bits is enough :)

I was just thinking values that are never negative should be unsigned--wasn't aware of the PR_ATOMIC issue.  I've reverted back to PRInt32 (but also changed mMaxConnections to PRInt32 too, so we avoid compiler warnings about comparing signed/unsigned).

> > -  LOG(("WebSocketChannel::GeneratePing() %p\n", this));
> 
> why?

I centralized the logging into EnqueueOutgoingMessage, which GeneratePing/Pong call, so I figured we didn't need what's essentially a dupe log msg.  You can still grep for 'ping' in the log and see when these were enqueued.  If you want these LOG msgs back we can add them (but we already do a lot of logging IMO).


> +/** This interface is deprecated and will be removed.  Don't use it.
> + *  You probably want nsI{Moz}WebSocket.idl
> 
> I disagree with this statement - the channel is potentially a useful interface. I don't mind the pointer to nsIWebSocket.idl

bz was of the opinion we should probably scrap the whole IDL if we're not going to use it from JS.  But let's punt on that discussion for now--I've removed the 'deprecated' part of the comment.   (I've kept the removal of 'scriptable' from the interface, so for now it's not accessible from JS.  If you think we want to keep it accessible from JS, let's chat about it with bz. I hate making stuff visible to addons that we may change/remove--and we'll definitely be changing this for large file support.  If you want JS access, perhaps we can add it after the inteface settles?)

	Jason Duell Assignee
	Comment 13 • 13 years ago

Removed unrelated necko change, carrying forward patrick's +r

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 14 • 13 years ago

Comment on attachment 577512 [details] [diff] [review]
v2: Websocket Binary Message support: DOM changes


>@@ -69,30 +70,35 @@ interface nsIMozWebSocket : nsISupports
Update uuid

>+#define TRUE_OR_FAIL_CXN(x, ret)                                          \
What does CXN mean? I don't see reason for these macro changes.

	Jason Duell Assignee
	Comment 15 • 13 years ago

> Update uuid

will do.

> What does CXN mean? I don't see reason for these macro changes.

"Connection".  I found ENSURE_TRUE_AND_FAIL_IF_FAILED to be overly long and a little vague (what's being failed?).  I'm open to suggestions for a better name (TRUE_OR_FAIL_WEBSOCKET is the first thing that comes to mind: I can add ENSURE_ if that seems needed).

	Jason Duell Assignee
	Comment 16 • 13 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/557f2ae6e8aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b97cde37493
https://hg.mozilla.org/integration/mozilla-inbound/rev/55f353e4d6f4

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 17 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/55f353e4d6f4
https://hg.mozilla.org/mozilla-central/rev/0b97cde37493
https://hg.mozilla.org/mozilla-central/rev/557f2ae6e8aa