Implement RTCPeerConnection canTrickleIceCandidates attribute

RESOLVED FIXED in Firefox 44

Status

()

Core
WebRTC: Signaling
P2
normal
Rank:
25
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mt, Assigned: mt)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla44
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox44 fixed, firefox47 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
This is pretty straightforward stuff.  I've modified existing tests to add some new checks.  The non-trickle tests now actively remove the a=ice-options line so that they can test this attribute becoming false.
(Assignee)

Comment 1

2 years ago
Created attachment 8667549 [details]
MozReview Request: Bug 1209744 - Switch to setIdentityProvider for js-implemented bindings test, r=jib

Bug 1209744 - Implement canTrickleIceCandidates attribute, r?bwc
Attachment #8667549 - Flags: review?(docfaraday)

Comment 2

2 years ago
https://reviewboard.mozilla.org/r/20773/#review18609

Looks like mozreview is not letting me say "fix it then ship it"...

::: dom/media/PeerConnection.js:972
(Diff revision 1)
> +      this._canTrickle = null;

Might want a comment that null is used to indicate that we don't know, according to the JSEP spec.

::: dom/media/tests/mochitest/nonTrickleIce.js:22
(Diff revision 1)
> -        test._local_offer = test.pcLocal.localDescription;
> +        test._local_offer = removeTrickleOption(test.pcLocal.localDescription);

We have code that ought to be removing the "trickle" ice-option from the local SDP once gathering completes. Is this not working?

::: dom/media/tests/mochitest/nonTrickleIce.js:54
(Diff revision 1)
> -        test._remote_answer = test.pcRemote.localDescription;
> +        test._remote_answer = removeTrickleOption(test.pcRemote.localDescription);

Same question here.

Updated

2 years ago
Attachment #8667549 - Flags: review?(docfaraday) → review+
(Assignee)

Comment 3

2 years ago
https://reviewboard.mozilla.org/r/20773/#review18609

> We have code that ought to be removing the "trickle" ice-option from the local SDP once gathering completes. Is this not working?

I looked for code like that, but while we handle a remote description that doesn't have `a=ice-options:trickle` perfectly well (as these tests show), we don't actually remove the option (at least in mochitests, the other unit tests are probably more thorough).  What I'm need to do is tell the other side that trickle isn't even possible, for which you have to remove the ice-options line.
(Assignee)

Updated

2 years ago
Assignee: nobody → martin.thomson
Keywords: checkin-needed
(Assignee)

Comment 4

2 years ago
Removing checkin-needed; this will break webplatform tests if bug 1155923 lands.
Keywords: checkin-needed
(Assignee)

Comment 5

2 years ago
Comment on attachment 8667549 [details]
MozReview Request: Bug 1209744 - Switch to setIdentityProvider for js-implemented bindings test, r=jib

Bug 1209744 - Implement canTrickleIceCandidates attribute, r=bwc
Attachment #8667549 - Attachment description: MozReview Request: Bug 1209744 - Implement canTrickleIceCandidates attribute, r?bwc → MozReview Request: Bug 1209744 - Implement canTrickleIceCandidates attribute, r=bwc
Attachment #8667549 - Flags: review+
(Assignee)

Comment 6

2 years ago
Comment on attachment 8667549 [details]
MozReview Request: Bug 1209744 - Switch to setIdentityProvider for js-implemented bindings test, r=jib

Bug 1209744 - Implement canTrickleIceCandidates attribute, r=bwc
Attachment #8667549 - Flags: review?(khuey)
Comment on attachment 8667549 [details]
MozReview Request: Bug 1209744 - Switch to setIdentityProvider for js-implemented bindings test, r=jib

Automatic r- for using mozreview.
Attachment #8667549 - Flags: review?(khuey) → review-
(Assignee)

Comment 8

2 years ago
Created attachment 8668596 [details] [diff] [review]
0001-Bug-1209744-Implement-canTrickleIceCandidates-attrib.patch

Just the webidl change thanks.
Attachment #8668596 - Flags: review?(khuey)
Attachment #8668596 - Flags: review?(khuey) → review+
Keywords: dev-doc-needed
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b958e25b1ecf
Keywords: checkin-needed
I'm seeing Mulet M(1) failures that probably have to do with this patch:

https://treeherder.mozilla.org/logviewer.html#?job_id=15169118&repo=mozilla-inbound

1253 INFO TEST-UNEXPECTED-FAIL | dom/bindings/test/test_exceptions_from_jsimplemented.html | Should have the exception we expect - got "conn.updateIce is not a function", expected "updateIce not yet implemented"
Flags: needinfo?(martin.thomson)
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3adc12ed39e2
(Assignee)

Updated

2 years ago
Attachment #8667549 - Attachment description: MozReview Request: Bug 1209744 - Implement canTrickleIceCandidates attribute, r=bwc → MozReview Request: Bug 1209744 - Remove updateIce from js-implemented bindings test, r=jib
Attachment #8667549 - Flags: review- → review?(jib)
(Assignee)

Comment 12

2 years ago
Comment on attachment 8667549 [details]
MozReview Request: Bug 1209744 - Switch to setIdentityProvider for js-implemented bindings test, r=jib

Bug 1209744 - Remove updateIce from js-implemented bindings test, r=jib
(Assignee)

Comment 13

2 years ago
Comment on attachment 8667549 [details]
MozReview Request: Bug 1209744 - Switch to setIdentityProvider for js-implemented bindings test, r=jib

