Closed Bug 1263312 Opened 9 years ago Closed 9 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.

Attachment

General

Creator:
Created:
Updated:
Size: