Remove RTCIceTransportPolicy "none".

Remove RTCIceTransportPolicy "none".

BUG=659134
Review-Url: https://codereview.chromium.org/2813883004
Cr-Commit-Position: refs/heads/master@{#466611}
Committed: https://chromium.googlesource.com/chromium/src/+/2fc4a74de010301a7266b3bf5d5f28b45604d544

[hbos_chromium, 2017-04-12 12:42:20]

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

[hbos_chromium, 2017-04-12 12:42:32]

hbos@chromium.org changed reviewers:
+ foolip@chromium.org

[commit-bot: I haz the power, 2017-04-12 12:42:33]

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

[hbos_chromium, 2017-04-12 12:42:33]

Please take a look, foolip.

[hbos_chromium, 2017-04-12 12:45:40]

https://codereview.chromium.org/2813883004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt
(right):

https://codereview.chromium.org/2813883004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt:48:
FAIL new RTCPeerConnection({ iceTransports: "none" }) Failed to construct
'RTCPeerConnection': The provided value 'none' is not a valid enum value of type
RTCIceTransportPolicy.
The test
external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html needs
to be updated not to expect "none", which it does due to Blink, not due to the
spec - comment says "iceTransportPolicy is called iceTransports in Blink":
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/...

Can this be done in the same CL?

[foolip, 2017-04-12 13:02:39]

foolip@chromium.org changed reviewers:
+ rbyers@chromium.org

[foolip, 2017-04-12 13:02:40]

The relevant use counters are:
https://www.chromestatus.com/metrics/feature/timeline/popularity/1663
https://www.chromestatus.com/metrics/feature/timeline/popularity/1655

On most days there is not a single hit for either of these, so deprecation would
actually be fairly pointless.

I also can't find RTCIceTransportPolicy "none" documented anywhere like on
https://developer.mozilla.org/en-US/docs/Web/API/RTCConfiguration#RTCIceTrans...
so I think we should bypass the intent process and just remove this. It's
similar to this recent case:
https://codereview.chromium.org/2803603003/

Adding another API owner to check my reasoning.

https://codereview.chromium.org/2813883004/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_peer_connection_handler.cc (left):

https://codereview.chromium.org/2813883004/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_peer_connection_handler.cc:254: webrtc_config->type =
webrtc::PeerConnectionInterface::kNone;
webrtc::PeerConnectionInterface::kNone will remain internally in webrtc I
suppose. If there's no use for it on the web, should it be removed from webrtc
as well?

https://codereview.chromium.org/2813883004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt
(right):

https://codereview.chromium.org/2813883004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt:48:
FAIL new RTCPeerConnection({ iceTransports: "none" }) Failed to construct
'RTCPeerConnection': The provided value 'none' is not a valid enum value of type
RTCIceTransportPolicy.
On 2017/04/12 12:45:40, hbos_chromium wrote:
> The test
> external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html needs
> to be updated not to expect "none", which it does due to Blink, not due to the
> spec - comment says "iceTransportPolicy is called iceTransports in Blink":
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/...
> 
> Can this be done in the same CL?

LayoutTests/external/wpt can be changed in CLs like this, but it's actually
correct that this should fail. It fails because we still support iceTransports
but not with "none" as one of the supported values, and will pass again once
iceTransports is dropped.

https://codereview.chromium.org/2813883004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(left):

https://codereview.chromium.org/2813883004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:255:
UseCounter::kRTCConfigurationIceTransportsNone);
This should also be removed from UseCounter.h

[commit-bot: I haz the power, 2017-04-12 14:13:56]

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

[commit-bot: I haz the power, 2017-04-12 14:13:57]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hbos_chromium, 2017-04-13 11:33:30]

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

[hbos_chromium, 2017-04-13 11:33:42]

PTAL

https://codereview.chromium.org/2813883004/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_peer_connection_handler.cc (left):

https://codereview.chromium.org/2813883004/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_peer_connection_handler.cc:254: webrtc_config->type =
webrtc::PeerConnectionInterface::kNone;
On 2017/04/12 13:02:39, foolip_UTC7 wrote:
> webrtc::PeerConnectionInterface::kNone will remain internally in webrtc I
> suppose. If there's no use for it on the web, should it be removed from webrtc
> as well?