Bug 1209744 - Switch to setIdentityProvider for js-implemented bindings test, r=jib
Attachment #8667549 - Attachment description: MozReview Request: Bug 1209744 - Remove updateIce from js-implemented bindings test, r=jib → MozReview Request: Bug 1209744 - Switch to setIdentityProvider for js-implemented bindings test, r=jib
Comment on attachment 8667549 [details]
MozReview Request: Bug 1209744 - Switch to setIdentityProvider for js-implemented bindings test, r=jib

https://reviewboard.mozilla.org/r/20775/#review19193

lgtm
Attachment #8667549 - Flags: review?(jib) → review+
Keywords: checkin-needed

Updated

2 years ago
backlog: --- → webrtc/webaudio+
Rank: 25
Priority: -- → P2

Comment 15

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/baa780fc37c3
Keywords: checkin-needed
(Assignee)

Comment 16

2 years ago
The latest checkin should address the test problem.
Flags: needinfo?(martin.thomson)
https://hg.mozilla.org/mozilla-central/rev/baa780fc37c3
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Documentation updated:

Added canTrickleIceCandidates to property list: https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection#Properties

Removed updateIce() from https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection

Created page https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/canTrickleIceCandidates

Verified that these changes are mentioned on Firefox 44 for developers; they already were: https://developer.mozilla.org/en-US/Firefox/Releases/44#WebRTC
Keywords: dev-doc-needed → dev-doc-complete
Martin, does this sticked? 

I don't see the canTrickleIceCandidates in the latest version of the webidl (http://mxr.mozilla.org/mozilla-central/source/dom/webidl/RTCPeerConnection.webidl ) and in hg log it looks like there was a backout of the patch: http://mxr.mozilla.org/mozilla-central/source/dom/webidl/RTCPeerConnection.webidl (backed out by Nigel). It looks like what landed in comment 17 is only touching tests and not adding the actual feature.

Sheppy: I think we need a definition (or a link to it) of 'tricked ICECandidate' from this article. It is far from obvious. I had to look at the spec to understand what it meant.
Flags: needinfo?(martin.thomson)
Keywords: dev-doc-complete → dev-doc-needed
(Assignee)

Comment 20

2 years ago
Thanks :teoli, yes, this needs to be re-landed.  When I have Internet better than this, I will make sure that the backout is backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 21

2 years ago
Created attachment 8719351 [details]
MozReview Request: Bug 1209744 - Implement canTrickleIceCandidates attribute, r=bwc,khuey

Review commit: https://reviewboard.mozilla.org/r/34939/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34939/
(Assignee)

Comment 22

2 years ago
Created attachment 8721730 [details]
MozReview Request: Bug 1209744 - Implement canTrickleIceCandidates attribute, r=bwc,khuey

Review commit: https://reviewboard.mozilla.org/r/35769/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35769/
Attachment #8721730 - Flags: review?(khuey)
Attachment #8721730 - Flags: review?(docfaraday)
(Assignee)

Updated

2 years ago
Attachment #8719351 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8721730 - Flags: review?(khuey)
Attachment #8721730 - Flags: review?(docfaraday)
(Assignee)

Comment 23

2 years ago
Comment on attachment 8721730 [details]
MozReview Request: Bug 1209744 - Implement canTrickleIceCandidates attribute, r=bwc,khuey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35769/diff/1-2/
Attachment #8721730 - Flags: review?(khuey)
Attachment #8721730 - Flags: review?(docfaraday)
Comment on attachment 8721730 [details]
MozReview Request: Bug 1209744 - Implement canTrickleIceCandidates attribute, r=bwc,khuey

r- for mozreview
Attachment #8721730 - Flags: review?(khuey) → review-

Comment 25

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d034fcc08831

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d034fcc08831
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Flags: needinfo?(martin.thomson)

Updated

2 years ago
Attachment #8721730 - Flags: review?(docfaraday) → review+

Comment 27

2 years ago
Comment on attachment 8721730 [details]
MozReview Request: Bug 1209744 - Implement canTrickleIceCandidates attribute, r=bwc,khuey

https://reviewboard.mozilla.org/r/35769/#review32469

::: dom/media/PeerConnection.js:977
(Diff revision 2)
> +      let lines = section.toLowerCase().split(/(?:\r\n?|\n)/);

This regex seems a little more complicated than it needs to be.

::: dom/media/PeerConnection.js:1000
(Diff revision 2)
> +    this._canTrickle =
> +      containsTrickle(topSection) || sections.every(containsTrickle);

The JSEP spec says to set this to true if any section has "trickle".
Blocks: 1050930
This has been documented: https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/canTrickleIceCandidates

It's linked to from the main RTCPeerConnection page, and is also mentioned on Firefox 44 for developers: https://developer.mozilla.org/en-US/Firefox/Releases/44#WebRTC

It would be helpful if someone can take a quick look to confirm that the content is correct. If it's not, please restore dev-doc-needed and let me know what I did wrong. :)
Keywords: dev-doc-needed → dev-doc-complete
(In reply to Eric Shepherd [:sheppy] from comment #28)
> It would be helpful if someone can take a quick look to confirm that the
> content is correct. If it's not, please restore dev-doc-needed and let me
> know what I did wrong. :)

Looks good.
You need to log in before you can comment on or make changes to this bug.