Convert Dictionary handling to RTCConfiguration IDL dictionary

Convert Dictionary handling to RTCConfiguration IDL dictionary

This preserves behavior as far as possible and adds use counters for
cases that aren't per spec. One intentional change is that the first
argument of the RTCPeerConnection constructor is made optional, as that
is a very low risk change, and requiring the dictionary makes no sense
when providing null, undefined and {iceServers:[]} already work.

Without custom bindings it is unfortunately not possible to distinguish
{certificates:null} from no certificates specified, so that is not
measured.

BUG=649343
Committed: https://crrev.com/379271912cc80682629cc26c7199f89841e3c038
Cr-Commit-Position: refs/heads/master@{#427065}

[foolip, 2016-10-21 13:37:02]

foolip@chromium.org changed reviewers:
+ guidou@chromium.org, hbos@chromium.org, hta@chromium.org

[foolip, 2016-10-21 13:37:03]

PTAL! With this done, I see nothing block unprefixing of RTCPeerConnection. (For
that, I'd send and Intent to Ship.)

[Mike West, 2016-10-21 14:09:13]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2016-10-21 14:09:13]

histograms.xml LGTM.

[hbos_chromium, 2016-10-21 17:18:12]

First pass: Looks good. I'll have to take a closer look on the airplane back to
Sweden or on Monday.

[foolip, 2016-10-21 19:54:57]

The CQ bit was checked by foolip@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 19:55:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 22:48:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 22:48:52]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[foolip, 2016-10-21 23:16:27]

Looks like WebRtcSimulcastBrowserTest.TestVgaReturnsTwoSimulcastStreams is a sad
test, probably because of this code changing behavior:
https://cs.chromium.org/chromium/src/chrome/test/data/webrtc/webrtc-simulcast...

[foolip, 2016-10-22 19:34:36]

The CQ bit was checked by foolip@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-22 19:34:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-22 21:21:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-22 21:21:22]

Dry run: This issue passed the CQ dry run.

[hta - Chromium, 2016-10-23 07:03:49]

lgtm

https://codereview.chromium.org/2442763002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/peerconnection/RTCIceServer.idl (right):

https://codereview.chromium.org/2442763002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCIceServer.idl:13: //
TODO(foolip): RTCIceCredentialType credentialType = "password";
Any reason not to add this now?

[foolip, 2016-10-23 08:58:56]

https://codereview.chromium.org/2442763002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/peerconnection/RTCIceServer.idl (right):

https://codereview.chromium.org/2442763002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCIceServer.idl:13: //
TODO(foolip): RTCIceCredentialType credentialType = "password";
On 2016/10/23 07:03:49, hta - Chromium wrote:
> Any reason not to add this now?

The question is what one would do with the "token" value, is that trivial to
support?

[foolip, 2016-10-23 10:17:58]

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCConfiguration.idl
(right):

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCConfiguration.idl:34: //
TODO(foolip): |iceTransports| should be |iceTransportPolicy|.
Having written https://github.com/w3c/web-platform-tests/pull/4062 and tested
Firefox, I think this is pretty serious, requiring { iceTransports: "relay",
iceTransportPolicy: "relay" } to work on both Chrome and Firefox. I'd like to
minimize observable changes in this CL for bisect-ability, but I think that's
worth fixing before shipping unprefixed RTCPeerConnection, by looking first at
iceTransportPolicy, and falling back to iceTransports with use counter.

[hta - Chromium, 2016-10-23 16:39:03]

lgtm still

https://codereview.chromium.org/2442763002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/peerconnection/RTCIceServer.idl (right):

https://codereview.chromium.org/2442763002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCIceServer.idl:13: //
TODO(foolip): RTCIceCredentialType credentialType = "password";
On 2016/10/23 08:58:56, foolip wrote:
> On 2016/10/23 07:03:49, hta - Chromium wrote:
> > Any reason not to add this now?
> 
> The question is what one would do with the "token" value, is that trivial to
> support?

The correct handling given current support should be checking that the token is
"password", and taking the action prescribed in the spec for unknown values if
it's anything else.

[foolip, 2016-10-23 20:56:51]

https://codereview.chromium.org/2442763002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/peerconnection/RTCIceServer.idl (right):

https://codereview.chromium.org/2442763002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCIceServer.idl:13: //
TODO(foolip): RTCIceCredentialType credentialType = "password";
On 2016/10/23 16:39:02, hta - Chromium wrote:
> On 2016/10/23 08:58:56, foolip wrote:
> > On 2016/10/23 07:03:49, hta - Chromium wrote:
> > > Any reason not to add this now?
> > 
> > The question is what one would do with the "token" value, is that trivial to
> > support?
> 
> The correct handling given current support should be checking that the token
is
> "password", and taking the action prescribed in the spec for unknown values if
> it's anything else.

