798825 - Add DataChannel DOM interfaces to RTCPeerConnection

Status: VERIFIED FIXED

(Core :: WebRTC, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

Attachment: DataChannel dom and related fixes

These were removed when reviewing the initial PeerConnection DOM with jst

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Correct patch

Argh.   Need to pop old patch and push new one

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 668834 [details] [diff] [review]
Correct patch

I hope there is a bug open to implement PeerConnection in C++ so that it can become a real event target.

>+  createDataChannel: function(label, dict) {
>+    if (dict.maxRetransmitTime != undefined &&
>+        dict.maxRetransmitNum != undefined) {
>+      throw new Error("maxRetransmitTime not provided");
>+    }
I don't understand the error. Also, dict is marked as optional in the interface
so it can be null here.

>+  notifyDataChannel: function(channel) {
>+    if (this._dompc.ondatachannel) {
>+      try {
>+        this._dompc.ondatachannel.onCallback(channel);
>+      } catch(e) {}
>+    }
>+    this._dompc._executeNext();
>+  },
>+
>+  notifyConnection: function() {
>+    if (this._dompc.onconnection) {
>+      try {
>+        this._dompc.onconnection.onCallback();
>+      } catch(e) {}
>+    }
>+    this._dompc._executeNext();
>+  },
>+
>+  notifyClosedConnection: function() {
>+    if (this._dompc.onclosedconnection) {
>+      try {
>+        this._dompc.onclosedconnection.onCallback();
>+      } catch(e) {}
>+    }
>+    this._dompc._executeNext();
Calling this._dompc._executeNext() right after calling the callback (effectively event listener)
is most probably wrong.
These kinds of events happen usually of a task, asynchronously, and the next thing after them happens also async.
It looks like _executeNext doesn't guarantee such behavior.


> [scriptable, uuid(84efc76f-41d9-496a-9444-2965d179d419)]
>@@ -23,16 +24,21 @@ interface IPeerConnectionObserver : nsIS
>   void onCreateOfferError(in unsigned long code);
>   void onCreateAnswerSuccess(in string answer);
>   void onCreateAnswerError(in unsigned long code);
>   void onSetLocalDescriptionSuccess(in unsigned long code);
>   void onSetRemoteDescriptionSuccess(in unsigned long code);
>   void onSetLocalDescriptionError(in unsigned long code);
>   void onSetRemoteDescriptionError(in unsigned long code);
> 
>+  /* Data channel callbacks */
>+  void notifyDataChannel(in nsIDOMDataChannel channel);
>+  void notifyConnection();
>+  void notifyClosedConnection();
>+
>   /* Notification of one of several types of state changed */
>   void onStateChange(in unsigned long state);
> 
>   /* Changes to MediaStreams */
>   void onAddStream(in nsIDOMMediaStream stream, in string type);
>   void onRemoveStream();
>   void onAddTrack();
>   void onRemoveTrack();
You're modifying an existing interface so uuid should be updated.

>@@ -89,9 +95,16 @@ interface IPeerConnection : nsISupports
> 
>   /* Attributes */
>   readonly attribute string localDescription;
>   readonly attribute string remoteDescription;
> 
>   readonly attribute unsigned long iceState;
>   readonly attribute unsigned long readyState;
>   readonly attribute unsigned long sipccState;
>+
>+  /* Data channels */
>+  nsIDOMDataChannel createDataChannel(in ACString label,
>+    in unsigned short type, in boolean outOfOrderAllowed,
>+    in unsigned short maxTime, in unsigned short maxNum);
>+  void connectDataConnection(in unsigned short localport,
>+    in unsigned short remoteport, in unsigned short numstreams);
> };
You're modifying an existing interface so uuid should be updated.

>+  /* Data channels */
>+  nsIDOMDataChannel createDataChannel([optional] in ACString label,
>+    [optional] in jsval options);
>+  void connectDataConnection(in unsigned short localport,
>+    in unsigned short remoteport, [optional] in unsigned short numstreams);
>+  attribute RTCPeerConnectionCallback ondatachannel;
>+  attribute RTCPeerConnectionCallbackVoid onconnection;
>+  attribute RTCPeerConnectionCallbackVoid onclosedconnection;
onclosedconnection or onconnectionclosed?

You're modifying an existing interface so uuid should be updated.


>+PeerConnectionImpl::ConnectDataConnection(PRUint16 localport,
>+                                          PRUint16 remoteport,
>+                                          PRUint16 numstreams)
>+{
Parameter names should be in form aParameterName

>+#ifdef MOZILLA_INTERNAL_API
>+    mDataConnection = new mozilla::DataChannelConnection(this);
>+    NS_ENSURE_TRUE(mDataConnection,NS_ERROR_FAILURE);
space after ,

>+    if (!mDataConnection->Init(localport, numstreams, true)) {
>+      CSFLogError(logTag,"%s DataConnection Init Failed",__FUNCTION__);
>+      return NS_ERROR_FAILURE;
>+    }
>+    // XXX Fix! Get the correct flow for DataChannel. Also error handling.
>+    nsRefPtr<TransportFlow> flow = GetTransportFlow(1,false).get();
space after ,

>+    CSFLogDebugS(logTag, "Transportflow[1] = " << flow.get());
>+    if (!mDataConnection->ConnectDTLS(flow, localport, remoteport)) {
>+      return NS_ERROR_FAILURE;
>+    }
>+    return NS_OK;
>+#else
>+    return NS_ERROR_FAILURE;
>+#endif
>+}
I hope there is somewhere a comment why the ifdef is needed.

>+
>+NS_IMETHODIMP
>+PeerConnectionImpl::CreateDataChannel(const nsACString& label,
>+                                      PRUint16 type,
>+                                      bool outOfOrderAllowed,
>+                                      PRUint16 maxTime,
>+                                      PRUint16 maxNum,
>+                                      nsIDOMDataChannel** aRetval)
>+{
Parameter names should be in form aParameterName

>+  MOZ_ASSERT(aRetval);
>+
>+#ifdef MOZILLA_INTERNAL_API
>+  mozilla::DataChannel *aDataChannel;
local variables should not be name aVariableName. a -prefix is for parameters/arguments.

>-  NS_ENSURE_TRUE(sizeof(*sac) >= sac->sac_length, /* */);
>+  NS_ENSURE_TRUE(sac->sac_length >= sizeof(*sac), /* */);
You want NS_ENSURE_TRUE_VOID here

Comment 3

[Anant Narayanan [:anant]]

(In reply to Olli Pettay [:smaug] from comment #2) 
> I hope there is a bug open to implement PeerConnection in C++ so that it can
> become a real event target.

On the contrary, there is a bug open to allow JS to implement a real event target :) Bug 731746.
 
> >+  createDataChannel: function(label, dict) {
> >+    if (dict.maxRetransmitTime != undefined &&
> >+        dict.maxRetransmitNum != undefined) {
> >+      throw new Error("maxRetransmitTime not provided");
> >+    }
> I don't understand the error. Also, dict is marked as optional in the
> interface
> so it can be null here.

Not sure, according to the spec I think we should ignore it too, but there was a comment saying "throw error". Jesup?

> >+  notifyClosedConnection: function() {
> >+    if (this._dompc.onclosedconnection) {
> >+      try {
> >+        this._dompc.onclosedconnection.onCallback();
> >+      } catch(e) {}
> >+    }
> >+    this._dompc._executeNext();
> Calling this._dompc._executeNext() right after calling the callback
> (effectively event listener)
> is most probably wrong.
> These kinds of events happen usually of a task, asynchronously, and the next
> thing after them happens also async.
> It looks like _executeNext doesn't guarantee such behavior.

I don't understand - _executeNext simply executes the next pending task, this is to ensure only one operation is executing at any given time for one PeerConnection. So for example, if content JS had called close() or addStream() right after createDataChannel or something else, it would be executed. It's all being done asynchronously (except createDataChannel which returns a value).

> You're modifying an existing interface so uuid should be updated.

If we push everything in one go as planned, this won't be necessary. However, if we do data channel as a separate push, then I suppose we should.

I'll address the C++ nits.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Anant Narayanan [:anant] from comment #3)
> (In reply to Olli Pettay [:smaug] from comment #2) 
> > I hope there is a bug open to implement PeerConnection in C++ so that it can
> > become a real event target.
> 
> On the contrary, there is a bug open to allow JS to implement a real event
> target :) Bug 731746.

I'm sure Anant went over this issue with JST during PeerConnection review

>  
> > >+  createDataChannel: function(label, dict) {
> > >+    if (dict.maxRetransmitTime != undefined &&
> > >+        dict.maxRetransmitNum != undefined) {
> > >+      throw new Error("maxRetransmitTime not provided");
> > >+    }
> > I don't understand the error. Also, dict is marked as optional in the
> > interface
> > so it can be null here.

Yes, null should be allowed - though when I tried I had some trouble getting that to work.

> 
> Not sure, according to the spec I think we should ignore it too, but there
> was a comment saying "throw error". Jesup?

It's invalid to specify *both* retransmitTime and retransmitNum - they implicitly select a transmission mode, and you can't do both at once.  Change the error text, and it's correct.

> 
> > >+  notifyClosedConnection: function() {
> > >+    if (this._dompc.onclosedconnection) {
> > >+      try {
> > >+        this._dompc.onclosedconnection.onCallback();
> > >+      } catch(e) {}
> > >+    }
> > >+    this._dompc._executeNext();
> > Calling this._dompc._executeNext() right after calling the callback
> > (effectively event listener)
> > is most probably wrong.
> > These kinds of events happen usually of a task, asynchronously, and the next
> > thing after them happens also async.
> > It looks like _executeNext doesn't guarantee such behavior.
> 
> I don't understand - _executeNext simply executes the next pending task,
> this is to ensure only one operation is executing at any given time for one
> PeerConnection. So for example, if content JS had called close() or
> addStream() right after createDataChannel or something else, it would be
> executed. It's all being done asynchronously (except createDataChannel which
> returns a value).

And also again, this mechanism was approved by JST; we're just adding new instances.

> 
> > You're modifying an existing interface so uuid should be updated.
> 
> If we push everything in one go as planned, this won't be necessary.
> However, if we do data channel as a separate push, then I suppose we should.
> 
> I'll address the C++ nits.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Anant Narayanan [:anant] from comment #3)
> On the contrary, there is a bug open to allow JS to implement a real event
> target :) Bug 731746.

I'm not aware of anyone from DOM team working on the bug.
The approach Shih-Chiang Chien's patch has is just a temporary hack.
(But, perhaps someone is actually implementing the plan we had last DOM work week.
 I'm just not aware who.)


> 
> I don't understand - _executeNext simply executes the next pending task,
But not task from task queue/ event loop, but something else.
Normal event loop should be used here.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(Anyhow, eventtarget implementation in C++ or JS discussion isn't about this bug, but some followup)

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Ok, this._dompc._executeNext(); was explained to me on IRC.
It is quite unusual setup, but I guess it should work here.

Comment 8

[Anant Narayanan [:anant]]

Attachment: patch v2

Address review comments.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 668852 [details] [diff] [review]
patch v2

>+// Data channels won't work without a window, so in order for the C++ unit
>+// tests to work (it doesn't have a window available) we ifdef the following
>+// two implementations.
>+NS_IMETHODIMP
>+PeerConnectionImpl::ConnectDataConnection(PRUint16 aLocalport,
>+                                          PRUint16 aRemoteport,
>+                                          PRUint16 aNumstreams)
>+{
>+#ifdef MOZILLA_INTERNAL_API
>+    mDataConnection = new mozilla::DataChannelConnection(this);
>+    NS_ENSURE_TRUE(mDataConnection,NS_ERROR_FAILURE);
>+    if (!mDataConnection->Init(aLocalport, aNumstreams, true)) {
>+      CSFLogError(logTag,"%s DataConnection Init Failed",__FUNCTION__);
>+      return NS_ERROR_FAILURE;
>+    }
>+    // XXX Fix! Get the correct flow for DataChannel. Also error handling.
>+    nsRefPtr<TransportFlow> flow = GetTransportFlow(1,false).get();
>+    CSFLogDebugS(logTag, "Transportflow[1] = " << flow.get());
>+    if (!mDataConnection->ConnectDTLS(flow, aLocalport, aRemoteport)) {
>+      return NS_ERROR_FAILURE;
>+    }
>+    return NS_OK;
>+#else
>+    return NS_ERROR_FAILURE;
>+#endif
>+}
Looks like this method is using 4 and 2 space indentation. Should be always 2.

>+PeerConnectionImpl::CreateDataChannel(const nsACString& aLabel,
>+                                      PRUint16 aType,
>+                                      bool outOfOrderAllowed,
>+                                      PRUint16 aMaxTime,
>+                                      PRUint16 aMaxNum,
>+                                      nsIDOMDataChannel** aRetval)
>+{
>+  MOZ_ASSERT(aRetval);
>+
>+#ifdef MOZILLA_INTERNAL_API
>+  mozilla::DataChannel *dataChannel;
* goes with the type, not with variable name

>+  mozilla::DataChannelConnection::Type theType =
>+    (mozilla::DataChannelConnection::Type) aType;
static_cast<mozilla::DataChannelConnection::Type>


>+  if (!mDataConnection) {
>+    return NS_ERROR_FAILURE;
>+  }
>+  dataChannel = mDataConnection->Open(
>+    aLabel, theType, !outOfOrderAllowed,
>+    aType == mozilla::DataChannelConnection::PARTIAL_RELIABLE_REXMIT ? aMaxNum :
>+    (aType == mozilla::DataChannelConnection::PARTIAL_RELIABLE_TIMED ? aMaxTime : 0),
>+    NULL, NULL
nullptr, nullptr



Assuming onclosedconnection is what we really want, r=me

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/884757053d1d

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/884757053d1d

Comment 12

[Jason Smith [:jsmith]]

Already verified through knowing the feature is landed with a sanity test through the given test page on webrtc landing.