Rewrite PeerConnection in JS-implemented WebIDL

VERIFIED FIXED in mozilla24

Status

()

Core
WebRTC
--
enhancement
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: abr, Assigned: jib)

Tracking

(Depends on: 1 bug)

unspecified
mozilla24
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRTC] [blocking-webrtc-], URL)

Attachments

(4 attachments, 20 obsolete attachments)

5.10 KB, patch
jesup
: checkin+
Details | Diff | Splinter Review
66.87 KB, patch
jib
: review+
jesup
: checkin+
Details | Diff | Splinter Review
2.65 KB, patch
jib
: review+
jesup
: checkin+
Details | Diff | Splinter Review
26.85 KB, patch
jib
: review+
jesup
: checkin+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
As suggested by :smaug in Bug 797534 comment 11: "Would be so nice to see Peerconnection to be rewritten in C++ using WebIDL bindings at some point. Reviewing would be easier and behavior would be more correct."
(Reporter)

Updated

5 years ago
Whiteboard: [WebRTC] [blocking-webrtc-]
It'd be interesting to understand the reasoning here in significantly more detail in order to really weigh the described pros against the cons of (at least) the loss of memory safety and corresponding potential increase of attack-surface.

Comment 2

5 years ago
+ (1) PeerConnection is an EventTarget. EventTarget objects can't be implemented in JS atm, nor
      any time near future. So the current implementation is rather hacky.
      (There is a plan to let JS implement EventTarget interfaces, but I don't think we have
      resources atm to fix that.)
+ (2) Right now it is impossible to implement interfaces per specs using JS. This may change somewhat
      soon if we implement JS backend for WebIDL bindings. Bz has ideas how to do this.
+ (3) I believe reviewers in this area are much more comfortable with C++. More people know (I mean know
      reasonable well) how C++ stuff is exposed to content JS than how chrome JS is exposed to content JS.
      if (2) gets fixed, then (3) may be no problem anymore.
+ (5) As of now it is easier to optimize out cycle collectable C++ objects from cycle collection than
      optimize garbage collection. The more we add JS the better gc we need, in general. CC optimizations are
      easier, at least atm.

- (6) Yes, memory safety problems of C++ can be an issue if the implementation isn't good.
      Though, I believe we've become a lot better with this. Cycle collector helps - no need to
      use raw pointers to avoid leaks.

Comment 3

5 years ago
Of course, those are quite subjective views.
Indeed; thanks for clarifying!  My views are plenty subjective too, of course.  :-)
(Reporter)

Updated

4 years ago
Blocks: 842531
Bug 731746 would appear to track the work to implement DOM EventTargets in chrome JS.

Comment 6

4 years ago
EventTargets will be still implemented in C++ only, but the idea is that JS implementations can
extend the base C++ implemented EventTarget.

(But I still believe PeerConnection should be in C++. People hacking this code are more familiar
with C/C++ I believe and cycle collection is easier to optimize in C++.)
It looks like PeerConnection has a constructor that takes multiple arguments, which will require some additional work.  I'm guessing, then, that it doesn't involve Navigator at all, so it doesn't relate to the JS-implemented WebIDL I'm working on right now.
I also see some optional arguments with default values.
Depends on: 841429
Summary: Rewrite PeerConnection in C++/WebIDL → Rewrite PeerConnection in JS-implemented WebIDL
Yes, PeerConnection doesn't hang off of navigator.
(Assignee)

Updated

4 years ago
Assignee: nobody → jib
(Assignee)

Updated

4 years ago
Depends on: 863386
(Assignee)

Updated

4 years ago
Depends on: 863402
Depends on: 858741
Depends on: 863880
Depends on: 863898
(Assignee)

Updated

4 years ago
Depends on: 864442
Depends on: 843264
bz says the use of callbacks for PeerConnection should be okay, so bug 858741 isn't blocking here.
No longer depends on: 858741
(Assignee)

Updated

4 years ago
Depends on: 851639
What is bug 851639 needed for?  I don't see anything in dom/media/PeerConnection.manifest that gets registered on navigator.
I believe what's needed here from that bug is the "init with a window" bits.
Sure, that would be easy enough to do, though I thought your plan was bug 865377 to handle something like that.  Either way is fine with me.
I think those are somewhat independent: we can have JS-implemented stuff that's not an EventTarget...
Depends on: 865544
No longer depends on: 851639
(Assignee)

Comment 15

4 years ago
> I believe what's needed here from that bug is the "init with a window" bits.

Yes, sorry that's why I linked it. I'm running a modified version of that bug's patch to give me the init() function I need to keep going (works! Thanks!) Splitting it out seems good for cleanliness and tracking.
Depends on: 865951
(Assignee)

Comment 16

4 years ago
Removing bug 865951 as a blocker since the workaround in it is fine here.

Remaining blockers for landing new PeerConnection in FF23 are:

 - Bug 851178. Constructor-args are needed for mozPeerConnection itself, createOffer
   and other essential methods. Mochitests fail without this and drop-in is a no-go.
   I think Andrew said it is next on his list.

 - Bug 863402 - Need more investigation to determine if this is critical. Open to
   workaround suggestions as well.
No longer depends on: 865951
We don't do serializer stuff now, I bet.

That said, the workaround I described in that bug, which is to allow toJSON in the parser and just implement it, should work.
(Assignee)

Updated

4 years ago
Depends on: 868448
(Assignee)

Comment 18

4 years ago
Created attachment 745279 [details] [diff] [review]
Rewrite PeerConnection in JS-implemented WebIDL (WIP)

Works and passes all mochitests (disabled 5 of them and modified one), so I wanted to upload a patch as a backup point if nothing else.

Notes:
- You need patch in bug 868448 (not landed as of writing) for test to succeed.
- Still includes some backwards compatible attributes (localDescription etc.)
  that we'll likely remove or rig with deprecation errors.

TODO:
- Integrate with bug 851178 (hopefully just removing workaround code).
- Only half of event handlers work. Refactor iceState etc. to match spec.

Try - https://tbpl.mozilla.org/?tree=Try&rev=477778884279
(Assignee)

Comment 19

4 years ago
Created attachment 745339 [details] [diff] [review]
Rewrite PeerConnection in JS-implemented WebIDL (2)

Minor cleanup so we can start reviews on parts. Todo list remains.
Attachment #745279 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #745339 - Flags: review?(continuation)
Attachment #745339 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Depends on: 869272
The patches here for the mochitests aren't the final version to be landed per the comments above, right? The code changes to turn tests off and modify the constructors feel off, but I'm assuming you are waiting for bug 851178 here, right?
(Assignee)

Comment 21

4 years ago
That's correct. I'm hoping to upload a new patch with all the mochis enabled today.
(Assignee)

Comment 22

4 years ago
Created attachment 746467 [details] [diff] [review]
Rewrite PeerConnection in JS-implemented WebIDL (3)

Updated.
Now works with patch in bug 851178 (required)

- Constructor arguments just work.
- Re-enabled all mochitests. All mochis pass.
- Still includes some backwards compatible attributes (localDescription etc.)
  that we'll likely remove or rig with deprecation errors.

TODO:
- Only half of event handlers work. Refactor iceState etc. to match spec.
I'm ready to start reviewing this.  Is this ready to go?  Should I look at version three (746467)?
(Assignee)

Comment 24

4 years ago
Hi, thanks! If you give me a few minutes I'll have a newer one.
Comment on attachment 746467 [details] [diff] [review]
Rewrite PeerConnection in JS-implemented WebIDL (3)

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

Here are a few comments on v3.

::: content/base/src/nsDOMDataChannel.cpp
@@ -166,5 @@
>    // Attempt to kill "ghost" DataChannel (if one can happen): but usually too early for check to fail
>    rv = CheckInnerWindowCorrectness();
>    NS_ENSURE_SUCCESS(rv,rv);
>  
> -  // See bug 696085

Why did you delete this comment?