Turns out that Gecko has credentialType in the dictionary,  but `new
RTCPeerConnection({ iceServers: [{ urls: "turn:turn.example.net", username:
"user", credential: "cred", credentialType: "token" }] })` logs
"RTCConfiguration TURN credentialType "token" is not yet implemented. Treating
as password. https://bugzil.la/1247616" in the console.

I don't think doing that is a good idea, there's then no way to later feature
detect when it's actually supported. Another way would be to have "password" as
the only RTCIceCredentialType enum entry, throwing TypeError for "token". Not
sure, an enum with just one value seems kind of pointless. WDYT?

[Guido Urdaneta, 2016-10-24 08:56:16]

lgtm

[Guido Urdaneta, 2016-10-24 08:56:27]

lgtm

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-expected.txt
(right):

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-expected.txt:22:
PASS new webkitRTCPeerConnection({iceServers:[1, 2, 3]}); threw exception
TypeError: Failed to construct 'RTCPeerConnection': cannot convert to
dictionary..
nit: extra period intended? you can leave it if you don't care.

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-expected.txt:29:
PASS new webkitRTCPeerConnection({iceServers:[], iceTransports:'foo'}); threw
exception TypeError: Failed to construct 'RTCPeerConnection': The provided value
'foo' is not a valid enum value of type RTCIceTransports..
nit: ditto

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-expected.txt:33:
PASS new webkitRTCPeerConnection({iceServers:[], bundlePolicy:'foo'}); threw
exception TypeError: Failed to construct 'RTCPeerConnection': The provided value
'foo' is not a valid enum value of type RTCBundlePolicy..
nit: ditto

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-expected.txt:36:
PASS new webkitRTCPeerConnection({iceServers:[], rtcpMuxPolicy:'foo'}); threw
exception TypeError: Failed to construct 'RTCPeerConnection': The provided value
'foo' is not a valid enum value of type RTCRtcpMuxPolicy..
nit: ditto

[foolip, 2016-10-24 09:17:36]

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-expected.txt
(right):

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-expected.txt:22:
PASS new webkitRTCPeerConnection({iceServers:[1, 2, 3]}); threw exception
TypeError: Failed to construct 'RTCPeerConnection': cannot convert to
dictionary..
On 2016/10/24 08:56:27, Guido Urdaneta wrote:
> nit: extra period intended? you can leave it if you don't care.

These are from the test harness, the exception message itself has one period.

[hbos_chromium, 2016-10-24 11:35:24]

