Closed Bug 1263312 Opened 8 years ago Closed 7 years ago

Have addIceCandidate, setLocalDescription et al take dictionary (spec update)

Categories

(Core :: WebRTC, defect, P2)

Product:
Core
Component:
WebRTC
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox48 --- affected
firefox53 --- fixed

People

(Reporter: jib, Assigned: jib)

References

(
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(6 files, 1 obsolete file)

Bug 1263312 - Update RTCIceCandidate to spec.
58 bytes, text/x-review-board-request
smaug
: review+
drno
: review+
Details
Bug 1263312 - Have addIceCandidate take a dictionary.
58 bytes, text/x-review-board-request
smaug
: review+
drno
: review+
Details
Bug 1263312 - Have set(Local|Remote)Description take dictionaries.
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1263312 - Have createOffer/Answer resolve with dictionaries.
58 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1263312 - Update most tests to skip RTCIceCandidate construction.
58 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1263312 - Update most tests to skip RTCSessionDescription construction.
58 bytes, text/x-review-board-request
drno
: review+
Details 
A recent update:

  Promise<void> setLocalDescription (RTCSessionDescriptionInit description);
  Promise<void> setRemoteDescription (RTCSessionDescriptionInit description);
  Promise<void> addIceCandidate ((RTCIceCandidateInit or RTCIceCandidate) candidate);

which lets people do:

    setLocalDescription(offer);

instead of the tedious:

    setLocalDescription(new RTCSessionDescription(offer));

(The latter still works since the interface when serialized is indistinguishable from its init-dict).
Assignee: nobody → jib
Rank: 23
Priority: -- → P2
No longer blocks: 1313966
Depends on: 1313966
Attachment #8740695 - Attachment is obsolete: true
Comment on attachment 8811474 [details]
Bug 1263312 - Update RTCIceCandidate to spec.

https://reviewboard.mozilla.org/r/93584/#review93640

::: dom/webidl/RTCIceCandidate.webidl:18
(Diff revision 1)
> -  unsigned short sdpMLineIndex;
> +  unsigned short? sdpMLineIndex = null;
>  };
>  
>  [Pref="media.peerconnection.enabled",
>   JSImplementation="@mozilla.org/dom/rtcicecandidate;1",
>   Constructor(optional RTCIceCandidateInit candidateInitDict)]

If I read the Constructor in the spec correctly the candidateInitDict is not optional.
Attachment #8811474 - Flags: review?(drno) → review+
Comment on attachment 8811474 [details]
Bug 1263312 - Update RTCIceCandidate to spec.

https://reviewboard.mozilla.org/r/93584/#review93642

