Closed Bug 1209744 Opened 5 years ago Closed 4 years ago

Implement RTCPeerConnection canTrickleIceCandidates attribute

Categories

(Core :: WebRTC: Signaling, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed
firefox47 --- fixed
Blocking Flags:

People

(Reporter: mt, Assigned: mt)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 1 obsolete file)

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.
Bug 1209744 - Implement canTrickleIceCandidates attribute, r?bwc
Attachment #8667549 - Flags: review?(docfaraday)
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.
Attachment #8667549 - Flags: review?(docfaraday) → review+
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: nobody → martin.thomson
Keywords: checkin-needed
Removing checkin-needed; this will break webplatform tests if bug 1155923 lands.
Keywords: checkin-needed
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+
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-
Just the webidl change thanks.
Attachment #8668596 - Flags: review?(khuey)
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)
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)
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
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+
backlog: --- → webrtc/webaudio+
Rank: 25
Priority: -- → P2
The latest checkin should address the test problem.
Flags: needinfo?(martin.thomson)
https://hg.mozilla.org/mozilla-central/rev/baa780fc37c3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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)
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 → ---
Attachment #8719351 - Attachment is obsolete: true
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

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-
https://hg.mozilla.org/mozilla-central/rev/d034fcc08831
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Flags: needinfo?(martin.thomson)
Attachment #8721730 - Flags: review?(docfaraday) → review+
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. :)
(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.