Possibly, I filed https://bugs.webrtc.org/7486

https://codereview.chromium.org/2813883004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt
(right):

https://codereview.chromium.org/2813883004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt:48:
FAIL new RTCPeerConnection({ iceTransports: "none" }) Failed to construct
'RTCPeerConnection': The provided value 'none' is not a valid enum value of type
RTCIceTransportPolicy.
On 2017/04/12 13:02:40, foolip_UTC7 wrote:
> On 2017/04/12 12:45:40, hbos_chromium wrote:
> > The test
> > external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html
needs
> > to be updated not to expect "none", which it does due to Blink, not due to
the
> > spec - comment says "iceTransportPolicy is called iceTransports in Blink":
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/...
> > 
> > Can this be done in the same CL?
> 
> LayoutTests/external/wpt can be changed in CLs like this, but it's actually
> correct that this should fail. It fails because we still support iceTransports
> but not with "none" as one of the supported values, and will pass again once
> iceTransports is dropped.

Acknowledged. So we want to keep expecting iceTransports with "none" but not
enforce it?

https://codereview.chromium.org/2813883004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(left):

https://codereview.chromium.org/2813883004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:255:
UseCounter::kRTCConfigurationIceTransportsNone);
On 2017/04/12 13:02:40, foolip_UTC7 wrote:
> This should also be removed from UseCounter.h

Done.

[commit-bot: I haz the power, 2017-04-13 11:33:45]

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, 2017-04-13 11:41:37]

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

[commit-bot: I haz the power, 2017-04-13 11:41:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hbos_chromium, 2017-04-18 08:42:16]

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

[commit-bot: I haz the power, 2017-04-18 08:42:33]

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, 2017-04-18 10:29:43]

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

[commit-bot: I haz the power, 2017-04-18 10:29:44]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hbos_chromium, 2017-04-18 12:15:07]

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

[commit-bot: I haz the power, 2017-04-18 12:15:23]

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, 2017-04-18 13:50:14]

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

[commit-bot: I haz the power, 2017-04-18 13:50:15]

Dry run: This issue passed the CQ dry run.

[hbos_chromium, 2017-04-18 13:54:49]

PTAL foolip, fixed the failing test.

[foolip, 2017-04-21 16:14:30]

lgtm.

rbyers@, please confirm if you agree with skipping all process for this. With
the gaps in
https://www.chromestatus.com/metrics/feature/timeline/popularity/1663 and
https://www.chromestatus.com/metrics/feature/timeline/popularity/1655, I don't
see what good a deprecation would do.

https://codereview.chromium.org/2813883004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt
(right):

https://codereview.chromium.org/2813883004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt:48:
FAIL new RTCPeerConnection({ iceTransports: "none" }) Failed to construct
'RTCPeerConnection': The provided value 'none' is not a valid enum value of type
RTCIceTransportPolicy.
On 2017/04/13 11:33:42, hbos_chromium wrote:
> On 2017/04/12 13:02:40, foolip_UTC7 wrote:
> > On 2017/04/12 12:45:40, hbos_chromium wrote:
> > > The test
> > > external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html
> needs
> > > to be updated not to expect "none", which it does due to Blink, not due to
> the
> > > spec - comment says "iceTransportPolicy is called iceTransports in Blink":
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/...
> > > 
> > > Can this be done in the same CL?
> > 
> > LayoutTests/external/wpt can be changed in CLs like this, but it's actually
> > correct that this should fail. It fails because we still support
iceTransports
> > but not with "none" as one of the supported values, and will pass again once
> > iceTransports is dropped.
> 
> Acknowledged. So we want to keep expecting iceTransports with "none" but not
> enforce it?

It now throws for the same reason that new RTCPeerConnection({ iceTransports:
"banana" }) would throw, which is that it's not a recognized enum value. But if
iceTransports wasn't a member of RTCConfiguration to begin with, so the fix will
be to remove iceTransports support, or to standardize it and change the test.

https://codereview.chromium.org/2813883004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection.html
(right):

https://codereview.chromium.org/2813883004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection.html:34:
shouldNotThrow("new RTCPeerConnection({iceServers:[],
iceTransportPolicy:'relay'});");
This is fine, but over time I think LayoutTests/fast/peerconnection/ must be
reconciled with LayoutTests/external/wpt/webrtc/, so making changes just to wpt
and where possible removing duplicate coverage would be my strategy.