::: dom/media/PeerConnection.js
@@ +121,5 @@
> +    this.sdpMid = candidateInitDict.sdpMid || null;
> +    this.sdpMLineIndex = (candidateInitDict.sdpMLineIndex === null)?
> +                         null : candidateInitDict.sdpMLineIndex + 1;
> +  } else {
> +    this.candiate = this.sdpMid = this.sdpMLineIndex = null;

candidate not candiate?

@@ +134,2 @@
>  
> +  init: function(win, candidateInitDict) {

drop the unused argument candidateInitDict?

@@ +204,5 @@
> +    }
> +  },
> +
> +  init3: function() {
> +    this.__DOM_IMPL__.initEvent(this.type, false, false);

You should be able to move this into __init now

@@ +237,5 @@
> +      this.candidate = eventInitDict.candidate || null;
> +    }
> +  },
> +
> +  init3: function() {

move this into __init

@@ -642,5 @@
>        return null;
>      }
> -    return {
> -      type: this._localType, sdp: sdp,
> -      __exposedProps__: { type: "rw", sdp: "rw" }

yay!  death to exposedProps!

@@ +966,5 @@
>  
> +  onAddStream: function(stream) {
> +    let event = new this._dompc._win.mozMediaStreamEvent("addstream",
> +                                                         { stream: stream });
> +    event.init3();

I assume these init3() things are all fake constructor stuff that can go away.

::: dom/webidl/MediaStreamEvent.webidl
@@ +13,5 @@
> + Constructor(DOMString type, optional mozMediaStreamEventInit eventInitDict)]
> +interface mozMediaStreamEvent : Event {
> +  readonly attribute MediaStream? stream;
> +  // See bug 851178
> +  void init3();

You should be able to remove this now.

::: dom/webidl/RTCIceCandidate.webidl
@@ +9,5 @@
> +  DOMString sdpMid;
> +  unsigned short sdpMLineIndex;
> +};
> +
> +[Pref="media.peerconnection.enabled",

Are there any tests that you can't construct these things when peer connection is disabled?

::: dom/webidl/RTCPeerConnectionIceEvent.webidl
@@ +14,5 @@
> +             optional mozRTCPeerConnectionIceEventInit eventInitDict)]
> +interface mozRTCPeerConnectionIceEvent : Event {
> +  readonly attribute mozRTCIceCandidate candidate;
> +  // See bug 851178
> +  void init3();

This can be removed now.
(Assignee)

Comment 26

4 years ago
Created attachment 746705 [details] [diff] [review]
Rewrite PeerConnection in JS-implemented WebIDL (4)

Fixed some bugs found with apprtc not detected by mochitests. I'm still not connecting between Firefox on Mac and Chrome on Windows, so will need to investigate more. Still, code is ripe for review I think.

Still have to fix icestate.

>> -  // See bug 696085
>
> Why did you delete this comment?

Because it referenced PeerConnection, which is no longer activated in this file. Is the comment still relevant? If so I'll gladly put it back, if someone can tell me how to reword it to not mention peerconnection.

Andrew, I've fixed your other nits so far. Thanks!
Attachment #745339 - Attachment is obsolete: true
Attachment #746467 - Attachment is obsolete: true
Attachment #745339 - Flags: review?(continuation)
Attachment #745339 - Flags: review?(bzbarsky)
Attachment #746705 - Flags: review?(continuation)
Attachment #746705 - Flags: review?(bzbarsky)
Comment on attachment 746705 [details] [diff] [review]
Rewrite PeerConnection in JS-implemented WebIDL (4)

>+++ b/content/events/src/nsDOMEvent.h
>+  void init(mozilla::dom::EventTarget* aOwner, nsPresContext* aPresContext,

"Init" with a capital "I", please.

>+++ b/dom/media/PeerConnection.js

>+function RTCIceCandidate(candidateInitDict) {
>+  if (candidateInitDict !== undefined) {

It's worth a comment about when this is expected to be undefined and when not.

>+    this.candidate = candidateInitDict.candidate || null;

So if candidateInitDict.candidate === "", do we want to set this.candidate to null?

It seems like what this really wants to be doing is more like:

  this.candidate = ("candidate" in candidateInitDict) ?
    candidateInitDict.candidate : null;

but it really depends on the behavior you want.

>+    this.sdpMid = candidateInitDict.sdpMid || null;

Similar.

>+    this.sdpMLineIndex = (candidateInitDict.sdpMLineIndex === null)?
>+                         null : candidateInitDict.sdpMLineIndex + 1;

candidateInitDict.sdpMLineIndex will never === null.  It'll either not be set at all (and undefined !== null) or it'll be an integer.

>+  __init: function(candidateInitDict) {
>     if (candidateInitDict !== undefined) {

Might be good to share the code with the RTCIceCandidate constructor via a helper function, if we really need it both places.

Note that candidateInitDict will never be undefined here.

>+  __init: function(descriptionInitDict) {

Same comments as above about things like descriptionInitDict.sdp === ""

>+MediaStreamEvent.prototype = {
>+    this.__DOM_IMPL__.initEvent(this.type, false, false);

The second and third arg need to come from the eventInitDict.

>+    if (eventInitDict !== undefined) {

It's never undefined.

>+  toString: function() {
>+    return JSON.stringify({ type: this.type, stream: this._stream });
>+  }

Why?  Is this just for our own internal debugging purposes?  Content will never see this bit as things stand.

>+RTCPeerConnectionIceEvent.prototype = {
>+    this.__DOM_IMPL__.initEvent(this.type, false, false);

Again, pass the right values for the second and third arg.

>+    if (eventInitDict !== undefined) {

And here.

>+  set onaddstream(handler)    { this.addEventListener("addstream", handler); },
>+  set onicecandidate(handler) { this.addEventListener("icecandidate", handler); },

That looks wrong.  First of all, you need getters, not just setters.  Second, doing |foo.onaddstream = null| is supposed to _remove_ the event listener.  Then there's the false return needing to preventDefault bit, etc, etc.

I'll look into setting something up for you here that doesn't suck on the C++ side, but if that fails I'll describe what the js here should be doing....

>   addIceCandidate: function(cand, onSuccess, onError) {
>+    if (!cand || (!cand.candidate && !cand.sdpMLineIndex)) {

cand can't test calse here, as far as I can tell.

Also, this changed the logic: before we threw if either of cand.candidate and cand.sdpMLineIndex tested false; now we only throw if both test false.  Why the change?

>   addStream: function(stream, constraints) {
>+    if (!stream || stream.currentTime === undefined) {

Likewise, stream will never test false.

>+++ b/dom/webidl/MediaStreamEvent.webidl

It's a good idea to put the spec link in a comment after the license.

>+++ b/dom/webidl/RTCConfiguration.webidl
>+dictionary mozRTCIceServer {

Why the renaming?  Just because it doesn't quite match what the spec has?

>+++ b/dom/webidl/RTCConstraints.webidl
>+  boolean? OfferToReceiveAudio;

So that allows four possible values for OfferToReceiveAudio: not set, null, false, true.  Why do we explicitly allow the "null" value here?

>+  boolean? OfferToReceiveVideo;
>+  boolean? MozDontOfferDataChannel;
>+  boolean? VoiceActivityDetection;

Same thing.

>+  mozRTCIceTransports? IceTransports;
>+  mozRTCRequestIdentity? RequestIdentity;

And here, similar.

>+dictionary mozRTCConstraints {
>+    object? mandatory = null;
>+    sequence<object>? _optional;

Someone who actually understands this stuff should review the code actually examining these objects, with an understanding that nothing guarantees that isObject(constraints.optional[i]) will return the same thing when called twice for the same i and that sort of thing.

Of slightly more worry to me, I don't think sequence<object> is currently safe; see bug 868715.  Unfortunately, we allow it to compile...

Can we do anything at all with the optional constraints to use a sequence of dictionaries instead?

Also, I can't find the IDL that uses this dictionary.  Is there just a file missing, or is it just unused?

>+++ b/dom/webidl/RTCPeerConnection.webidl

>+enum mozRTCSignalingState {
>+enum mozRTCIceGatheringState {
>+enum mozRTCIceConnectionState {

Why the "moz"?

>+              optional object? constraints)]

Again, someone needs to make sure we're very very extremely super-careful about working with that object.

Worth documenting why it's nullable (because you're using it as a poor-man's dictionary).

>+interface mozRTCPeerConnection : EventTarget  {

Why mozRTCPeerConnection instead of RTCPeerConnection?  That seems wrong, since the interface name is what the constructor ends up as....  Unless we're shipping this prefixed for some reason?

This generally applies to all the other constructible interfaces in this patch.

>+  void createOffer (mozRTCSessionDescriptionCallback successCallback,
>+                    mozRTCPeerConnectionErrorCallback? failureCallback, // apprtc
>+                    optional object? constraints);

This seems like something that will end up as a Future in the future...  I guess that can be done via overloads.  :(

Again, someone needs to quadruple-check our use of the constraints argument.

In the spec failureCallback is not nullable.  Why is it here and in createAnswer?

>+//  readonly attribute mozRTCSessionDescription localDescription;
>+//  readonly attribute mozRTCSessionDescription remoteDescription;

Why?  This used to work, afaict. Do we not have any tests for it?

>+  attribute EventHandler onnegotiationneeded;
>+  attribute EventHandler onicecandidate;
>+  attribute EventHandler onsignalingstatechange;
>+  attribute EventHandler onaddstream;
>+  attribute EventHandler onremovestream;
>+  attribute EventHandler oniceconnectionstatechange;

I don't see any provisions for making some of these work.  We need to either do that or remove the properties; right now it looks like all except onaddstream and onicecandidate will let you set and get them but not actually get you any events, right?  Is that expected?

>+  // Four backwards-compatible attributes (to pass mochitests)
>+  attribute mozRTCSessionDescription localDescription;
>+  attribute mozRTCSessionDescription remoteDescription;

Then take out the commented bits above?  Or at least document above why these need to be writable?

But they were not writable in the XPIDL, so why writable here?

>+  readonly attribute object localStreams;
>+  readonly attribute object remoteStreams;

If these are not part of the spec, just put them on a partial interface documented to be a Mozilla extension?

r=me with the above fixed, but I'd love to see an interdiff addressing the comments
Attachment #746705 - Flags: review?(bzbarsky) → review+
Comment on attachment 746705 [details] [diff] [review]
Rewrite PeerConnection in JS-implemented WebIDL (4)

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

Mochitest updates look okay with one question inline. Btw, for any bugs you caught that was not caught by the mochitests, can you indicate which areas specifically? I'd like to get bugs on file to track the automation weaknesses so we can look into this in the future.

::: dom/media/tests/mochitest/test_peerConnection_bug825703.html
@@ +41,5 @@
>      exception = null;
>  
>      makePC(1, false);
>  
> +    makePC({}, true);

What's the reasoning behind this now being true and the associated tests being removed?
Depends on: 870219
(Assignee)

Comment 29

4 years ago
>>+++ b/content/events/src/nsDOMEvent.h
>>+  void init(mozilla::dom::EventTarget* aOwner, nsPresContext* aPresContext,
>
>"Init" with a capital "I", please.

Ok, there's a preexisting Init() function (with different args, but still), so I'm renaming the new one ConstructorInit().

>+++ b/dom/media/PeerConnection.js

>>+function RTCIceCandidate(candidateInitDict) {
>>+  if (candidateInitDict !== undefined) {
>
>It's worth a comment about when this is expected to be undefined and when not.

Actually, thanks to webidl it'll never be undefined. Deleting! :-)

>>+    this.candidate = candidateInitDict.candidate || null;
>
>So if candidateInitDict.candidate === "", do we want to set this.candidate to null?

Good catch! Probably not, since null when used with an Event is overloaded to mean "done" in the spec in one case.

>It seems like what this really wants to be doing is more like:
>
>  this.candidate = ("candidate" in candidateInitDict) ?
>    candidateInitDict.candidate : null;

Thanks, I'll use that throughout. Or would an even simpler pattern be to use defaults in webidl? Like this:

  dictionary mozRTCIceCandidateInit {
    DOMString? candidate = null;
    DOMString? sdpMid = "";
    unsigned short sdpMLineIndex;
  }

Then I shouldn't need the ()?:

> candidateInitDict.sdpMLineIndex will never === null.  It'll either not be set at
> all (and undefined !== null) or it'll be an integer.

I'll use your pattern here.

>>+  __init: function(candidateInitDict) {
>>     if (candidateInitDict !== undefined) {
>
>Might be good to share the code with the RTCIceCandidate constructor via a helper
>function, if we really need it both places.
>
>Note that candidateInitDict will never be undefined here.

I'll delete the redundancy since constructors never have args.

>>+  __init: function(descriptionInitDict) {
>
>Same comments as above about things like descriptionInitDict.sdp === ""

Willdo. I'm not even sure why null is in the spec here (I'll make a note to ask).

>>+MediaStreamEvent.prototype = {
>>+    this.__DOM_IMPL__.initEvent(this.type, false, false);
>
>The second and third arg need to come from the eventInitDict.

I think you're right. I misinterpreted http://dev.w3.org/2011/webrtc/editor/webrtc.html#mediastreamevent on this one (it's not super-clear).

>>+  toString: function() {
>>+    return JSON.stringify({ type: this.type, stream: this._stream });
>>+  }
>
>Why?  Is this just for our own internal debugging purposes?  Content will never
>see this bit as things stand.

An XPIDL pattern I copied. No longer need for serializers I suppose (they did show up in DumpJSStack() though. Neat). Deleting.

>>+  set onaddstream(handler)    { this.addEventListener("addstream", handler); },
>>+  set onicecandidate(handler) { this.addEventListener("icecandidate", handler); },
>
> That looks wrong.  First of all, you need getters, not just setters.  Second,
> doing |foo.onaddstream = null| is supposed to _remove_ the event listener.  Then
> there's the false return needing to preventDefault bit, etc, etc.
>
> I'll look into setting something up for you here that doesn't suck on the C++ side,
> but if that fails I'll describe what the js here should be doing....

Will fix based on Bug 870219. Thanks!

>>   addIceCandidate: function(cand, onSuccess, onError) {
>>+    if (!cand || (!cand.candidate && !cand.sdpMLineIndex)) {
>
>cand can't test calse here, as far as I can tell.

Deleting.

> Also, this changed the logic: before we threw if either of cand.candidate
> and cand.sdpMLineIndex tested false; now we only throw if both test false.
> Why the change?

A fixed I picked up from abr. Logic was incorrect before; Don't need both.

>>+++ b/dom/webidl/MediaStreamEvent.webidl
>
>It's a good idea to put the spec link in a comment after the license.

Willdo.

>>+++ b/dom/webidl/RTCConfiguration.webidl
>>+dictionary mozRTCIceServer {
>
>Why the renaming?  Just because it doesn't quite match what the spec has?

Webkit does as well - http://src.chromium.org/multivm/trunk/webkit/Source/Platform/chromium/public/WebRTCConfiguration.h

>>+++ b/dom/webidl/RTCConstraints.webidl
>>+  boolean? OfferToReceiveAudio;
>
>So that allows four possible values for OfferToReceiveAudio: not set, null,
>false, true.  Why do we explicitly allow the "null" value here?

Sorry, that webidl is unused in the end. Constraints don't map to webidl well. I'll remove it.

> Someone who actually understands this stuff should review the code actually
> examining these objects, with an understanding that nothing guarantees that
> isObject(constraints.optional[i]) will return the same thing when called twice
> for the same i and that sort of thing.

This patch does not alter that parsing code. Is it less safe on account of webidl vs. xpidl?

> Also, I can't find the IDL that uses this dictionary.  Is there just a file
> missing, or is it just unused?

Unused. My bad.

>>+++ b/dom/webidl/RTCPeerConnection.webidl
>
>>+enum mozRTCSignalingState {
>>+enum mozRTCIceGatheringState {
>>+enum mozRTCIceConnectionState {
>
>Why the "moz"?

I thought we were supposed to. Or is it just for mozRTCPeerConnection? Randell?

>>+interface mozRTCPeerConnection : EventTarget  {
>
>Why mozRTCPeerConnection instead of RTCPeerConnection?  That seems wrong, since
>the interface name is what the constructor ends up as....  Unless we're shipping
>this prefixed for some reason?

It's called mozRTCPeerConnection in Firefox and webkitRTCPeerConnection in Chrome. This path doesn't change that. I'm not sure what the convention for this in webidl is.

> This generally applies to all the other constructible interfaces in this patch.

Fair question about the new interfaces and dictionaries exposed.

>>+  void createOffer (mozRTCSessionDescriptionCallback successCallback,
>>+                    mozRTCPeerConnectionErrorCallback? failureCallback, // apprtc
>>+                    optional object? constraints);
>
>This seems like something that will end up as a Future in the future...  I
>guess that can be done via overloads.  :(

> In the spec failureCallback is not nullable.  Why is it here and in createAnswer?

Because Google's webrtc reference app http://apprtc.appspot.com wont run otherwise.

>>+  attribute EventHandler onnegotiationneeded;
>>+  attribute EventHandler onicecandidate;
>>+  attribute EventHandler onsignalingstatechange;
>>+  attribute EventHandler onaddstream;
>>+  attribute EventHandler onremovestream;
>>+  attribute EventHandler oniceconnectionstatechange;
>
>I don't see any provisions for making some of these work.  We need to either do
>that or remove the properties; right now it looks like all except onaddstream
>and onicecandidate will let you set and get them but not actually get you any
>events, right?  Is that expected?

Yes, I have a part 2 patch that addresses that. I'll upload it as a separate patch here.

>>+  // Four backwards-compatible attributes (to pass mochitests)
>>+  attribute mozRTCSessionDescription localDescription;
>>+  attribute mozRTCSessionDescription remoteDescription;
>
>Then take out the commented bits above?  Or at least document above why these
>need to be writable?
I worry someone removing the deprecated lines later would forget to add back the readonly ones.

> But they were not writable in the XPIDL, so why writable here?

XPIDL actually generated an implicit set call I believe (!) - But regardless, I'm deleting it with further testing I found it to be unused.

>>+  readonly attribute object localStreams;
>>+  readonly attribute object remoteStreams;
>
>If these are not part of the spec, just put them on a partial interface
>documented to be a Mozilla extension?

Didn't know about that. Willdo.

>r=me with the above fixed, but I'd love to see an interdiff addressing the comments

Thanks, willdo!
(Assignee)

Comment 30

4 years ago
> Mochitest updates look okay with one question inline. Btw, for any bugs you caught
> that was not caught by the mochitests, can you indicate which areas specifically?

 - onaddstream and onicecandidate callbacks/event-handlers.
 - The various state event-handlers (which are changing here).
 - DataChannels

Also, apprtc is non-conformant:
 - it calls setLocal/RemoteDescription without callbacks.
 - it calls createOffer/Answer without failure callback.
 - it calls setLocal/RemoteDescription with a dictionary as the description
   argument (I need a judgement call on whether this should be allowed or not)

> I'd like to get bugs on file to track the automation weaknesses so we can look
> into this in the future.

Good idea.

>::: dom/media/tests/mochitest/test_peerConnection_bug825703.html
>@@ +41,5 @@
>>      exception = null;
>>  
>>      makePC(1, false);
>>  
>> +    makePC({}, true);
>
>What's the reasoning behind this now being true and the associated tests being
>removed?

Webidl now normalizes input for us, replacing the manual structural validation we did by hand before, and it acts slightly differently. Specifically, it doesn't appear to throw on type errors in dictionaries.
For enums and dictionaries, there's no need to prefix; these names are not exposed to the web.

(Our current policy doesn't use prefixes at all, but I guess this has sailed...)
(Assignee)

Comment 32

4 years ago
Makes sense. I'll remove the moz for all enums and dictionaries then at least.
Comment on attachment 746705 [details] [diff] [review]
Rewrite PeerConnection in JS-implemented WebIDL (4)

>+  nsDOMEvent(nsPIDOMWindow* aWindow);
>   virtual ~nsDOMEvent();
>+private:
>+  void init(mozilla::dom::EventTarget* aOwner, nsPresContext* aPresContext,
>+            nsEvent* aEvent);
Init, not init
Comment on attachment 746705 [details] [diff] [review]
Rewrite PeerConnection in JS-implemented WebIDL (4)

>+[Pref="media.peerconnection.enabled",
>+ JSImplementation="@mozilla.org/dom/mediastreamevent;1",
>+ Constructor(DOMString type, optional mozMediaStreamEventInit eventInitDict)]
>+interface mozMediaStreamEvent : Event {
>+  readonly attribute MediaStream? stream;

>+dictionary mozRTCPeerConnectionIceEventInit : EventInit {
>+  mozRTCIceCandidate? candidate;
>+};
>+
>+[Pref="media.peerconnection.enabled",
>+ JSImplementation="@mozilla.org/dom/rtcpeerconnectioniceevent;1",
>+ Constructor(DOMString type,
>+             optional mozRTCPeerConnectionIceEventInit eventInitDict)]
>+interface mozRTCPeerConnectionIceEvent : Event {
>+  readonly attribute mozRTCIceCandidate? candidate;
>+};
ditto
Er, what did bugzilla do.
Anyhow,  Both those events are in the spec, so why prefix?
> Actually, thanks to webidl it'll never be undefined. Deleting! :-)

Well.  In your _constructor_ it will be, since you're being created via createInstance, right?

> Or would an even simpler pattern be to use defaults in webidl?

If you have the option, yes!

> I'll delete the redundancy since constructors never have args.

At least not when called from content, right.

> I misinterpreted
> http://dev.w3.org/2011/webrtc/editor/webrtc.html#mediastreamevent

Actually, the relevant spec is at http://dom.spec.whatwg.org/#constructing-events (note the bit about "or of an interface that inherits from the Event interface".  Yes, it would be nice if specs explicitly linked to this bit when defining event constructors instead of the reader having to know about the "come from" semantics of the specs... ;)

> Is it less safe on account of webidl vs. xpidl?

No.  Just not sure whether the code was ever carefully audited before either.  ;)

> This path doesn't change that.

Oh, interesting.  So current trunk has a window.RTCPeerConnection that throws when called and a window.mozRTCPeerConnection that constructs a new peerconnection.  That's broken if people use object-detection.

I assume your patch fixes that problem; we should add a test for it.

I do recall now that we wanted to prefix RTCPeerConnection for now because we want to ship this stuff while the spec is still far from anything resembling stable...  OK.  Please sort out with jesup which interfaces actually need prefixing here; maybe all for consistency.  :(

> Because Google's webrtc reference app http://apprtc.appspot.com wont run
> otherwise.

Awesome.  Are they passing in null or undefined?

In any case, sounds like a spec issue that needs raising.  

> I worry someone removing the deprecated lines later would forget to add back
> the readonly ones.

Add comments explaining all that!  I have no problem with commented out stuff in there but it needs to be documented _why_ it's commented out.  And the deprecated line bits should have a "if you remove this, uncomment this other thing" comment.
(Assignee)

Comment 37

4 years ago
jesup says no need to prefix beyond top-level apis (RTCPeerConnection, getUserMedia, etc) "unless our implementation is at odds with others".
(Assignee)

Comment 38

4 years ago
>> Because Google's webrtc reference app http://apprtc.appspot.com wont run
>> otherwise.
>
> Awesome.  Are they passing in null or undefined?

They're omitting the last two args.
Then don't you want an optional argument rather than a nullable one?  A non-optional nullable argument, as in the IDL here, doesn't allow the arg to be omitted...
(Assignee)

Comment 40

4 years ago
Huh, good point. Either apprtc fixes stuff real fast or I was must have been confused. I swear I saw "TypeError: Not enough arguments to mozRTCPeerConnection.createOffer." somewhere. Seems to no longer be needed. Deleting.

Thanks for pointing that out!
Comment on attachment 746705 [details] [diff] [review]
Rewrite PeerConnection in JS-implemented WebIDL (4)

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

I looked over this a bit, but I don't really understand enough to qualify it as a review.
Attachment #746705 - Flags: review?(continuation) → feedback+
(Assignee)

Comment 42

4 years ago
Created attachment 747616 [details] [diff] [review]
Rewrite PeerConnection in JS-implemented WebIDL (5) r=bz

Integrated all feedback. Carrying forward r+ from bz.

- Many small corrections so feel free to look over again.
- Removed all new 'moz' prefixes.
- Less arg parsing(relies on webidl invariants)
- Correct EventHandler args behavior (in bug 870219, not landed atow)
Attachment #746705 - Attachment is obsolete: true
Attachment #747616 - Flags: review?(rjesup)
Comment on attachment 747616 [details] [diff] [review]
Rewrite PeerConnection in JS-implemented WebIDL (5) r=bz

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

r+ to the level that I follow the WebIDL bits

::: dom/media/PeerConnection.js
@@ +454,5 @@
> +  get onaddstream()    { return this.getEventHandler("onaddstream"); },
> +  get onicecandidate() { return this.getEventHandler("onicecandidate"); },
> +
> +  set onaddstream(handler) { this.setEventHandler("onaddstream", handler); },
> +  set onicecandidate(h)    { this.setEventHandler("onicecandidate", h); },

be consistent about it taking a handler, don't use handler for one and h for the other.

@@ +627,5 @@
>      this._checkClosed();
>      return this._getPC().remoteStreams;
>    },
>  
> +  // Four backwards-compatible attributes (to pass mochitests)

Please file a bug and note the number here for updating the mochitests and removing these; we may not be able to do so for a little while if outside people are depending on them; discuss in the bug.
Attachment #747616 - Flags: review?(rjesup) → review+
(Assignee)

Comment 44

4 years ago
Created attachment 747708 [details] [diff] [review]
Part 2: Update PeerConnection to spec. States + DataChannel

This second and last patch adds:

 - DataChannel
 - States fired on EventHandlers updated to spec (as best as currently possible).

Try - https://tbpl.mozilla.org/?tree=Try&rev=b605590db4cc

Testing appreciated! Especially remote connections.
Attachment #747708 - Flags: review?(rjesup)
Attachment #747708 - Flags: review?(jsmith)
Attachment #747708 - Flags: review?(bzbarsky)
Attachment #747708 - Flags: review?(adam)
(Assignee)

Comment 45

4 years ago
> Please file a bug and note the number here for updating the mochitests and
> removing these; we may not be able to do so for a little while if outside
> people are depending on them; discuss in the bug.

Will do. We also discussed perhaps emitting deprecation warnings to web console for the deprecated methods.
The part 2 is on top of a part 1 with my review comments largely addressed, right?  Was there an interdiff for that bit?
Comment on attachment 747708 [details] [diff] [review]
Part 2: Update PeerConnection to spec. States + DataChannel

>+    this.__DOM_IMPL__.initEvent(type, dict.bubbles || false,
>+                                dict.cancelable || false);

"bubbles" and "cancelable" have default values.  So you can just pass them in directly without the || bits.

I don't seen an implementation of createDataChannel here.  Did it already exist?

r=me I guess...
Attachment #747708 - Flags: review?(bzbarsky) → review+
Comment on attachment 747708 [details] [diff] [review]
Part 2: Update PeerConnection to spec. States + DataChannel

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

r+, but please also run the datachannel tests on webrtc-landing, as the mochitests still aren't covering datachannels.
I'd like to see deprecated attributes/etc warn the JS app dev in some manner

::: dom/media/PeerConnection.js
@@ +677,2 @@
>    get localStreams() { return this.getLocalStreams(); },
>    get remoteStreams() { return this.getRemoteStreams(); },

deprecated things should put warnings in the error console

@@ +718,5 @@
> +  },
> +
> +  // Deprecated
> +  changedIceState: function() {
> +    this.dispatchEvent(new this._win.Event("icechange"));

deprecated things should put warnings in the error console
Attachment #747708 - Flags: review?(rjesup) → review+
(Assignee)

Comment 49

4 years ago
> The part 2 is on top of a part 1 with my review comments largely addressed,
> right?

Right. New part one is in comment 42, Attachment 747616 [details] [diff]. I carried forward your r+.
Feel free to look over!

> Was there an interdiff for that bit?

I assumed bugzilla's interdiff under "diff" would do. https://bugzilla.mozilla.org/attachment.cgi?oldid=746705&action=interdiff&newid=747616&headers=1

I see I was wrong. :-( I get "Warning: interdiff encountered errors while comparing these patches." - Is this known?
> Feel free to look over!

The point is, I'd like to look over just the changes, not the whole thing all over again....

> I assumed bugzilla's interdiff under "diff" would do.

It almost never does, especially when context changes, because of how interdiff has to work.  Hence the warning you see.

The right way to produce an interdiff is to diff inside a VCS between "tree with first patch applied" and "tree with second patch applied".

I'd do that myself, if I had to but these patches are being applied on top of other stuff and I don't know which other stuff.
(Assignee)

Comment 51

4 years ago
Created attachment 747985 [details] [diff] [review]
Bug 823512: Interdiff between versions 4 and 5 of webidl_peerconnection.patch

Here is the interdiff for the changes in comment 42 (Thanks bz and jesup for explaining how).
Flags: needinfo?(bzbarsky)
Comment on attachment 747985 [details] [diff] [review]
Bug 823512: Interdiff between versions 4 and 5 of webidl_peerconnection.patch

Thanks!

+    this.__DOM_IMPL__.initEvent(type, dict.bubbles || false,
+                                dict.cancelable || false);

Like in the other patch, no need for the "|| false" bit.

>+    this._candidate = dict.candidate || null;

Why not default to null in the dictionary?

The rest looks good.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 53

4 years ago
From Comment 48:
>>> Because Google's webrtc reference app http://apprtc.appspot.com wont run
>>> otherwise.
>>
>> Awesome.  Are they passing in null or undefined?
>
>They're omitting the last two args.

Sorry, I misremembered. They pass in null. From http://apprtc.appspot.com :

  pc.createAnswer(setLocalAndSendMessage, null, sdpConstraints);

'?' going back in.
> '?' going back in.

Sounds good.  Make sure to raise a spec issue!
(Assignee)

Updated

4 years ago
Attachment #747708 - Flags: review?(adam) → review?(ekr)
(Assignee)

Comment 55

4 years ago
Created attachment 748293 [details] [diff] [review]
Rewrite PeerConnection in JS-implemented WebIDL (6) r=bz, rjesup

Updated nits. Carrying forward r+ from bz and rjesup.
Attachment #747616 - Attachment is obsolete: true
Attachment #748293 - Flags: review+
(Assignee)

Comment 56

4 years ago
Created attachment 748294 [details] [diff] [review]
Part 2: Update PeerConnection to spec. States + DataChannel (2), r=bz,rjesup

Updated with nits. Carrying forward r+ from bz and rjesup.
Attachment #747708 - Attachment is obsolete: true
Attachment #747708 - Flags: review?(jsmith)
Attachment #747708 - Flags: review?(ekr)
Attachment #748294 - Flags: review?(ekr)
Comment on attachment 748294 [details] [diff] [review]
Part 2: Update PeerConnection to spec. States + DataChannel (2), r=bz,rjesup

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

This seems to need cleanup.
Attachment #748294 - Flags: review?(ekr) → review-
(Assignee)

Comment 58

4 years ago
Can you be more specific - just formatting or are the states wrong?
(Assignee)

Comment 59

4 years ago
Created attachment 748384 [details] [diff] [review]
Rewrite PeerConnection in JS-implemented WebIDL (7) r=bz, rjesup

Includes codegen tweak from bz to compile on Android.

Carrying forward r+ from bz and rjesup.
Attachment #748293 - Attachment is obsolete: true
Attachment #748384 - Flags: review+
Comment on attachment 748294 [details] [diff] [review]
Part 2: Update PeerConnection to spec. States + DataChannel (2), r=bz,rjesup

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

::: dom/media/PeerConnection.js
@@ +213,5 @@
>    __init: function(type, dict) {
>      this.type = type;
>      this.__DOM_IMPL__.initEvent(type, dict.bubbles || false,
>                                  dict.cancelable || false);
> +    this._candidate = dict.candidate;

Why was this change needed? Can we we guarantee that null is the only falsy value that this._candidate might take on?

@@ +472,5 @@
> +  get onremovestream()         { return this.getEH("onremovestream"); },
> +  get ondatachannel()          { return this.getEH("ondatachannel"); },
> +  get onconnection()           { return this.getEH("onconnection"); },
> +  get onclosedconnection()     { return this.getEH("onclosedconnection"); },
> +  get oniceconnectionstatechange() { return this.getEH("oniceconnectionstatechange"); },

I would try to rewrite these to remove cut-and-paste. Perhaps something like this (untested) code:

function makeGetterSetter(name) {
   Object.defineProperty(this, name,
                         {
                           get:function() {
                             return getEH(name);
                           },
                           set:function(h) {
                             return setEH(name, h);
                           }
                         }

Then you can do

makeGetterSetter("onaddstream");

@@ +493,5 @@
> +
> +  deprecated: function() {
> +    let caller = Error().stack.split("\n")[1].split("@")[0];
> +    // caller ~= "PeerConnectionObserver.prototype.onicechange"
> +    dump(caller.split(".")[2] + " is deprecated!\n");

What happens if these splits don't produce the expected result? Maybe that's a can't happen but I want to make sure it doesn't do something awful.

@@ +577,1 @@
>        default:

These changes should occur on callback completion, IMO.

@@ +606,3 @@
>          break;
> +      case "pranswer":
> +        throw new Components.Exception("pranswer not yet implemented",

Theee too.

@@ +613,2 @@
>      }
> +    this.changeSignalingState("have-remote-offer");

Doesn't this override the stable above?

@@ +708,5 @@
> +  get iceConnectionState() { return this._iceConnectionState; },
> +
> +  changeSignalingState: function(state) {
> +    this._signalingState = state;
> +    this.dispatchEvent(new this._win.Event("signalingstatechange"));

Are these events dispatched asynchronously? I.e., say I do:

pc.onsignalingstatechange(function() { console.log("State change";});

pc.setRemote(...);
console.log("Set remote");

What is the output?

@@ +718,5 @@
> +    this.dispatchEvent(new this._win.Event("gatheringchange"));
> +  },
> +
> +  changeIceConnectionState: function(state) {
> +    this._iceConnectionState = state;

Suggest having this call changedIceState() to avoid duplication below.

@@ +967,5 @@
> +            break;
> +          case Ci.IPeerConnection.kIceWaiting:
> +            this._dompc.changedIceState();
> +            // Now that the PC is ready to go, execute any pending operations.
> +            this._dompc._executeNext();

Let's put this first since it's the first state change.

@@ +973,5 @@
> +          case Ci.IPeerConnection.kIceConnected:
> +            // ICE gathering complete.
> +            this._dompc.changeIceConnectionState("connected");
> +            this._dompc.changeIceGatheringState("complete");
> +            this._dompc.changedIceState();

These don't seem right. IceGatheringState("complete") means gathering is complete, not that the handshake is done.
> Can we we guarantee that null is the only falsy value that this._candidate might take on?

It's declared in the IDL as:

  mozRTCIceCandidate? candidate;

So if it's coming through the bindings it will be either null or a mozRTCIceCandidate object.
> Are these events dispatched asynchronously?

No, dispatchEvent is synchronous.
(Assignee)

Comment 63

4 years ago
Created attachment 748447 [details] [diff] [review]
Part 3: Deprecated API calls emit warnings to web-console.

Adds deprecation warnings. I added it as a third patch to ease review.

Bz, if you could look it over that'd be great. It reuses the existing reportError mechanism. Thanks!
Attachment #747985 - Attachment is obsolete: true
Attachment #748447 - Flags: review?(rjesup)
Attachment #748447 - Flags: review?(bzbarsky)
Comment on attachment 748447 [details] [diff] [review]
Part 3: Deprecated API calls emit warnings to web-console.

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

r+ from my perspective, but I don't know the error stuff that well
Attachment #748447 - Flags: review?(rjesup) → review+
(Assignee)

Comment 65

4 years ago
>> Can we we guarantee that null is the only falsy value that this._candidate
>> might take on?
>
>It's declared in the IDL as:
>
>  mozRTCIceCandidate? candidate;
>
>So if it's coming through the bindings it will be either null or a
>mozRTCIceCandidate object.

Wait, I think that's a valid point, bz from what you said earlier, doesn't it need to say:

  mozRTCIceCandidate? candidate = null;

I'll add that.

> I would try to rewrite these to remove cut-and-paste. Perhaps something
> like this (untested) code:

Good idea. Thank for the template!

>> +
>> +  deprecated: function() {
>> +    let caller = Error().stack.split("\n")[1].split("@")[0];
>> +    // caller ~= "PeerConnectionObserver.prototype.onicechange"
>> +    dump(caller.split(".")[2] + " is deprecated!\n");
>
>What happens if these splits don't produce the expected result? Maybe that's
>a can't happen but I want to make sure it doesn't do something awful.

I think it's a can't happen, since the Error() stack is predictable from the few
places that calls this, and I only look one level up, so it's all inside peerconnection.js. Even if something did happen (which it can't, I believe) the worst that would happen, since this is JS, is that the output would be wrong.

>@@ +577,1 @@
>>        default:
>
>These changes should occur on callback completion, IMO.

Not sure I agree. I posted this question to public-webrtc@w3.org as you suggested.
Since these are rather new, both to firefox and the spec, there should be room to tweak these later if need be.

> Theee too.

?

>> +    this.changeSignalingState("have-remote-offer");
>
>Doesn't this override the stable above?

Whoops! cut'n'paste bug. Good catch, thanks!

>> +  changeIceConnectionState: function(state) {
>> +    this._iceConnectionState = state;
>
>Suggest having this call changedIceState() to avoid duplication below.

Good idea. Will do, though we'd still need to call it from kIceWaiting, which apparently is not a state in the new model.

>> +          case Ci.IPeerConnection.kIceConnected:
>> +            // ICE gathering complete.
>> +            this._dompc.changeIceConnectionState("connected");
>> +            this._dompc.changeIceGatheringState("complete");
>> +            this._dompc.changedIceState();
>
>These don't seem right. IceGatheringState("complete") means gathering is
>complete, not that the handshake is done.

You sure? It's the same as before, from http://mxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js#933 :

 930       case Ci.IPeerConnection.kIceConnected:
 931         // ICE gathering complete.
 932         this.callCB(this._dompc.onicechange, "connected");
 933         this.callCB(this._dompc.ongatheringchange, "complete");

so maybe we can file a separate bug on that, unless you know off hand how it should behave?
(Assignee)

Comment 66

4 years ago
Created attachment 748459 [details] [diff] [review]
Part 2: Update PeerConnection to spec. States + DataChannel (3), r=bz,rjesup

Integrated feedback from ekr.

- Except I couldn't get makeGetterSetter() to work (it compiled but didn't
  run ok). I think the get: set: properties need to get named, no?

Perhaps we can file a bug to optimize this one part later?

Carrying forward r+ from bz and rjesup.
Attachment #748294 - Attachment is obsolete: true
Attachment #748459 - Flags: review?(ekr)
(Assignee)

Comment 67

4 years ago
These 3 patches should work on m-c now (and m-i too I hope).

I have to run out and buy a mother's day present. If someone can get logs or figure out why these patches don't work with
 - http://apprtc.appspot.com
 - http://webrtcme.herokuapp.com/r/jib
 - http://gupshup.herokuapp.com

that would be awesome!
>  mozRTCIceCandidate? candidate = null;

Er, yes, you do need that.  Though undefined would get coerced to null when going back through webidl anyway, fwiw, since the getter for "candidate" on the interface returns a "mozRTCIceCandidate?".
(In reply to Jan-Ivar Bruaroey [:jib] from comment #66)
> Created attachment 748459 [details] [diff] [review]
> Part 2: Update PeerConnection to spec. States + DataChannel (3), r=bz,rjesup
> 
> Integrated feedback from ekr.
> 
> - Except I couldn't get makeGetterSetter() to work (it compiled but didn't
>   run ok). I think the get: set: properties need to get named, no?
> 
> Perhaps we can file a bug to optimize this one part later?


Seems like it would be better to figure out why it doesn't work.

bz?


> Carrying forward r+ from bz and rjesup.
(Assignee)

Comment 70

4 years ago
Created attachment 748495 [details] [diff] [review]
Part 2: Update PeerConnection to spec. States + DataChannel (4), r=bz,rjesup

Figured out makeGetterSetter(). Was missing a this. reference. 

Carrying forward r+ from bz and rjesup.
Attachment #748459 - Attachment is obsolete: true
Attachment #748459 - Flags: review?(ekr)
Attachment #748495 - Flags: review?(ekr)
(Assignee)

Comment 71

4 years ago
Created attachment 748496 [details] [diff] [review]
Part 3: Deprecated API calls emit warnings to web-console. (2) r=rjesup

Updated to match part 2.
Attachment #748447 - Attachment is obsolete: true
Attachment #748447 - Flags: review?(bzbarsky)
Attachment #748496 - Flags: review?(bzbarsky)
(Assignee)

Comment 72

4 years ago
Created attachment 748497 [details] [diff] [review]
Part 1: Rewrite PeerConnection in JS-implemented WebIDL (8) r=bz, rjesup

Fixed missing moz-prefix in mochitest/test_peerConnection_bug840344.html

Carrying forward r+ from bz and rjesup.

Try with all three patches - https://tbpl.mozilla.org/?tree=Try&rev=9c6d2e0720ce
Attachment #748384 - Attachment is obsolete: true
Attachment #748497 - Flags: review+
> Seems like it would be better to figure out why it doesn't work.

Can I see the non-working code?  Is the defineProperty being done on the right object?
(Assignee)

Comment 74

4 years ago
No worries. I figured it out in comment 70.
Comment on attachment 748495 [details] [diff] [review]
Part 2: Update PeerConnection to spec. States + DataChannel (4), r=bz,rjesup

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

Some problems still remain.

Also, does this actually work? c67 is kind of concerning

::: dom/media/PeerConnection.js
@@ +247,5 @@
>     */
>    this._pending = false;
> +  this._iceGatheringState = this._iceConnectionState = "new";
> +  this._signalingState = "stable";
> + }catch(e){dump(e.fileName + ", line " + e.lineNumber + ":\n" + e + "\n");}

Why this change? What can go wrong here?

@@ +582,4 @@
>          break;
> +      case "pranswer":
> +        throw new Components.Exception("pranswer not yet implemented",
> +                                       Cr.NS_ERROR_NOT_IMPLEMENTED);

My previous comments about the timing of this state change apply here.

@@ +615,4 @@
>          break;
> +      case "pranswer":
> +        throw new Components.Exception("pranswer not yet implemented",
> +                                       Cr.NS_ERROR_NOT_IMPLEMENTED);

And here.
Attachment #748495 - Flags: review?(ekr) → review-
(Assignee)

Comment 76

4 years ago
> Also, does this actually work? c67 is kind of concerning

I have yet to complete a remote call (tried various setups). Apps sit without clear error messages (not clear to me anyway). Mochis provide insufficient coverage. More debugging needed.
Following a promising error on Windows-only.

There's also a crashtest failure in https://tbpl.mozilla.org/?tree=Try&rev=9c6d2e0720ce (reftest timeout).

>> + }catch(e){dump(e.fileName + ", line " + e.lineNumber + ":\n" + e + "\n");}
>
>Why this change? What can go wrong here?

Debug junk, sorry. Will remove.

> My previous comments about the timing of this state change apply here.

I tried posting to the list yesterday, but it being my first post there, I suspect it got jammed for 1-2 business days. :-(

Moving them to callback completion sounds doable, so I'll follow your judgement.
Comment on attachment 748496 [details] [diff] [review]
Part 3: Deprecated API calls emit warnings to web-console. (2) r=rjesup

r=me
Attachment #748496 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 78

4 years ago
Created attachment 749579 [details] [diff] [review]
Part 2: Update PeerConnection to spec. States + DataChannel (5), r=bz,rjesup

Updated with feedback from ekr, and backed out some changes.

- Deprecated callbacks are callbacks again rather than event handlers, since
  their whole point is to handle legacy behavior (with string argument).
- Removed deprecated "statechange" event because we never supported it.
- Made local/remoteDescription attributes nullable in api to match behavior.
- Deprecation warnings in more places.
- Removed attempt at implementing signalingState, as bug 784519 will address it.
- Retired stale dom/media/tests/crashtests/837421.html

Since this mostly removes code, I'm carrying forward r+ from bz and rjesup, but feel free to look it over again.
Attachment #748495 - Attachment is obsolete: true
Attachment #749579 - Flags: review?(ekr)
(Assignee)

Comment 79

4 years ago
Created attachment 749583 [details] [diff] [review]
Part 4: Workaround for lack of serializer support (bug 863402).

Fixes remote calls (appRtc and heroku now work).

-  Problem was lack of serializer in RTCSessionDescription, so apps that used
   JSON.Stringify(description.sdp); worked, but those that used
   JSON.Stringify(description); did not.

This should be it!

(I'll add a try link as soon as the try server reopens).
Attachment #749583 - Flags: review?(rjesup)
Attachment #749583 - Flags: review?(bzbarsky)
(Assignee)

Comment 80

4 years ago
That was quick. Try - https://tbpl.mozilla.org/?tree=Try&rev=a27707d7ff6d
Comment on attachment 749583 [details] [diff] [review]
Part 4: Workaround for lack of serializer support (bug 863402).

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

This one is for bz...
Attachment #749583 - Flags: review?(rjesup)
Comment on attachment 749583 [details] [diff] [review]
Part 4: Workaround for lack of serializer support (bug 863402).

>+++ b/dom/bindings/parser/WebIDL.py

Please add a comment there that toJSON should be in the list too, with the bug# of the followup bug on restoring it to that list.  Please cc me on that bug.+  

>object _toJSON();

Do you still need the leading underscore?  If not, remove it, please.

r=me with that
Attachment #749583 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 83

4 years ago
Created attachment 749655 [details] [diff] [review]
Part 4: Workaround for lack of serializer support, bug 863402 (2). r=bz

Update with feedback. Carrying forward r+ from bz.
Attachment #749583 - Attachment is obsolete: true
Attachment #749655 - Flags: review+
(Assignee)

Comment 84

4 years ago
Created attachment 749663 [details] [diff] [review]
Interdiff for part 2 (4)->(5)
(Assignee)

Comment 85

4 years ago
Created attachment 749666 [details] [diff] [review]
Interdiff for part 2 (4)->(5)
Attachment #749663 - Attachment is obsolete: true
(Assignee)

Comment 86

4 years ago
Heads up that, as of this patch, the following stops working:

   var answer = { type:"answer", sdp:"v=0\r\no=Mozi..." };
   pc.setRemoteDescription(JSON.parse(answer), success, failed);

The correct API (which already works) is:

   pc.setRemoteDescription(new mozRTCSessionDescription(JSON.parse(answer)),
                           success, failed);

Randell just fixed http://nightly-gupshup.herokuapp.com/chat in this regard.

Typically, setLocalDescription is fine since createOffer returns the right object.
(Assignee)

Updated

4 years ago
Attachment #749579 - Flags: review?(ekr) → review?(adam)
(Reporter)

Updated

4 years ago
Duplicate of this bug: 842531
(Assignee)

Updated

4 years ago
Attachment #749579 - Flags: review?(adam) → review?(ekr)
Comment on attachment 749666 [details] [diff] [review]
Interdiff for part 2 (4)->(5)

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

I think the states are still wrong.

::: dom/media/PeerConnection.js
@@ +960,5 @@
> +    }
> +
> +    switch (this._dompc._pc.iceState) {
> +      case Ci.IPeerConnection.kIceWaiting:
> +        this.callCB(this._onicechange, "starting");

Shouldn't this change the gathering state to completed?

@@ +976,5 @@
> +      case Ci.IPeerConnection.kIceConnected:
> +        // ICE gathering complete.
> +        this._dompc.changeIceConnectionState("connected");
> +        this.callCB(this._onicechange, "connected");
> +        this.callCB(this._dompc.ongatheringchange, "complete");

This shouldn't be here.

::: dom/webidl/RTCPeerConnection.webidl
@@ +65,5 @@
>    void setRemoteDescription (mozRTCSessionDescription description,
>                               optional VoidFunction successCallback,
>                               optional RTCPeerConnectionErrorCallback failureCallback);
> +  readonly attribute mozRTCSessionDescription? localDescription;
> +  readonly attribute mozRTCSessionDescription? remoteDescription;

What does this ? do?
Attachment #749666 - Flags: review-
(In reply to Eric Rescorla (:ekr) from comment #88)
> ::: dom/webidl/RTCPeerConnection.webidl
> @@ +65,5 @@
> >    void setRemoteDescription (mozRTCSessionDescription description,
> >                               optional VoidFunction successCallback,
> >                               optional RTCPeerConnectionErrorCallback failureCallback);
> > +  readonly attribute mozRTCSessionDescription? localDescription;
> > +  readonly attribute mozRTCSessionDescription? remoteDescription;
> 
> What does this ? do?

It means that the attribute can be either a mozRTCSessionDescription object or null. See <http://dev.w3.org/2006/webapi/WebIDL/>, <https://developer.mozilla.org/en/Mozilla/WebIDL_bindings> for more information about WebIDL.
(Assignee)

Comment 90

4 years ago
>> +      case Ci.IPeerConnection.kIceWaiting:
>> +        this.callCB(this._onicechange, "starting");
>
> Shouldn't this change the gathering state to completed?

Agreed from the code you showed me on irc: https://hg.mozilla.org/mozilla-central/file/da429c311864/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#l1359 - Will move it up as you show.

> What does this ? do?

As Ms2ger said, and the reason is to allow the code to return null, which it already does here http://mxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js#644 . It's number 6 from my post to the list:

#6 - Proposal: I notice local/remoteDescription are not marked as nullable ('?') even though "A null object will be returned if the local/remote description has not yet been set." I think the wording makes sense and the webidl is wrong, so I propose this "null means unset" change:

- readonly attribute RTCSessionDescription localDescription;
- readonly attribute RTCSessionDescription remoteDescription;
+ readonly attribute RTCSessionDescription? localDescription;
+ readonly attribute RTCSessionDescription? remoteDescription;

Will upload update asap.
(Assignee)

Comment 91

4 years ago
Created attachment 750635 [details] [diff] [review]
Part 2: Update PeerConnection to spec. States + DataChannel (6), r=bz,rjesup
Attachment #750635 - Flags: review?(ekr)
(Assignee)

Updated

4 years ago
Attachment #749666 - Attachment is obsolete: true
Attachment #749666 - Flags: review-
(Assignee)

Comment 92

4 years ago
Created attachment 750637 [details] [diff] [review]
Interdiff for part 2 (6)->(6)

Hit return too quick. Here's a new interdiff.
Attachment #749579 - Attachment is obsolete: true
Attachment #749579 - Flags: review?(ekr)
(Assignee)

Comment 93

4 years ago
Created attachment 750640 [details] [diff] [review]
Interdiff for part 2 (5)->(6)

Just a typo.
Attachment #750637 - Attachment is obsolete: true
Comment on attachment 750635 [details] [diff] [review]
Part 2: Update PeerConnection to spec. States + DataChannel (6), r=bz,rjesup

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

lgtm with changes below.

::: dom/media/PeerConnection.js
@@ +661,5 @@
>      });
>    },
>  
>    removeStream: function(stream) {
>       //Bug844295: Not implemeting this functionality.

Spelling eeror.

@@ +684,5 @@
>  
>    getRemoteStreams: function() {
>      this._checkClosed();
>      return this._getPC().remoteStreams;
>    },

Can we get stubs for the API points that are unimplemented, such as getStreamById().
Attachment #750635 - Flags: review?(ekr) → review+
(Assignee)

Comment 95

4 years ago
Created attachment 750799 [details] [diff] [review]
Part 2: Update PeerConnection to spec. States + DataChannel (7), r=bz,rjesup,ekr

Updated nits. Carrying forward r+ from bj, rjesup and ekr.

- Spelling error + Added stub for getStreamById().
Attachment #750635 - Attachment is obsolete: true
Attachment #750799 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #750640 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #748497 - Flags: checkin?(rjesup)
(Assignee)

Updated

4 years ago
Attachment #750799 - Flags: checkin?(rjesup)
(Assignee)

Updated

4 years ago
Attachment #748496 - Flags: checkin?(rjesup)
(Assignee)

Updated

4 years ago
Attachment #749655 - Flags: checkin?(rjesup)
Depends on: 872856
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e02515f7a1d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0221d61cef72
https://hg.mozilla.org/integration/mozilla-inbound/rev/b115d0dbc1d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f60fe3eefc5
Target Milestone: --- → mozilla24

Updated

4 years ago
Attachment #748496 - Flags: checkin?(rjesup) → checkin+

Updated

4 years ago
Attachment #748497 - Flags: checkin?(rjesup) → checkin+

Updated

4 years ago
Attachment #749655 - Flags: checkin?(rjesup) → checkin+

Updated

4 years ago
Attachment #750799 - Flags: checkin?(rjesup) → checkin+
The point of blocking on bug 872856 was that apparently having too many WebIDL things is causing PGO builds to blow up.  Hopefully this won't make things worse or it'll have to be backed out.
https://hg.mozilla.org/mozilla-central/rev/7e02515f7a1d
https://hg.mozilla.org/mozilla-central/rev/0221d61cef72
https://hg.mozilla.org/mozilla-central/rev/b115d0dbc1d5
https://hg.mozilla.org/mozilla-central/rev/4f60fe3eefc5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Updated

4 years ago
Keywords: verifyme
By caveat, I ended up regression testing this pretty heavily while preparing for a WebRTC presentation. Mostly okay overall - nothing scary appeared to be broken for the video chat cases going through the full process of building three different WebRTC apps.

I dropped my comments for what I found here:

https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.media/OBLvUf7nNkw
Keywords: verifyme

Updated

4 years ago
Status: RESOLVED → VERIFIED

Updated

4 years ago
Duplicate of this bug: 856345
(Assignee)

Updated

2 years ago
Duplicate of this bug: 867408
You need to log in before you can comment on or make changes to this bug.