1209744 - Implement RTCPeerConnection canTrickleIceCandidates attribute

Status: RESOLVED FIXED

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

Description

[Martin Thomson [:mt:]]

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.

Comment 1

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1209744 - Switch to setIdentityProvider for js-implemented bindings test, r=jib

Bug 1209744 - Implement canTrickleIceCandidates attribute, r?bwc

Comment 2

[Byron Campen [:bwc]]

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.

Comment 3

[Martin Thomson [:mt:]]

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.

Comment 4

[Martin Thomson [:mt:]]

Removing checkin-needed; this will break webplatform tests if bug 1155923 lands.

Comment 5

[Martin Thomson [:mt:]]

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

Comment 6

[Martin Thomson [:mt:]]

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

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Automatic r- for using mozreview.

Comment 8

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-1209744-Implement-canTrickleIceCandidates-attrib.patch

Just the webidl change thanks.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b958e25b1ecf

Comment 10

[Nigel Babu [:nigelb]]

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"

Comment 11

[Nigel Babu [:nigelb]]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3adc12ed39e2

Comment 12

[Martin Thomson [:mt:]]

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 13

[Martin Thomson [:mt:]]

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

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/baa780fc37c3

Comment 16

[Martin Thomson [:mt:]]

The latest checkin should address the test problem.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/baa780fc37c3

Comment 18

[Eric Shepherd [:sheppy]]

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

Comment 19

[Jean-Yves Perrier [:teoli]]

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.

Comment 20

[Martin Thomson [:mt:]]

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.

Comment 21

[Martin Thomson [:mt:]]

Attachment: 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/

Comment 22

[Martin Thomson [:mt:]]

Attachment: 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/

Comment 23

[Martin Thomson [:mt:]]

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/

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

r- for mozreview

Comment 25

[Pulsebot]

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

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d034fcc08831

Comment 27

[Byron Campen [:bwc]]

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".

Comment 28

[Eric Shepherd [:sheppy]]

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. :)

Comment 29

[Nils Ohlmeier [:drno]]

(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.