lgtm

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCConfiguration.idl
(right):

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCConfiguration.idl:6: 
nit: Here and other places - do we have a space between the link and the
enum/dictionary in other files? I would remove it here and other places in this
file, but up to you.

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:224:
RtcpMuxPolicy* selectedRtcpMuxPolicy) {
nit:
DCHECK(context) or make it ExecutionContext&
DCHECK(selectedRtcpMuxPolicy) or make it RtcpMuxPolicy&

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:297:
iceServers.append(WebRTCIceServer{url, username, credential});
Add TODO that there should be one WebRTCIceServer with multiple "urls" instead
of multiple WebRTCIceServer with single "url"s.

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:306:
size_t numberOfCertificates = certificates.size();
nit: remove numberOfCertificates in favor of certificates.size()

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:420: //
updateIce as well?
Remove TODO comment.

Because I'm lazy.
Kidding. Because it is still a valid RTCConfiguration even if the certificates
are expired so I don't think parseConfiguration should fail, but it is invalid
to try to use an expired certificate in the construction of a peer connection.
It is not invalid to use the same certificates in updateIce even if they expired
in-between construction and updateIce.

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:774: void
RTCPeerConnection::updateIce(ExecutionContext* context,
Note that not only is the name different from the spec (should be called
setConfiguration), but the implementation is different. Both here and in lower
layers (m_peerHandler->updateICE). File a bug?

Can you add a UseCounter for updateIce?

[foolip, 2016-10-24 11:36:30]

The CQ bit was checked by foolip@chromium.org

[foolip, 2016-10-24 11:36:31]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2442763002/#ps40001
(title: "revert rtcpMuxPolicy default, also measure urls")

[foolip, 2016-10-24 11:36:33]

The CQ bit was unchecked by foolip@chromium.org

[foolip, 2016-10-24 11:36:38]

The CQ bit was unchecked by foolip@chromium.org

[foolip, 2016-10-24 12:05:26]

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCConfiguration.idl
(right):

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCConfiguration.idl:6: 
On 2016/10/24 11:35:24, hbos_chromium wrote:
> nit: Here and other places - do we have a space between the link and the
> enum/dictionary in other files? I would remove it here and other places in
this
> file, but up to you.

In core/ there's pretty consistently a blank line after the spec link, and in
modules/peerconnection/ there's a bit of both. I've always had the blank line
because often a single spec link is for several of the typedefs, enums and the
dictionary/interface that follows.

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:224:
RtcpMuxPolicy* selectedRtcpMuxPolicy) {
On 2016/10/24 11:35:24, hbos_chromium wrote:
> nit:
> DCHECK(context) or make it ExecutionContext&
> DCHECK(selectedRtcpMuxPolicy) or make it RtcpMuxPolicy&

Done with DCHECKs.

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:297:
iceServers.append(WebRTCIceServer{url, username, credential});
On 2016/10/24 11:35:24, hbos_chromium wrote:
> Add TODO that there should be one WebRTCIceServer with multiple "urls" instead
> of multiple WebRTCIceServer with single "url"s.

The spec has a kind of flattening into a validatedServers list in
https://w3c.github.io/webrtc-pc/#set-pc-configuration. Assuming no differences
in semantics between one RTCIceServer with multiple URLs and multiple
RTCIceServer entries, this flattening probably makes implementation easier, so
will stay around I think.

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:306:
size_t numberOfCertificates = certificates.size();
On 2016/10/24 11:35:24, hbos_chromium wrote:
> nit: remove numberOfCertificates in favor of certificates.size()

Done.

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:420: //
updateIce as well?
On 2016/10/24 11:35:24, hbos_chromium wrote:
> Remove TODO comment.
> 
> Because I'm lazy.
> Kidding. Because it is still a valid RTCConfiguration even if the certificates
> are expired so I don't think parseConfiguration should fail, but it is invalid
> to try to use an expired certificate in the construction of a peer connection.
> It is not invalid to use the same certificates in updateIce even if they
expired
> in-between construction and updateIce.

You're right, https://w3c.github.io/webrtc-pc/#dom-peerconnection has this in
the spec but
https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-setconfiguration does
not.

https://codereview.chromium.org/2442763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:774: void
RTCPeerConnection::updateIce(ExecutionContext* context,
On 2016/10/24 11:35:24, hbos_chromium wrote:
> Note that not only is the name different from the spec (should be called
> setConfiguration), but the implementation is different. Both here and in lower
> layers (m_peerHandler->updateICE). File a bug?
> 
> Can you add a UseCounter for updateIce?

I added [Measure] to updateIce in https://codereview.chromium.org/2413703002

There's https://bugs.chromium.org/p/chromium/issues/detail?id=581348 and
https://bugs.chromium.org/p/chromium/issues/detail?id=587453, can you dupe one
into the other and comment on the real one that it replaces updateIce?

[foolip, 2016-10-24 12:05:33]

The CQ bit was checked by foolip@chromium.org

[foolip, 2016-10-24 12:05:34]

The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org,
hta@chromium.org, mkwst@chromium.org, hbos@chromium.org
Link to the patchset: https://codereview.chromium.org/2442763002/#ps60001
(title: "address hbos@'s feedback")

[commit-bot: I haz the power, 2016-10-24 12:05:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-24 14:09:10]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-24 14:10:54]

Description was changed from

==========
Convert Dictionary handling to RTCConfiguration IDL dictionary

This preserves behavior as far as possible and adds use counters for
cases that aren't per spec. One intentional change is that the first
argument of the RTCPeerConnection constructor is made optional, as that
is a very low risk change, and requiring the dictionary makes no sense
when providing null, undefined and {iceServers:[]} already work.

Without custom bindings it is unfortunately not possible to distinguish
{certificates:null} from no certificates specified, so that is not
measured.

BUG=649343
==========

to

==========
Convert Dictionary handling to RTCConfiguration IDL dictionary

This preserves behavior as far as possible and adds use counters for
cases that aren't per spec. One intentional change is that the first
argument of the RTCPeerConnection constructor is made optional, as that
is a very low risk change, and requiring the dictionary makes no sense
when providing null, undefined and {iceServers:[]} already work.

Without custom bindings it is unfortunately not possible to distinguish
{certificates:null} from no certificates specified, so that is not
measured.

BUG=649343

Committed: https://crrev.com/379271912cc80682629cc26c7199f89841e3c038
Cr-Commit-Position: refs/heads/master@{#427065}
==========

[commit-bot: I haz the power, 2016-10-24 14:10:55]

Patchset 4 (id:??) landed as
https://crrev.com/379271912cc80682629cc26c7199f89841e3c038
Cr-Commit-Position: refs/heads/master@{#427065}