This is the DOM API and JS objects exposed via the PeerConnection for WebRTC dataChannels, per the evolving (mostly agreed-to) spec in the WebRTC W3C list.

An example of the API needed is in https://docs.google.com/document/d/16csYCaHxIYP83DzCZJL7relQm2QNxT-qkay4-jLxoKA/edit?ndplr=1 though please ignore the discussion of SDP negotiation; the current proposal by me and others is to have a small wire protocol over SCTP to implement the bidirectional opens and closes. An IETF draft on this will be submitted today.

Another proposal in the tread that starts at http://lists.w3.org/Archives/Public/public-webrtc/2012Feb/0183.html Much of the discussion hinges on the handling of "glare" and crossing open requestst. This is largely the API I expect we'll reach consensus on.

Ok, we have an updated draft (though still not at consensus, this should be close): http://dev.w3.org/2011/webrtc/editor/webrtc.html#peer-to-peer-data-api

Note: started from WebSockets and did a bunch of query-replaces; the IDL files are at least somewhere in the outfield of the ballpark, the cpp files are still looking for the exit off the freeway

This builds, no idea yet if it runs

updated a few bits for DTLS

update port number we use for Init()

Also cleans up a couple of warnings

Comment on attachment 652687 [details] [diff] [review] Add support for DTLS connections to DataChannels This already is on alder and works, but I thought you should look over the SCTP<->transport parts at least

Comment on attachment 652890 [details] [diff] [review] Implement proper SCTP shutdown on Mozilla exit This is really part of bug 729511, not DOM

Pushed patch from bug 729512 for Label support and also added DataChannelInit dictionary objects, so you can ask for unreliable channels. Updated the chat app to used labels and an unreliable channel. Bug: you have to specify the channel type as an integer (values are shown as constants in PeerConnection); I need to talk to an IDL/DOM person. This fills in the last major missing parts of the protocol and DOM API

Builds on m-c on top of the latest bug 729511 and bug 729512 patches (plus a disable-dtls patch I need to upload)