::: dom/webidl/RTCIceCandidate.webidl:23
(Diff revision 1)
>   Constructor(optional RTCIceCandidateInit candidateInitDict)]
>  interface RTCIceCandidate {
> -  attribute DOMString?      candidate;
> +  attribute DOMString       candidate;
>    attribute DOMString?      sdpMid;
>    attribute unsigned short? sdpMLineIndex;
>  

If we don't have it already can you please file a follow up bug to add all the missing attributes to the RTCIceCandidate interface?
Comment on attachment 8811475 [details]
Bug 1263312 - Have addIceCandidate take a dictionary.

https://reviewboard.mozilla.org/r/93586/#review93646

I would like to resolve my question about the PC_REMOTE_ADD_MATCHING_MID_AND_MISSING_INDEX before r+'ing this.

::: dom/media/PeerConnection.js:1025
(Diff revision 1)
>  
> -
>    addIceCandidate: function(c, onSuccess, onError) {
>      return this._legacyCatchAndCloseGuard(onSuccess, onError, () => {
> -      if (!c.candidate && !c.sdpMLineIndex) {
> -        throw new this._win.DOMException("Invalid candidate passed to addIceCandidate!",
> +      if (!c) {
> +        // TODO: Implement processing for end-of-candidates indication

I created bug 1318167 for that, as I think that mostly involves work on nICEr. Can you add the bug number to the comment please?

::: dom/media/tests/mochitest/test_peerConnection_addIceCandidate.html:85
(Diff revision 1)
> +      function PC_REMOTE_ADD_MATCHING_MID_AND_MISSING_INDEX(test) {
> +        // Note: it is probably not a good idea to automatically fill a missing
> +        //       MLineIndex with a default value of zero, see bug 1157034
> +        var broken = new RTCIceCandidate(
> +          {candidate:"candidate:1 1 UDP 2130706431 192.168.2.1 50005 typ host",
> +           sdpMid: "sdparta_0"});
> +        return test.pcRemote._pc.addIceCandidate(broken)
>          .then(
> -          generateErrorCallback("addIceCandidate should have failed."),
> -          err => {
> +          // FIXME this needs to be updated once bug 1157034 is fixed
> +          todo(false, "Missing index in got automatically set to a valid value bz://1157034")
> -            is(err.name, "InvalidCandidateError", "Error is InvalidCandidateError");
> -          }

Why is this an error?
My understanding is that the caller can provide either the Mid or the Index or both. So a missing index is not error as long the Mid has a valid value.
Comment on attachment 8811478 [details]
Bug 1263312 - Update most tests to skip RTCIceCandidate construction.

https://reviewboard.mozilla.org/r/93592/#review93654
Attachment #8811478 - Flags: review?(drno) → review+
Comment on attachment 8811477 [details]
Bug 1263312 - Have createOffer/Answer resolve with dictionaries.

https://reviewboard.mozilla.org/r/93590/#review93656

::: dom/media/PeerConnection.js:1176
(Diff revision 1)
> -    return new this._win.RTCSessionDescription({ type: this._localType,
> +    return new this._win.RTCSessionDescription({ type: this._localType, sdp });
> -                                                    sdp: sdp });

I do not understand why this no longer needs the 'sdp: ' part?
Is that some magic like: the RTCSessionDescription only takes two parameters and if you name one of the parameters the second unnamed parameter is automatically filled in?
Attachment #8811477 - Flags: review?(drno) → review+
Comment on attachment 8811477 [details]
Bug 1263312 - Have createOffer/Answer resolve with dictionaries.

https://reviewboard.mozilla.org/r/93590/#review93658

::: dom/media/PeerConnection.js:1185
(Diff revision 1)
> -    return new this._win.RTCSessionDescription({ type: this._remoteType,
> +    return new this._win.RTCSessionDescription({ type: this._remoteType, sdp });
> -                                                    sdp: sdp });

Same question regarding missing 'sdp: ' here.
Comment on attachment 8811479 [details]
Bug 1263312 - Update most tests to skip RTCSessionDescription construction.

https://reviewboard.mozilla.org/r/93594/#review93660

::: dom/media/tests/mochitest/identity/test_fingerprints.html:89
(Diff revision 1)
> -      var desc = new RTCSessionDescription({ type: 'offer', sdp: sdp });
> +      return pcStrict.setRemoteDescription({ type: 'offer', sdp });
> -      return pcStrict.setRemoteDescription(desc);

Everywhere else you specified 'sdp: sdp', but here not. Is that on purpose?
Attachment #8811479 - Flags: review?(drno) → review+
Comment on attachment 8811474 [details]
Bug 1263312 - Update RTCIceCandidate to spec.

https://reviewboard.mozilla.org/r/93584/#review93664

::: dom/webidl/RTCIceCandidate.webidl:18
(Diff revision 1)
> -  unsigned short sdpMLineIndex;
> +  unsigned short? sdpMLineIndex = null;
>  };
>  
>  [Pref="media.peerconnection.enabled",
>   JSImplementation="@mozilla.org/dom/rtcicecandidate;1",
>   Constructor(optional RTCIceCandidateInit candidateInitDict)]

As drno said, param to ctor shouldn't be optional.
Attachment #8811474 - Flags: review?(bugs) → review+
Comment on attachment 8811475 [details]
Bug 1263312 - Have addIceCandidate take a dictionary.

https://reviewboard.mozilla.org/r/93586/#review93666

r+ to the .webidl
Attachment #8811475 - Flags: review?(bugs) → review+
Comment on attachment 8811476 [details]
Bug 1263312 - Have set(Local|Remote)Description take dictionaries.

https://reviewboard.mozilla.org/r/93588/#review93670

::: dom/webidl/RTCPeerConnection.webidl:10
(Diff revision 1)
>   *
>   * The origin of this IDL file is
>   * http://w3c.github.io/webrtc-pc/#interface-definition
>   */
>  
> -callback RTCSessionDescriptionCallback = void (RTCSessionDescription sdp);
> +callback RTCSessionDescriptionCallback = void (RTCSessionDescriptionInit description);

This doesn't follow what is in the spec.
Attachment #8811476 - Flags: review?(bugs) → review-
I've filed https://github.com/w3c/webrtc-pc/pull/949 with the spec to fix this.
Comment on attachment 8811476 [details]
Bug 1263312 - Have set(Local|Remote)Description take dictionaries.

https://reviewboard.mozilla.org/r/93588/#review97714

::: dom/webidl/RTCPeerConnection.webidl:10
(Diff revision 1)
>   *
>   * The origin of this IDL file is
>   * http://w3c.github.io/webrtc-pc/#interface-definition
>   */
>  
> -callback RTCSessionDescriptionCallback = void (RTCSessionDescription sdp);
> +callback RTCSessionDescriptionCallback = void (RTCSessionDescriptionInit description);

https://github.com/w3c/webrtc-pc/pull/949 was merged, so this should be good now.
Attachment #8811476 - Flags: review- → review?(bugs)
Comment on attachment 8811474 [details]
Bug 1263312 - Update RTCIceCandidate to spec.

https://reviewboard.mozilla.org/r/93584/#review93642

> If we don't have it already can you please file a follow up bug to add all the missing attributes to the RTCIceCandidate interface?

Filed bug 1322186.
(In reply to Nils Ohlmeier [:drno] from comment #12)
> > -    return new this._win.RTCSessionDescription({ type: this._localType, sdp: sdp });
> > +    return new this._win.RTCSessionDescription({ type: this._localType, sdp });
> 
> I do not understand why this no longer needs the 'sdp: ' part?

It's implicit. ES6 object literal shorthand. [1]

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer#New_notations_in_ECMAScript_2015
Rebased.
And addressed feedback.
Comment on attachment 8811476 [details]
Bug 1263312 - Have set(Local|Remote)Description take dictionaries.

https://reviewboard.mozilla.org/r/93588/#review97808

I assume there are tests verifying the old style works still (the one not using *Init as param).

The change is of course a breaking change, since by returning dictionary and not interface object one returns quite a bit different thing. But given this is webrtc, perhaps I shouldn't be too worried and it is in the spec.
Attachment #8811476 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #30)
> Comment on attachment 8811476 [details]
> Bug 1263312 - Have set(Local|Remote)Description take dictionaries.
> 
> https://reviewboard.mozilla.org/r/93588/#review97808
> 
> I assume there are tests verifying the old style works still (the one not
> using *Init as param).

Yes [1].

> The change is of course a breaking change, since by returning dictionary and
> not interface object one returns quite a bit different thing. But given this
> is webrtc, perhaps I shouldn't be too worried and it is in the spec.

It helps that the interface is little more than a glorified dictionary, serializable, and usable as input to its own dictionary-taking (copy) constructor.

To be broken by this, code would probably need use typeof, or rely on read-only attributes resisting change, unusual behavior. Below my personal threshold for breakage of legacy code (which I'll readily admit is high as I fought and failed to keep this out of the spec entirely).

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/test_peerConnection_callbacks.html
PTAL. [1] should solve your concern in comment 10. Would love to land this and bug 1322274.

[1] https://reviewboard.mozilla.org/r/93586/diff/2/
Flags: needinfo?(drno)
Comment on attachment 8811475 [details]
Bug 1263312 - Have addIceCandidate take a dictionary.

https://reviewboard.mozilla.org/r/93586/#review98190
Attachment #8811475 - Flags: review?(drno) → review+
Sorry, work week plus missing notifications for re-reviews... I guess next time I take myself out of the review to make sure I see it if a new revision comes in.
Flags: needinfo?(drno)
No worries. Maybe updates should generate a notification if r? is already set. I wonder if there's a mozReview bug on that.
Rebased.
Pushed by jbruaroey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a667af08ffb5
Update RTCIceCandidate to spec. r=drno,smaug
https://hg.mozilla.org/integration/autoland/rev/b311b7e439e8
Have addIceCandidate take a dictionary. r=drno,smaug
https://hg.mozilla.org/integration/autoland/rev/33449a5d1676
Have set(Local|Remote)Description take dictionaries. r=smaug
https://hg.mozilla.org/integration/autoland/rev/6b71306765d2
Have createOffer/Answer resolve with dictionaries. r=drno
https://hg.mozilla.org/integration/autoland/rev/1814c92e203b
Update most tests to skip RTCIceCandidate construction. r=drno
https://hg.mozilla.org/integration/autoland/rev/17ae85e95667
Update most tests to skip RTCSessionDescription construction. r=drno
Pushed by jbruaroey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d42e17954d98
Update RTCIceCandidate to spec. r=drno,smaug
https://hg.mozilla.org/integration/autoland/rev/67e0a99797a4
Have addIceCandidate take a dictionary. r=drno,smaug
https://hg.mozilla.org/integration/autoland/rev/4e02aa4db316
Have set(Local|Remote)Description take dictionaries. r=smaug
https://hg.mozilla.org/integration/autoland/rev/1e7047778cce
Have createOffer/Answer resolve with dictionaries. r=drno
https://hg.mozilla.org/integration/autoland/rev/ede4f590709f
Update most tests to skip RTCIceCandidate construction. r=drno
https://hg.mozilla.org/integration/autoland/rev/ce72565dbefd
Update most tests to skip RTCSessionDescription construction. r=drno
You need to log in before you can comment on or make changes to this bug.