[Rick Byers, 2017-04-22 05:49:54]

On 2017/04/21 16:14:30, foolip_UTC7 wrote:
> lgtm.
> 
> rbyers@, please confirm if you agree with skipping all process for this. With
> the gaps in
> https://www.chromestatus.com/metrics/feature/timeline/popularity/1663 and
> https://www.chromestatus.com/metrics/feature/timeline/popularity/1655, I don't
> see what good a deprecation would do.

Sorry I missed the earlier request.  Yes I agree - this is trivial, just a bug
that an extra enum was supported, but too small to warrant outreach or anything.
 We fix lots of bugs in the handling of edge case values which don't happen to
change an IDL file without doing an intent.  This seems no different really to
me.

In general your judgement here is better than anyone's and there will always be
some grey area where we learn from mistakes on either side.  So don't feel the
need to block on a 2nd opinion ;-)

> 
>
https://codereview.chromium.org/2813883004/diff/1/third_party/WebKit/LayoutTe...
> File
>
third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt
> (right):
> 
>
https://codereview.chromium.org/2813883004/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor-expected.txt:48:
> FAIL new RTCPeerConnection({ iceTransports: "none" }) Failed to construct
> 'RTCPeerConnection': The provided value 'none' is not a valid enum value of
type
> RTCIceTransportPolicy.
> On 2017/04/13 11:33:42, hbos_chromium wrote:
> > On 2017/04/12 13:02:40, foolip_UTC7 wrote:
> > > On 2017/04/12 12:45:40, hbos_chromium wrote:
> > > > The test
> > > > external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html
> > needs
> > > > to be updated not to expect "none", which it does due to Blink, not due
to
> > the
> > > > spec - comment says "iceTransportPolicy is called iceTransports in
Blink":
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/...
> > > > 
> > > > Can this be done in the same CL?
> > > 
> > > LayoutTests/external/wpt can be changed in CLs like this, but it's
actually
> > > correct that this should fail. It fails because we still support
> iceTransports
> > > but not with "none" as one of the supported values, and will pass again
once
> > > iceTransports is dropped.
> > 
> > Acknowledged. So we want to keep expecting iceTransports with "none" but not
> > enforce it?
> 
> It now throws for the same reason that new RTCPeerConnection({ iceTransports:
> "banana" }) would throw, which is that it's not a recognized enum value. But
if
> iceTransports wasn't a member of RTCConfiguration to begin with, so the fix
will
> be to remove iceTransports support, or to standardize it and change the test.
> 
>
https://codereview.chromium.org/2813883004/diff/60001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection.html
> (right):
> 
>
https://codereview.chromium.org/2813883004/diff/60001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection.html:34:
> shouldNotThrow("new RTCPeerConnection({iceServers:[],
> iceTransportPolicy:'relay'});");
> This is fine, but over time I think LayoutTests/fast/peerconnection/ must be
> reconciled with LayoutTests/external/wpt/webrtc/, so making changes just to
wpt
> and where possible removing duplicate coverage would be my strategy.

[hbos_chromium, 2017-04-24 08:05:02]

The CQ bit was checked by hbos@chromium.org

[commit-bot: I haz the power, 2017-04-24 08:05:25]

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, 2017-04-24 09:31:21]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493021102481030,
"parent_rev": "c9dd99240fda60e9daca1f9b87c20a92b10de38a", "commit_rev":
"2fc4a74de010301a7266b3bf5d5f28b45604d544"}

[commit-bot: I haz the power, 2017-04-24 09:31:31]

Description was changed from

==========
Remove RTCIceTransportPolicy "none".

BUG=659134
==========

to

==========
Remove RTCIceTransportPolicy "none".

BUG=659134

Review-Url: https://codereview.chromium.org/2813883004
Cr-Commit-Position: refs/heads/master@{#466611}
Committed:
https://chromium.googlesource.com/chromium/src/+/2fc4a74de010301a7266b3bf5d5f...
==========

[commit-bot: I haz the power, 2017-04-24 09:31:32]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2fc4a74de010301a7266b3bf5d5f...