Comment on attachment 659983 [details] [diff] [review] DataChannel DOM part 1 rollup DOM for Datachannels (not including how it's accessed via PeerConnection) up for review. Strongly based on nsWebSockets.cpp. Ted: Trivial makesystem changes

Comment on attachment 659983 [details] [diff] [review] DataChannel DOM part 1 rollup probly :smaug or :bz for dom reviewing dom code.

Comment on attachment 659983 [details] [diff] [review] DataChannel DOM part 1 rollup Sorry, too late at night when I marked it for review. I meant to hit the DOM guys.

Comment on attachment 659983 [details] [diff] [review] DataChannel DOM part 1 rollup >diff --git a/content/base/public/nsIDOMDataChannel.idl b/content/base/public/nsIDOMDataChannel.idl >new file mode 100644 >--- /dev/null >+++ b/content/base/public/nsIDOMDataChannel.idl >@@ -0,0 +1,39 @@ >+#include "domstubs.idl" >+ >+interface nsIDOMEventListener; Do you need this? >+interface nsIVariant; >+ >+[scriptable, builtinclass, uuid(fb7a8ec4-c1eb-4d9f-b927-fbb8b4493e6d)] >+interface nsIDOMDataChannel : nsISupports >+{ >+ readonly attribute DOMString label; >+ readonly attribute boolean reliable; >+ readonly attribute boolean ordered; >+ >+ 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; >+ >+ [implicit_jscontext] attribute jsval onopen; >+ [implicit_jscontext] attribute jsval onerror; >+ [implicit_jscontext] attribute jsval onclose; >+ [implicit_jscontext] attribute jsval onmessage; So, nsIDOMDataChannel is apparently event target. Then it should inherit nsIDOMEventTarget. >+ >+ class nsDOMDataChannel : public nsDOMEventTargetHelper, >+ public nsIDOMDataChannel, >+ public mozilla::DataChannelListener Indentation is odd here. Looks like you have \t before 'class' >+ // Get msg info out of JS variable being sent (string, arraybuffer, blob) >+ nsresult GetSendParams(nsIVariant *aData, nsCString &aStringOut, >+ nsCOMPtr<nsIInputStream> &aStreamOut, >+ bool &aIsBinary, uint32_t &aOutgoingLength, >+ JSContext *aCx); >+ >+ nsresult CreateResponseBlob(const nsACString& aData, JSContext *aCx, >+ jsval &jsData); Nit, please be consistent where * and & go. (They should be with the type.) >+ >+ // Owning reference >+ nsAutoPtr<mozilla::DataChannel> mDataChannel; >+ nsString mUTF16Origin; I guess this could be just mOrigin >+ >+ // XXX any need to CheckInnerWindowCorrectness() like WebSockets? >+ // It's only an issue if the PeerConnection can likewise leak, which I think it can't. >+ // See bug 696085 Yeas, please add a check for innerwindowcorrectness >+ // Do we need to observe for window destroyed or frozen? (same bug) Well, what should happen when the window/document goes to bfcache? And what should happen when the page goes out from bfcache? I think it might be ok to disable bfcache if we have webrtc active. Add something to nsDocument::CanSavePresentation >+NS_IMETHODIMP >+nsDOMDataChannel::Send(nsIVariant *aData, JSContext *aCx) nsIVariant* >+nsresult >+nsDOMDataChannel::DoOnMessageAvailable(const nsACString& aData, >+ bool isBinary) aIsBinary >+ if (isBinary) { >+ if (mBinaryType == DC_BINARY_TYPE_BLOB) { >+ rv = CreateResponseBlob(aData, cx, jsData); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } else if (mBinaryType == DC_BINARY_TYPE_ARRAYBUFFER) { >+ JSObject *arrayBuf; JSObject* >+nsresult >+nsDOMDataChannel::OnChannelConnected(nsISupports* aContext) >+{ >+ MOZ_ASSERT(NS_IsMainThread()); >+ >+ nsCOMPtr<nsIDOMEvent> event; >+ nsresult rv = NS_NewDOMEvent(getter_AddRefs(event), nullptr, nullptr); >+ NS_ENSURE_SUCCESS(rv,rv); >+ >+ rv = event->InitEvent(NS_LITERAL_STRING("open"), false, false); >+ NS_ENSURE_SUCCESS(rv,rv); >+ >+ event->SetTrusted(true); >+ >+ LOG(("%p(%p): %s - Dispatching\n",this,(void*)mDataChannel,__FUNCTION__)); >+ >+ return DispatchDOMEvent(nullptr, event, nullptr, nullptr); >+} >+ >+nsresult >+nsDOMDataChannel::OnChannelClosed(nsISupports* aContext) >+{ >+ MOZ_ASSERT(NS_IsMainThread()); >+ >+ nsCOMPtr<nsIDOMEvent> event; >+ nsresult rv = NS_NewDOMEvent(getter_AddRefs(event), nullptr, nullptr); >+ NS_ENSURE_SUCCESS(rv,rv); >+ >+ rv = event->InitEvent(NS_LITERAL_STRING("close"), false, false); >+ NS_ENSURE_SUCCESS(rv,rv); >+ >+ event->SetTrusted(true); >+ >+ LOG(("%p(%p): %s - Dispatching\n",this,(void*)mDataChannel,__FUNCTION__)); >+ >+ return DispatchDOMEvent(nullptr, event, nullptr, nullptr); >+} You could dispatch events the following way: return GetOwner() ? nsContentUtils::DispatchTrustedEvent(GetOwner()->GetExtantDoc(), this, NS_LITERAL_STRING(< you event name >), false, false) : NS_ERROR_FAILURE; Though, it is not clear to me whether we want to propagate the error. Perhaps NS_OK would be ok too. >+NS_NewDOMDataChannel(mozilla::DataChannel* dataChannel, >+ nsPIDOMWindow* aWindow, >+ nsIDOMDataChannel** domDataChannel) Parameters should be in form aParameterName The interface will be converted to use webidl, but that can be done later in a followup. (I'll need to get some more experience with webidl, so that I could start reviewing such patches.)

(In reply to Olli Pettay [:smaug] from comment #37) > >+ [implicit_jscontext] attribute jsval onopen; > >+ [implicit_jscontext] attribute jsval onerror; > >+ [implicit_jscontext] attribute jsval onclose; > >+ [implicit_jscontext] attribute jsval onmessage; > So, nsIDOMDataChannel is apparently event target. Then it should inherit > nsIDOMEventTarget. Yup, done. > >+ > >+ class nsDOMDataChannel : public nsDOMEventTargetHelper, > >+ public nsIDOMDataChannel, > >+ public mozilla::DataChannelListener > Indentation is odd here. Looks like you have \t before 'class' Oops, typo. > > >+ // Get msg info out of JS variable being sent (string, arraybuffer, blob) > >+ nsresult GetSendParams(nsIVariant *aData, nsCString &aStringOut, > >+ nsCOMPtr<nsIInputStream> &aStreamOut, > >+ bool &aIsBinary, uint32_t &aOutgoingLength, > >+ JSContext *aCx); > >+ > >+ nsresult CreateResponseBlob(const nsACString& aData, JSContext *aCx, > >+ jsval &jsData); > Nit, please be consistent where * and & go. > (They should be with the type.) Yeah, my fingers have 27 years of C/C++ doing it the other way... But in any case the style needs to match the rest of the code it's with. (Ditto for the other similar instances) We're nowhere near consistent in the tree, but as this is new code (even if much of it is copied from nsWebSocket), I'll normalize. > >+ > >+ // Owning reference > >+ nsAutoPtr<mozilla::DataChannel> mDataChannel; > >+ nsString mUTF16Origin; > I guess this could be just mOrigin Done. > >+ > >+ // XXX any need to CheckInnerWindowCorrectness() like WebSockets? > >+ // It's only an issue if the PeerConnection can likewise leak, which I think it can't. > >+ // See bug 696085 > Yeas, please add a check for innerwindowcorrectness Well, reading that bug, I think there are differences between WebSockets and DataChannels here that indicate that the problem in bug 696085 can't happen in the same way (DataChannels are attached to PeerConnections via DataChannelConnection objects). However, *maybe* the PeerConnection this is hung off of (which is new'd in JS) might be able to leak in a similar way (probably tougher, perhaps impossible, but better to be safe). > >+ // Do we need to observe for window destroyed or frozen? (same bug) > Well, what should happen when the window/document goes to bfcache? > And what should happen when the page goes out from bfcache? > I think it might be ok to disable bfcache if we have webrtc active. > Add something to nsDocument::CanSavePresentation A very good point; PeerConnections should die on navigation (and that will ripple down to the DataChannels attached to it). Perhaps they should add themselves as unload or beforeunload listeners to the document, which will let them kill themselves as well as block CanSavePresentation. Per Olli, it would need to a system event handler, so it can't be blocked (e.g. AddSystemEventListener()) (This patch is being reviewed in a slight vacuum, since PeerConnection isn't ready - I plan to lash it into the DOM in a temporary place until PeerConnection lands). If we ever decide to use DataChannels without PeerConnection (which I doubt due to all the NAT traversal and negotiation issues), we'd need to deal with this directly. Revised comment: // See bug 696085 // We don't need to observe for window destroyed or frozen; but PeerConnection needs // to not allow itself to be bfcached (and get destroyed on navigation). > You could dispatch events the following way: > return GetOwner() ? > nsContentUtils::DispatchTrustedEvent(GetOwner()->GetExtantDoc(), this, > NS_LITERAL_STRING(< you event name >), false, false) : NS_ERROR_FAILURE; > Though, it is not clear to me whether we want to propagate the error. > Perhaps NS_OK would be ok too. Tried this; I ended up going back to DispatchDOMEvent() (though cleaned up some) because the target isn't 'this', and I wasn't sure what to use (nullptr works, but complains and some stuff doesn't seem to work entirely). > >+NS_NewDOMDataChannel(mozilla::DataChannel* dataChannel, > >+ nsPIDOMWindow* aWindow, > >+ nsIDOMDataChannel** domDataChannel) > Parameters should be in form aParameterName Done > The interface will be converted to use webidl, but that can be done later in > a followup. > (I'll need to get some more experience with webidl, so that I could start > reviewing such patches.) I wrote a WebIDL version at one point, though it's not in the patch.

Comment on attachment 661000 [details] [diff] [review] DataChannel DOM implementation Part 1 Review of attachment 661000 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/public/nsIDOMDataChannel.idl @@ +15,5 @@ > + const unsigned short OPEN = 1; > + const unsigned short CLOSING = 2; > + const unsigned short CLOSED = 3; > + > + readonly attribute unsigned short readyState; readyState should be a DOMString ::: content/base/src/Makefile.in @@ +145,5 @@ > +LOCAL_INCLUDES += \ > + -I$(topsrcdir)/netwerk/sctp/datachannel \ > + -I$(topsrcdir)/media/webrtc/trunk/third_party/libjingle/source \ > + $(NULL) > +endif ifdef MOZ_WEBRTC EXPORTS += \ nsDOMDataChannel.h \ $(NULL) CPPSRCS += \ nsDOMDataChannel.cpp \ $(NULL) LOCAL_INCLUDES += \ -I$(topsrcdir)/netwerk/sctp/datachannel \ -I$(topsrcdir)/media/webrtc/trunk/third_party/libjingle/source \ $(NULL) endif (Definitely no tabs.) ::: content/base/src/nsDOMDataChannel.cpp @@ +44,5 @@ > + public mozilla::DataChannelListener > +{ > +public: > + nsDOMDataChannel(mozilla::DataChannel* aDataChannel) : mDataChannel(aDataChannel), > + mBinaryType(DC_BINARY_TYPE_BLOB) nsDOMDataChannel(mozilla::DataChannel* aDataChannel) : mDataChannel(aDataChannel) , mBinaryType(DC_BINARY_TYPE_BLOB) @@ +222,5 @@ > + mBinaryType = DC_BINARY_TYPE_ARRAYBUFFER; > + } else if (aBinaryType.EqualsLiteral("blob")) { > + mBinaryType = DC_BINARY_TYPE_BLOB; > + } else { > + return NS_ERROR_INVALID_ARG; Don't throw @@ +255,5 @@ > + nsresult rv = GetSendParams(aData, msgString, msgStream, isBinary, msgLen, aCx); > + NS_ENSURE_SUCCESS(rv, rv); > + > + // Always increment outgoing buffer len, even if closed > + //mOutgoingBufferedAmount += msgLen; Bah @@ +281,5 @@ > +} > + > +// XXX Exact clone of nsWebSocketChannel::GetSendParams() - find a way to share! > +nsresult > +nsDOMDataChannel::GetSendParams(nsIVariant* aData, nsCString& aStringOut, You really need to use WebIDL. @@ +359,5 @@ > +// Initial implementation: only stores to RAM, not file > +// TODO: bug 704447: large file support > +nsresult > +nsDOMDataChannel::CreateResponseBlob(const nsACString& aData, JSContext* aCx, > + jsval& jsData) JS::Value* aBlob @@ +362,5 @@ > +nsDOMDataChannel::CreateResponseBlob(const nsACString& aData, JSContext* aCx, > + jsval& jsData) > +{ > + uint32_t blobLen = aData.Length(); > + void* blobData = PR_Malloc(blobLen); PR_Malloc? @@ +378,5 @@ > +nsresult > +nsDOMDataChannel::DoOnMessageAvailable(const nsACString& aData, > + bool isBinary) > +{ > + nsresult rv; Declare lower @@ +379,5 @@ > +nsDOMDataChannel::DoOnMessageAvailable(const nsACString& aData, > + bool isBinary) > +{ > + nsresult rv; > + NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread"); MOZ_ASSERT throughout @@ +415,5 @@ > + } > + } else { > + NS_ConvertUTF8toUTF16 utf16data(aData); > + JSString* jsString; > + jsString = JS_NewUCStringCopyN(cx, utf16data.get(), utf16data.Length()); Same line @@ +492,5 @@ > +nsDOMDataChannel::OnChannelClosed(nsISupports* aContext) > +{ > + LOG(("%p(%p): %s - Dispatching\n",this,(void*)mDataChannel,__FUNCTION__)); > + > + One line suffices @@ +521,5 @@ > +NS_NewDOMDataChannel(mozilla::DataChannel* aDataChannel, > + nsPIDOMWindow* aWindow, > + nsIDOMDataChannel** aDomDataChannel) > +{ > + nsresult rv; Lower @@ +528,5 @@ > + > + rv = domdc->Init(aWindow); > + NS_ENSURE_SUCCESS(rv,rv); > + > + return CallQueryInterface(domdc, aDomDataChannel); domdc.forget(aDomDataChannel); return NS_OK;

Comment on attachment 661000 [details] [diff] [review] DataChannel DOM implementation Part 1 >+#include "domstubs.idl" >+ >+#include "nsIEventTarget.idl" >+ >+interface nsIVariant; >+ >+[scriptable, builtinclass, uuid(fb7a8ec4-c1eb-4d9f-b927-fbb8b4493e6d)] >+interface nsIDOMDataChannel : nsIEventTarget nsIDOMEventTarget. nsIEventTarget is something very different in Gecko ;)

Comment on attachment 661889 [details] [diff] [review] Switch nsIEventTarget -> nsIDOMEventTarget per review Delta diff for response to last review comment on nsIEventTarget. Update including this and ms2ger's comments forthcoming.

(In reply to :Ms2ger from comment #40) > Comment on attachment 661000 [details] [diff] [review] > DataChannel DOM implementation Part 1 > > Review of attachment 661000 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/base/public/nsIDOMDataChannel.idl > @@ +15,5 @@ > > + const unsigned short OPEN = 1; > > + const unsigned short CLOSING = 2; > > + const unsigned short CLOSED = 3; > > + > > + readonly attribute unsigned short readyState; > > readyState should be a DOMString Ok. The spec changed this recently to strings with no discussion. Since we're trying to emulate (and be largely plug-competive with) WebSockets, and WebSockets uses integers, I want to check with people on the webrtc list. I'll make the change under the assumption this will stick. > ::: content/base/src/Makefile.in > @@ +145,5 @@ > > +LOCAL_INCLUDES += \ > > + -I$(topsrcdir)/netwerk/sctp/datachannel \ > > + -I$(topsrcdir)/media/webrtc/trunk/third_party/libjingle/source \ > > + $(NULL) > > +endif > > ifdef MOZ_WEBRTC > EXPORTS += \ > nsDOMDataChannel.h \ > $(NULL) > CPPSRCS += \ > nsDOMDataChannel.cpp \ > $(NULL) > LOCAL_INCLUDES += \ > -I$(topsrcdir)/netwerk/sctp/datachannel \ > -I$(topsrcdir)/media/webrtc/trunk/third_party/libjingle/source \ > $(NULL) > endif > > (Definitely no tabs.) I understand that's current Make style. I normally avoid making a small mod to a large existing file enforce style different than the style within the file (which my mods match). If you want to convert style on the entire file, ok, but that's a different issue/bug. > ::: content/base/src/nsDOMDataChannel.cpp > @@ +44,5 @@ > > + public mozilla::DataChannelListener > > +{ > > +public: > > + nsDOMDataChannel(mozilla::DataChannel* aDataChannel) : mDataChannel(aDataChannel), > > + mBinaryType(DC_BINARY_TYPE_BLOB) > > nsDOMDataChannel(mozilla::DataChannel* aDataChannel) > : mDataChannel(aDataChannel) > , mBinaryType(DC_BINARY_TYPE_BLOB) Sure. > @@ +222,5 @@ > > + mBinaryType = DC_BINARY_TYPE_ARRAYBUFFER; > > + } else if (aBinaryType.EqualsLiteral("blob")) { > > + mBinaryType = DC_BINARY_TYPE_BLOB; > > + } else { > > + return NS_ERROR_INVALID_ARG; > > Don't throw Tell WebSockets that ;-) We're trying to emulate as far as reasonable the WebSocket API (and impl). > @@ +255,5 @@ > > + nsresult rv = GetSendParams(aData, msgString, msgStream, isBinary, msgLen, aCx); > > + NS_ENSURE_SUCCESS(rv, rv); > > + > > + // Always increment outgoing buffer len, even if closed > > + //mOutgoingBufferedAmount += msgLen; > > Bah Sorry, cruft and we haven't decided exactly how stats like this will be handled. > > @@ +281,5 @@ > > +} > > + > > +// XXX Exact clone of nsWebSocketChannel::GetSendParams() - find a way to share! > > +nsresult > > +nsDOMDataChannel::GetSendParams(nsIVariant* aData, nsCString& aStringOut, > > You really need to use WebIDL. It wasn't ready when I started this work. I do have a webidl description, but I've never tried it, and it's unclear to me how much I'd need to change the impl to match. > @@ +359,5 @@ > > +// Initial implementation: only stores to RAM, not file > > +// TODO: bug 704447: large file support > > +nsresult > > +nsDOMDataChannel::CreateResponseBlob(const nsACString& aData, JSContext* aCx, > > + jsval& jsData) > > JS::Value* aBlob Copied from WebSockets, and changing it breaks the nsContentUtils::WrapNative() call; that takes a jsval. I suspect I can transform JS::Value to jsval, but I don't see what that buys us here. > @@ +362,5 @@ > > +nsDOMDataChannel::CreateResponseBlob(const nsACString& aData, JSContext* aCx, > > + jsval& jsData) > > +{ > > + uint32_t blobLen = aData.Length(); > > + void* blobData = PR_Malloc(blobLen); > > PR_Malloc? Ditto -- and correct! nsDOMMemoryFile assumes PR_Malloc'd memory, as it uses PR_Free() to release it. > @@ +378,5 @@ > > +nsresult > > +nsDOMDataChannel::DoOnMessageAvailable(const nsACString& aData, > > + bool isBinary) > > +{ > > + nsresult rv; > > Declare lower Ok. > @@ +379,5 @@ > > +nsDOMDataChannel::DoOnMessageAvailable(const nsACString& aData, > > + bool isBinary) > > +{ > > + nsresult rv; > > + NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread"); > > MOZ_ASSERT throughout Matches WebSocket - will change, almost all were MOZ_ASSERT already > @@ +415,5 @@ > > + } > > + } else { > > + NS_ConvertUTF8toUTF16 utf16data(aData); > > + JSString* jsString; > > + jsString = JS_NewUCStringCopyN(cx, utf16data.get(), utf16data.Length()); > > Same line ok > > @@ +492,5 @@ > > +nsDOMDataChannel::OnChannelClosed(nsISupports* aContext) > > +{ > > + LOG(("%p(%p): %s - Dispatching\n",this,(void*)mDataChannel,__FUNCTION__)); > > + > > + > > One line suffices typo > > @@ +521,5 @@ > > +NS_NewDOMDataChannel(mozilla::DataChannel* aDataChannel, > > + nsPIDOMWindow* aWindow, > > + nsIDOMDataChannel** aDomDataChannel) > > +{ > > + nsresult rv; > > Lower ok > @@ +528,5 @@ > > + > > + rv = domdc->Init(aWindow); > > + NS_ENSURE_SUCCESS(rv,rv); > > + > > + return CallQueryInterface(domdc, aDomDataChannel); > > domdc.forget(aDomDataChannel); > return NS_OK; Looking over other DOM NS_New*() functions, this sort of signature and returning CallQueryInterface() is the norm. Why should we be doing it differently here? Is this mishandling the refcnt's?

Comment on attachment 661917 [details] [diff] [review] readyState -> DOMString, and minor changes per ms2ger's review Delta patch to address ms2ger's comments

This is the full rollup patch including both deltas; no other changes.

Comment on attachment 662017 [details] [diff] [review] fire onopen if set after we're already in open state to solve application race conditions (unlike WebSockets) when you get a DataChannel handed to you by ondatachannel it may be open already or will be very soon, by the time the app sets the onopen handler we may have already transitioned to open. With a default implementation of onopen, the app never gets notified and may wait forever for onopen to start sending. This fires onopen if it's set when we're already in the 'open' state. It might not be a bad idea to consider this sort of patch for WebSockets (or if we started doing this generally for state-transition-events). (This is just the DOM portion; we added a ResendOpen() method to DataChannel as well.)

Comment on attachment 661920 [details] [diff] [review] Rollup patch of datachannel code and DOM code including nsIDOMEventTarget and ms2ger patches >+ nsresult >+ DoOnMessageAvailable(const nsACString& message, bool isBinary); Paramaters should be in form aParameterName Same also elsewhere. >+nsDOMDataChannel::SetBinaryType(const nsAString& aBinaryType) >+{ >+ if (aBinaryType.EqualsLiteral("arraybuffer")) { >+ mBinaryType = DC_BINARY_TYPE_ARRAYBUFFER; >+ } else if (aBinaryType.EqualsLiteral("blob")) { >+ mBinaryType = DC_BINARY_TYPE_BLOB; >+ } else { >+ return NS_ERROR_INVALID_ARG; >+ } I doubt NS_ERROR_INVALID_ARG is the right error per the spec. See either WebRTC spec or WebIDL spec for the right error. >+// XXX Exact clone of nsWebSocketChannel::GetSendParams() - find a way to share! Uh, add a method to nsContentUtils and use it in both places? Or add a new DataSender class or some such which both inherit. Would help also with reviewing. Could I still a new patch with that fixed, please.

Comment on attachment 662017 [details] [diff] [review] fire onopen if set after we're already in open state to solve application race conditions If this is really needed, that sounds like a spec bug. Web page shouldn't get access to un-opened channel, or there should be some clear state that it is opened and open event shouldn't have this special case. Also, this is anyway wrong if someone happens to set onopen multiple times. The event should be dispatched only once.

Comment on attachment 662782 [details] [diff] [review] Delta patch in response to last review These are the changes since the last rollup. GetSendParams is no longer shared with WebSockets (since it doesn't use it anymore). We do share the former GetResponseBlob() (now nsContentUtils::GetBlobBuffer()) SetBinaryType() uses the same error return as WebSockets, and none of this is in the spec (we're driving the spec, and patterning on WebSockets).

Updated - modified file from the wrong directory

So, the reason I was trying to fire 'open' if it's set after it's already open is that I wasn't seeing the 'open' events in the app, even though I know they were dispatched from DataChannel after 'datachannel' was dispatched. On the sending side, we get onopen for all events. On the receiving side, none (or maybe one). The sequence on the receiving side is: 1) OpenRequest message received 2) OpenResponse message sent 3) ON_CHANNEL_CREATED sent to PeerConnection (via NS_DispatchToMainThread) 4) PeerConnectionImpl->NotifyDataChannel() is called and creates nsDOMDataChannel 5) runnable dispatched to PeerConnectionObserver (which is in JS) 6) NotifyDataChannel (in JS) runs and calls the onCallBack in the PeerConnection 7) the ondatachannel function in the app sets onopen, etc. Somewhere after this (or more likely perhaps during 5 or 6 above): A) OpenResponseAck received B) DataChannel changes state to OPEN C) ON_CHANNEL_OPEN sent to nsDOMDataChannel (via NS_DispatchToMainThread) D) OnChannelConnected is called and called DispatchDOMEvent() for "open". So, *perhaps* the issue is the multiple trips through the main thread event queue for ondatachannel, while the open event makes less trips and thus (maybe) can 'jump' ahead of the final ondatachannel runnable event. Maybe that's not the reason, but we don't get the open event in the app in any case. If we wait for the JS to acknowledge the datachannel event in some way and queue the open event (and any data messages), obviously we're good. Or don't send OpenResponse until the datachannel is set up and has notified the low-level code.

(In reply to Randell Jesup [:jesup] from comment #54) > SetBinaryType() uses the same error return as WebSockets, and none of this > is in the spec (we're driving the spec, and patterning on WebSockets). It doesn't. WebSocket has void SetBinaryType(dom::BinaryType aData) { mBinaryType = aData; }

(In reply to Randell Jesup [:jesup] from comment #58) > If we wait for the JS to acknowledge the datachannel event in some way and > queue the open event (and any data messages), obviously we're good. Or > don't send OpenResponse until the datachannel is set up and has notified the > low-level code. Either way sounds ok.

(In reply to :Ms2ger from comment #59) > (In reply to Randell Jesup [:jesup] from comment #54) > > SetBinaryType() uses the same error return as WebSockets, and none of this > > is in the spec (we're driving the spec, and patterning on WebSockets). > > It doesn't. WebSocket has > > void SetBinaryType(dom::BinaryType aData) { mBinaryType = aData; } My apologies, WebSockets *had* the exact same implementation until a week or so ago.

Comment on attachment 662795 [details] [diff] [review] Updated rollup patch >+// XXX Exact clone of nsWebSocketChannel::GetSendParams() - find a way to share! So this comment isn't valid anymore?

Yup, realized after I posted it. I'll remove that line. Thanks! I'm working on a solution to the open event issue

Grr, hit return instead of tab - smaug, the patch here works with another patch I'll add to bug 729512 (and mods to PeerConnectionImpl.cpp to invoke NS_DataChannelAppReady()). Basically, I queue both CONNECTING->OPEN state changes and incoming data messages until we get positive confirmation that we've delivered the datachannel event to the PeerConnection object. My test is much happier.

Comment on attachment 663130 [details] [diff] [review] delta patch to solve open/message event races against datachannel events >+ virtual void >+ AppReady(); could be just one line. >+// Tell DataChannel it's ok to deliver open and message events >+void NS_DataChannelAppReady(nsIDOMDataChannel* domDataChannel); aDOMDataChannel

This landed on inbound (without a changeset link posted here, apparently...), but had to be backed out due to mochitest-3 failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/94279b84aee8 https://tbpl.mozilla.org/php/getParsedLog.php?id=15791309&tree=Mozilla-Inbound 13366 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_interfaces.html | Unexpected interface name in global scope: DataChannel

The patch adds a new interface so it should be added to test_interfaces.html

reland: https://hg.mozilla.org/integration/mozilla-inbound/rev/37ac46f7dd40 https://hg.mozilla.org/integration/mozilla-inbound/rev/de48716d5b48

Going to mark verified given that we've confirmed for months that this is hooked up (confirmed the feature is landed).