1181768 - Implement already-defined RTCPeerConnection.getConfiguration

Status: RESOLVED FIXED

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

Description

[Martin Thomson [:mt:]]

The current situation is poor:
 - getConfiguration is defined in WebIDL, but not actually implemented in PeerConnection.js
 - updateIce is in the WebIDL and PeerConnection.js, though it only generates a NotImplemented error; but it has been removed from the spec
 - setConfiguration is in the spec, but nowhere else

We actually have a configuration, we could return it.  We could reject any attempts to set a configuration the same way.  updateIce can go.

Comment 1

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

Part of implementing setConfiguration will be to figure out the desired effects of changing things mid-connection. E.g. iceTransportPolicy, Bug 1187775 comment 5.

Comment 2

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

And Bug 1187775 comment 22.

Comment 3

[Maire Reavy [:mreavy]]

Jan-Ivar - where does this stand?  Is setConfiguration problematic?

Comment 4

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

Aside from some unknowns [1] in the spec, this seems straight forward, since it looks like an ICE restart (Bug 906986) is needed for many of these settings to take effect, which hopefully rules out expectations that these things should be changeable in the middle of call setup. 

[1] https://github.com/w3c/webrtc-pc/issues/373

Comment 5

[Maire Reavy [:mreavy]]

Sorry, marked the wrong bug.  Back to 15.

Comment 6

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

(Going through my priorities)

Re-reading comment 0, is the ask here merely to move the NotImplemented error from updateIce to setConfiguration and make getConfiguration return the config, or is it to actually implement setConfiguration?

If the latter, is there a pressing use-case (don't most people set config in constructor and never change it)?

Comment 7

[Martin Thomson [:mt:]]

updateIce is gone already.  I think that reduces this bug to: implement getConfiguration.

Comment 8

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

Attachment: MozReview Request: Bug 1181768 - Make already-defined pc.getConfiguration() work.

Review commit: https://reviewboard.mozilla.org/r/38291/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38291/

Comment 9

[Martin Thomson [:mt:]]

Comment on attachment 8726878 [details]
MozReview Request: Bug 1181768 - Make already-defined pc.getConfiguration() work.

https://reviewboard.mozilla.org/r/38291/#review34961

::: dom/media/tests/mochitest/test_peerConnection_bug825703.html:25
(Diff revision 1)
> +var comparable = o => (typeof o != 'object' || !o)? o :
> +  Object.keys(o).sort().reduce((co, key) => (co[key] = comparable(o[key]), co), {});

This doesn't preserve array-ness, which I suppose is OK (but it needs a comment to that effect in addition to the one explaining why this function exists).  You could definiedly do better at making this clearer too; it's pretty tough reading this sort of code.  The comma operator in particular is nasty.  Please consider using {}.

::: dom/media/tests/mochitest/test_peerConnection_bug825703.html:88
(Diff revision 1)
> +  var pc = new RTCPeerConnection(config);
> +  is(JSON.stringify(comparable(pc.getConfiguration())),
> +     JSON.stringify(comparable(config)), "getConfiguration");

What happens if I feed in:
```
{ watermelon: 'hat' }
```
In other words, please test unknown keys.

Comment 10

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

Thanks, will add comments.

(In reply to Martin Thomson [:mt:] from comment #9)
> ::: dom/media/tests/mochitest/test_peerConnection_bug825703.html:88
> (Diff revision 1)
> > +  var pc = new RTCPeerConnection(config);
> > +  is(JSON.stringify(comparable(pc.getConfiguration())),
> > +     JSON.stringify(comparable(config)), "getConfiguration");
> 
> What happens if I feed in:
> ```
> { watermelon: 'hat' }
> ```
> In other words, please test unknown keys.

This seems out of scope, since { watermelon: 'hat' } would never make it to PeerConnection.js - presumably the webidl binding code is sufficiently tested elsewhere - and redundant because of invariants: PeerConnection.js would see this default dictionary instead:

  {"bundlePolicy":"balanced","iceTransportPolicy":"all","peerIdentity":null}

which would go through the same code path as the existing getConfiguration test.

Comment 11

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

https://reviewboard.mozilla.org/r/38291/#review34961

> What happens if I feed in:
> ```
> { watermelon: 'hat' }
> ```
> In other words, please test unknown keys.

replied in comment 10.

Comment 12

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

Comment on attachment 8726878 [details]
MozReview Request: Bug 1181768 - Make already-defined pc.getConfiguration() work.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38291/diff/1-2/

Comment 13

[Martin Thomson [:mt:]]

Comment on attachment 8726878 [details]
MozReview Request: Bug 1181768 - Make already-defined pc.getConfiguration() work.

https://reviewboard.mozilla.org/r/38291/#review34969

Oops, I did mean to give you an r+ on the last one.  Here.  And you are right about the default dictionary state.

maybe rename 'comparable' to 'toComparable', because 'comparable' implies to me 'isComparable'.

Comment 14

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

Comment on attachment 8726878 [details]
MozReview Request: Bug 1181768 - Make already-defined pc.getConfiguration() work.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38291/diff/2-3/

Comment 15

[Pulsebot]

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

Comment 16

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

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