Change the default rtcp mux policy from negotiate to require.

Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux policy from negotiated to require.
According to the UMA data in 28 days, 99.96% users choose the default value of rtcp mux policy which means nearly all the users who are using "negotiate" policy would be affected by this change. Because among all the users with media, only 3%  disabled the rtcp mux so it's not risky to make this change.

The UMA "SelectedRtcpMuxPolicy" is removed in this CL since it is only used to gauge the risk of this change.

BUG=webrtc:6030
Committed: https://crrev.com/3291edb6105e4f7e39a0b4e81da9c24e234ad185
Cr-Commit-Position: refs/heads/master@{#439955}

[zhihuang1, 2016-06-09 00:26:35]

zhihuang@chromium.org changed reviewers:
+ pthatcher@webrtc.org

[zhihuang1, 2016-06-09 00:26:35]

I find these three places where we should make changes.

[pthatcher2, 2016-06-13 16:17:52]

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

[pthatcher2, 2016-06-13 16:17:53]

lgtm

[zhihuang1, 2016-06-21 23:35:02]

zhihuang@chromium.org changed reviewers:
+ tommi@chromium.org
- pthatcher@chromium.org, pthatcher@webrtc.org

[zhihuang1, 2016-06-21 23:35:03]

Hi Tommi, 
We plan to change the default rtcp-mux policy to "require". Can you review these
one line changes? Thanks.

[tommi (sloooow) - chröme, 2016-06-22 14:46:00]

On 2016/06/21 23:35:03, zhihuang1 wrote:
> Hi Tommi, 
> We plan to change the default rtcp-mux policy to "require". Can you review
these
> one line changes? Thanks.

Is there a bug with more background?

[zhihuang1, 2016-06-22 22:43:12]

Description was changed from

==========
Change the default rtcp mux policy from negotiate to require.

This CL will be landed after monioring the UMA of rtcp mux policy.

BUG=
==========

to

==========
Change the default rtcp mux policy from negotiate to require.

This CL will be landed after monioring the UMA of rtcp mux policy.

BUG=webrtc:6030
==========

[zhihuang1, 2016-06-22 22:44:32]

On 2016/06/22 14:46:00, tommi-chrömium wrote:
> On 2016/06/21 23:35:03, zhihuang1 wrote:
> > Hi Tommi, 
> > We plan to change the default rtcp-mux policy to "require". Can you review
> these
> > one line changes? Thanks.
> 
> Is there a bug with more background?

We just created one for it. I have updated the issue description.

[tommi (sloooow) - chröme, 2016-06-23 07:47:24]

On 2016/06/22 22:44:32, zhihuang1 wrote:
> On 2016/06/22 14:46:00, tommi-chrömium wrote:
> > On 2016/06/21 23:35:03, zhihuang1 wrote:
> > > Hi Tommi, 
> > > We plan to change the default rtcp-mux policy to "require". Can you review
> > these
> > > one line changes? Thanks.
> > 
> > Is there a bug with more background?
> 
> We just created one for it. I have updated the issue description.

Great, thanks! 
Lgtm

[zhihuang1, 2016-11-21 23:15:10]

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

[commit-bot: I haz the power, 2016-11-21 23:16:07]

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

[honghaiz3, 2016-11-22 00:19:35]

honghaiz@webrtc.org changed reviewers:
+ honghaiz@webrtc.org

[honghaiz3, 2016-11-22 00:19:36]

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

https://codereview.chromium.org/2055553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:271:
rtcpMuxPolicy = WebRTCRtcpMuxPolicy::kRequire;
I think you need to move this line to the else clause and change it to 
rtcpMuxPolicy = WebRTCRtcpMuxPolicy::kNegotiate;

[zhihuang1, 2016-11-22 00:30:33]

Patchset #3 (id:40001) has been deleted

[zhihuang1, 2016-11-22 00:31:55]

zhihuang@chromium.org changed reviewers:
- tommi@chromium.org

[zhihuang1, 2016-11-22 00:31:55]

Please take another look.

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

https://codereview.chromium.org/2055553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:271:
rtcpMuxPolicy = WebRTCRtcpMuxPolicy::kRequire;
On 2016/11/22 00:19:35, honghaiz3 wrote:
> I think you need to move this line to the else clause and change it to 
> rtcpMuxPolicy = WebRTCRtcpMuxPolicy::kNegotiate; 

Ah, you are right. This is a silly mistake.

[zhihuang1, 2016-11-22 00:31:56]

Please take another look.

[honghaiz3, 2016-11-22 00:33:03]

lgtm

[zhihuang1, 2016-11-22 18:40:54]

The CQ bit was checked by zhihuang@chromium.org

[zhihuang1, 2016-11-22 18:40:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@chromium.org, tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/2055553003/#ps60001
(title: "Monior Fix.")

[commit-bot: I haz the power, 2016-11-22 18:41:15]

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-11-22 18:51:27]

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

[commit-bot: I haz the power, 2016-11-22 18:51:28]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[zhihuang1, 2016-11-22 19:04:08]

zhihuang@chromium.org changed reviewers:
+ foolip@chromium.org
- honghaiz@webrtc.org

[zhihuang1, 2016-11-22 19:04:08]

Hello,
I have a simple change on the the WebRTCConfiguration. Please take a look.
Thanks.

[pthatcher2, 2016-11-29 01:05:13]

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

[pthatcher2, 2016-11-29 01:05:13]

lgtm

[foolip, 2016-11-29 10:24:42]

> This CL will be landed after monioring the UMA of rtcp mux policy

Do you intend to land this CL soonish, and can you update the description to
include the conclusions from looking at UMA?

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

https://codereview.chromium.org/2055553003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:264: //
For the histogram value of "WebRTC.PeerConnection.SelectedRtcpMuxPolicy".
This histogram exists only to gauge the risk of this change I think. Can you
remove it in this CL too?

[zhihuang1, 2016-11-29 18:59:41]

Description was changed from

==========
Change the default rtcp mux policy from negotiate to require.

This CL will be landed after monioring the UMA of rtcp mux policy.

BUG=webrtc:6030
==========

to

==========
Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.

According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy. Among the users with media, only 3% users disabled the rtcp
mux. In conclusion, it's not risky to make this change.

The UMA "selectedRtcpMuxPolicy" is also removed from this CL since it is used to
gauge the risk of making the change.

BUG=webrtc:6030
==========

[zhihuang1, 2016-11-29 19:00:33]

Description was changed from

==========
Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.

According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy. Among the users with media, only 3% users disabled the rtcp
mux. In conclusion, it's not risky to make this change.

The UMA "selectedRtcpMuxPolicy" is also removed from this CL since it is used to
gauge the risk of making the change.

BUG=webrtc:6030
==========

to

==========
Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.

According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy. Among the users with media, only 3% users disabled the rtcp
mux. In conclusion, it's not risky to make this change.

The UMA "selectedRtcpMuxPolicy" is also removed in this CL since it is used to
gauge the risk of making the change.

BUG=webrtc:6030
==========

[zhihuang1, 2016-11-29 19:01:02]

Description was changed from

==========
Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.

According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy. Among the users with media, only 3% users disabled the rtcp
mux. In conclusion, it's not risky to make this change.

The UMA "selectedRtcpMuxPolicy" is also removed in this CL since it is used to
gauge the risk of making the change.

BUG=webrtc:6030
==========

to

==========
Change the default rtcp mux policy from negotiate to require.
After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.
According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy. Among the users with media, only 3% users disabled the rtcp
mux. In conclusion, it's not risky to make this change.
The UMA "selectedRtcpMuxPolicy" is also removed in this CL since it is used to
gauge the risk of making the change.

BUG=webrtc:6030
==========

[zhihuang1, 2016-11-29 19:09:39]

Description was changed from

==========
Change the default rtcp mux policy from negotiate to require.
After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.
According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy. Among the users with media, only 3% users disabled the rtcp
mux. In conclusion, it's not risky to make this change.
The UMA "selectedRtcpMuxPolicy" is also removed in this CL since it is used to
gauge the risk of making the change.

BUG=webrtc:6030
==========

to

==========
Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.
According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy. Among the users with media, only 3% users disabled the rtcp
mux. In conclusion, it's not risky to make this change.
The UMA "selectedRtcpMuxPolicy" is also removed in this CL since it is used to
gauge the risk of making the change.

BUG=webrtc:6030
==========

[zhihuang1, 2016-11-29 19:26:48]

Description was changed from

==========
Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.
According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy. Among the users with media, only 3% users disabled the rtcp
mux. In conclusion, it's not risky to make this change.
The UMA "selectedRtcpMuxPolicy" is also removed in this CL since it is used to
gauge the risk of making the change.

BUG=webrtc:6030
==========

to

==========
Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.
According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy. Among the users with media, only 3% users disabled the rtcp
mux. In conclusion, it's not risky to make this change.

BUG=webrtc:6030
==========

[zhihuang1, 2016-11-29 19:47:32]

Description was changed from

==========
Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.
According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy. Among the users with media, only 3% users disabled the rtcp
mux. In conclusion, it's not risky to make this change.

BUG=webrtc:6030
==========

to

==========
Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.
According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy. Among the users with media, only 3% users disabled the rtcp
mux. In conclusion, it's not risky to make this change.

The UMA "SelectedRtcpMuxPolicy" is removed in this CL since it is only used to
gauge the risk of this change.

BUG=webrtc:6030
==========

[zhihuang1, 2016-11-29 20:40:35]

Patchset #4 (id:80001) has been deleted

[zhihuang1, 2016-11-29 22:35:38]

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

[commit-bot: I haz the power, 2016-11-29 22:36:34]

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

[zhihuang1, 2016-11-29 22:47:23]

zhihuang@chromium.org changed reviewers:
+ mkwst@chromium.org
- pthatcher@chromium.org

[zhihuang1, 2016-11-29 22:47:24]

Hi,
Please take a look.
Thanks!

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

https://codereview.chromium.org/2055553003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:264: //
For the histogram value of "WebRTC.PeerConnection.SelectedRtcpMuxPolicy".
On 2016/11/29 10:24:42, foolip wrote:
> This histogram exists only to gauge the risk of this change I think. Can you
> remove it in this CL too?

Done.

[commit-bot: I haz the power, 2016-11-30 00:39:01]

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

[commit-bot: I haz the power, 2016-11-30 00:39:02]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[foolip, 2016-11-30 13:06:53]

On 2016/11/29 10:24:42, foolip wrote:
> > This CL will be landed after monioring the UMA of rtcp mux policy
> 
> Do you intend to land this CL soonish, and can you update the description to
> include the conclusions from looking at UMA?

How about this?

[zhihuang1, 2016-11-30 18:07:02]

On 2016/11/30 13:06:53, foolip wrote:
> On 2016/11/29 10:24:42, foolip wrote:
> > > This CL will be landed after monioring the UMA of rtcp mux policy
> > 
> > Do you intend to land this CL soonish, and can you update the description to
> > include the conclusions from looking at UMA?
> 
> How about this?

My plan is to land it soon since we have already made the changes in native c++
code.

The description has already been updated. Please take another look.

[foolip, 2016-12-02 10:14:24]

On 2016/11/30 18:07:02, zhihuang1 wrote:
> On 2016/11/30 13:06:53, foolip wrote:
> > On 2016/11/29 10:24:42, foolip wrote:
> > > > This CL will be landed after monioring the UMA of rtcp mux policy
> > > 
> > > Do you intend to land this CL soonish, and can you update the description
to
> > > include the conclusions from looking at UMA?
> > 
> > How about this?
> 
> My plan is to land it soon since we have already made the changes in native
c++
> code.
> 
> The description has already been updated. Please take another look.

What the UMA stats show is that in 99.96% of cases, no rtcpMuxPolicy dictionary
member was provided. It previously defaulted to "negotiate", and will now
default to "require", so the risk is with sites that depend on the default being
what it is.

After talking to hta@ it sounds like the risks here are small enough that it's
worth trying this change without measuring anything further, but an Intent to
Ship on blink-dev would be good. See
https://www.chromium.org/blink#launch-process for the template, and make sure to
elaborate on the compatibility risk, why it is probably low, and concrete
suggestions for how affected sites should adapt. (Use { rtcpMuxPolicy:
"negotiate" } to get the old behavior.)

[Zhi Huang, 2016-12-03 19:47:58]

Description was changed from

==========
Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.
According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy. Among the users with media, only 3% users disabled the rtcp
mux. In conclusion, it's not risky to make this change.

The UMA "SelectedRtcpMuxPolicy" is removed in this CL since it is only used to
gauge the risk of this change.

BUG=webrtc:6030
==========

to

==========
Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.
According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy which means nearly all the users who are using "Negotiated"
policy would be affected by this change. Because among all the users with media,
only 3%  disabled the rtcp mux so it's not risky to make this change.

The UMA "SelectedRtcpMuxPolicy" is removed in this CL since it is only used to
gauge the risk of this change.

BUG=webrtc:6030
==========

[Zhi Huang, 2016-12-03 20:01:36]

Description was changed from

==========
Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.
According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy which means nearly all the users who are using "Negotiated"
policy would be affected by this change. Because among all the users with media,
only 3%  disabled the rtcp mux so it's not risky to make this change.

The UMA "SelectedRtcpMuxPolicy" is removed in this CL since it is only used to
gauge the risk of this change.

BUG=webrtc:6030
==========

to

==========
Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.
According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy which means nearly all the users who are using "negotiate"
policy would be affected by this change. Because among all the users with media,
only 3%  disabled the rtcp mux so it's not risky to make this change.

The UMA "SelectedRtcpMuxPolicy" is removed in this CL since it is only used to
gauge the risk of this change.

BUG=webrtc:6030
==========

[zhihuang1, 2016-12-14 19:07:44]

zhihuang@chromium.org changed reviewers:
+ holte@chromium.org

[zhihuang1, 2016-12-14 19:07:46]

Hello,
Please take another look.
The "Intent to Ship" has got 3 LGTMs from Chromium api owners.

Thanks!

[foolip, 2016-12-14 19:24:47]

https://codereview.chromium.org/2055553003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2055553003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:263: if
(configuration.hasRtcpMuxPolicy()) {
Now that rtcpMuxPolicy has a default value in the IDL, you can remove this
check.

https://codereview.chromium.org/2055553003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:265: if
(rtcpMuxPolicyString != "require") {
To match the above structure, I'd do:

if (rtcpMuxPolicyString == "negotiate") {
  rtcpMuxPolicy = WebRTCRtcpMuxPolicy::kNegotiate;
} else {
  DCHECK_EQ(rtcpMuxPolicyString, "require");
}

[foolip, 2016-12-14 19:56:06]

Apart from those nits, can this change be tested in some way?

[zhihuang1, 2016-12-14 20:52:55]

On 2016/12/14 19:56:06, foolip wrote:
> Apart from those nits, can this change be tested in some way?

For the histogram part, I don't think we need test.

I'm not sure how the WebKit is tested. I didn't find any tests directly related
to "third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp". My
guess is that there are some integration tests which exercise all the
peerconnection related parts. If so, all the conditions should be covered
(default, "negotiate", "require"). 

Anyway, I can hold this CL and look into this if you think it is import to add
the tests before making the change.

[foolip, 2016-12-14 21:33:04]

On 2016/12/14 20:52:55, zhihuang1 wrote:
> On 2016/12/14 19:56:06, foolip wrote:
> > Apart from those nits, can this change be tested in some way?
> 
> For the histogram part, I don't think we need test.
> 
> I'm not sure how the WebKit is tested. I didn't find any tests directly
related
> to "third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp".
My
> guess is that there are some integration tests which exercise all the
> peerconnection related parts. If so, all the conditions should be covered
> (default, "negotiate", "require"). 
> 
> Anyway, I can hold this CL and look into this if you think it is import to add
> the tests before making the change.

In third_party/WebKit one can write *Test.cpp unit tests, but for web exposed
stuff more commonly layout tests. The relevant ones are in
LayoutTests/fast/peerconnection/ and LayoutTests/imported/wpt/webrtc/.

Presumably rtcpMuxPolicy has some observable effect if used with the JavaScript
API, but it could be that it's currently impossible to test because of some
mocking going on when running LayoutTests. Will add reviewers who might now.

[foolip, 2016-12-14 21:33:54]

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

[foolip, 2016-12-14 21:33:55]

guidou@, hbos@, see previous comment about whether this can be tested.

[Steven Holte, 2016-12-14 22:14:00]

https://codereview.chromium.org/2055553003/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2055553003/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:73427: -<histogram
name="WebRTC.PeerConnection.SelectedRtcpMuxPolicy"
Don't delete the histogram entry, instead add an <obsolete> tag.

[hbos_chromium, 2016-12-15 15:12:06]

On 2016/12/14 21:33:04, foolip wrote:
> On 2016/12/14 20:52:55, zhihuang1 wrote:
> > On 2016/12/14 19:56:06, foolip wrote:
> > > Apart from those nits, can this change be tested in some way?
> > 
> > For the histogram part, I don't think we need test.
> > 
> > I'm not sure how the WebKit is tested. I didn't find any tests directly
> related
> > to "third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp".
> My
> > guess is that there are some integration tests which exercise all the
> > peerconnection related parts. If so, all the conditions should be covered
> > (default, "negotiate", "require"). 
> > 
> > Anyway, I can hold this CL and look into this if you think it is import to
add
> > the tests before making the change.
> 
> In third_party/WebKit one can write *Test.cpp unit tests, but for web exposed
> stuff more commonly layout tests. The relevant ones are in
> LayoutTests/fast/peerconnection/ and LayoutTests/imported/wpt/webrtc/.
> 
> Presumably rtcpMuxPolicy has some observable effect if used with the
JavaScript
> API, but it could be that it's currently impossible to test because of some
> mocking going on when running LayoutTests. Will add reviewers who might now.

There are tests for being able to pass arguments, but not testing that the
result is correct or what the default value is.
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/peer...
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/imported/...

I believe all LayoutTests use MockWebRTCPeerConnectionHandler as their fake glue
to WebRTC. Which means that our WebRTC LayoutTests only tests the input
parameter validation logic of the top layer, not if WebRTC code is working as
intended or is glued correctly. If WebRTC code barfs at the parameters,
LayoutTests wouldn't find out. This separation is not good, but fixing that
shouldn't block changes like these.

When I've needed to test code all the way down I've used
https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_brows...
which uses
https://cs.chromium.org/chromium/src/chrome/test/data/webrtc/peerconnection.js
and friends.

But setting up a WebRTC call in an integration test seems like such an overkill
for something like this. We should file a bug about WebRTC LayoutTests and
mocking.

https://codereview.chromium.org/2055553003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2055553003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:264: if
(rtcpMuxPolicyString == "negotiate") {
This CL gives rtcpMuxPolicy a default value, but it's still not required, so I
believe it can be explicitly set to undefined, is this the case? If so we could
hit the DCHECK_EQ and crash in debug builds.

[foolip, 2016-12-15 15:26:03]

https://codereview.chromium.org/2055553003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2055553003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:264: if
(rtcpMuxPolicyString == "negotiate") {
On 2016/12/15 15:12:06, hbos_chromium wrote:
> This CL gives rtcpMuxPolicy a default value, but it's still not required, so I
> believe it can be explicitly set to undefined, is this the case? If so we
could
> hit the DCHECK_EQ and crash in debug builds.

If dictionary members have a default value, they can't be missing from the point
of view of the generated RTCConfiguration. Even if one uses { rtcpMuxPolicy:
undefined } explicitly, it's treated as missing and then gets the default value.

For required dictionary members, it can also never be missing, but for another
reason.

[foolip, 2016-12-15 15:26:50]

Oh, and based on what hbos@ said, LGTM to land this without any new tests. I
trust that there are tests somewhere down the layers for the difference between
negotiate and required :)

[hbos_chromium, 2016-12-15 15:31:46]

On 2016/12/15 15:26:50, foolip wrote:
> Oh, and based on what hbos@ said, LGTM to land this without any new tests. I
> trust that there are tests somewhere down the layers for the difference
between
> negotiate and required :)

Yeah, and that there isn't a bug in the translation of arguments from one layer
to the next. That's what you have to do :)

[hbos_chromium, 2016-12-15 15:31:55]

lgtm

https://codereview.chromium.org/2055553003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2055553003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:264: if
(rtcpMuxPolicyString == "negotiate") {
On 2016/12/15 15:26:03, foolip wrote:
> On 2016/12/15 15:12:06, hbos_chromium wrote:
> > This CL gives rtcpMuxPolicy a default value, but it's still not required, so
I
> > believe it can be explicitly set to undefined, is this the case? If so we
> could
> > hit the DCHECK_EQ and crash in debug builds.
> 
> If dictionary members have a default value, they can't be missing from the
point
> of view of the generated RTCConfiguration. Even if one uses { rtcpMuxPolicy:
> undefined } explicitly, it's treated as missing and then gets the default
value.
> 
> For required dictionary members, it can also never be missing, but for another
> reason.

Oh cool!

[zhihuang1, 2016-12-15 19:13:14]

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

[commit-bot: I haz the power, 2016-12-15 19:13:38]

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

[zhihuang1, 2016-12-15 19:14:27]

zhihuang@chromium.org changed reviewers:
- foolip@chromium.org, guidou@chromium.org, hbos@chromium.org,
mkwst@chromium.org

[zhihuang1, 2016-12-15 19:14:28]

Hi Steven,
I have change the histogram.
Please take another look.
Thanks.

https://codereview.chromium.org/2055553003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2055553003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:263: if
(configuration.hasRtcpMuxPolicy()) {
On 2016/12/14 19:24:47, foolip wrote:
> Now that rtcpMuxPolicy has a default value in the IDL, you can remove this
> check.

Done.

https://codereview.chromium.org/2055553003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:265: if
(rtcpMuxPolicyString != "require") {
On 2016/12/14 19:24:47, foolip wrote:
> To match the above structure, I'd do:
> 
> if (rtcpMuxPolicyString == "negotiate") {
>   rtcpMuxPolicy = WebRTCRtcpMuxPolicy::kNegotiate;
> } else {
>   DCHECK_EQ(rtcpMuxPolicyString, "require");
> }

Done.

[Steven Holte, 2016-12-15 20:23:42]

On 2016/12/15 19:14:28, zhihuang1 wrote:
> Hi Steven,
> I have change the histogram.
> Please take another look.
> Thanks.
> 
>
https://codereview.chromium.org/2055553003/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
> (right):
> 
>
https://codereview.chromium.org/2055553003/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:263: if
> (configuration.hasRtcpMuxPolicy()) {
> On 2016/12/14 19:24:47, foolip wrote:
> > Now that rtcpMuxPolicy has a default value in the IDL, you can remove this
> > check.
> 
> Done.
> 
>
https://codereview.chromium.org/2055553003/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:265: if
> (rtcpMuxPolicyString != "require") {
> On 2016/12/14 19:24:47, foolip wrote:
> > To match the above structure, I'd do:
> > 
> > if (rtcpMuxPolicyString == "negotiate") {
> >   rtcpMuxPolicy = WebRTCRtcpMuxPolicy::kNegotiate;
> > } else {
> >   DCHECK_EQ(rtcpMuxPolicyString, "require");
> > }
> 
> Done.

lgtm

[commit-bot: I haz the power, 2016-12-15 20:47:23]

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

[commit-bot: I haz the power, 2016-12-15 20:47:24]

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

[zhihuang1, 2016-12-16 18:29:05]

The CQ bit was checked by zhihuang@chromium.org

[zhihuang1, 2016-12-16 18:29:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@chromium.org, tommi@chromium.org, honghaiz@webrtc.org,
foolip@chromium.org, hbos@chromium.org
Link to the patchset: https://codereview.chromium.org/2055553003/#ps140001
(title: "Add obsolete tag to the histogram")

[commit-bot: I haz the power, 2016-12-16 18:29:53]

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

[zhihuang1, 2016-12-16 18:47:13]

zhihuang@chromium.org changed reviewers:
+ dmazzoni@chromium.org
- holte@chromium.org

[zhihuang1, 2016-12-16 18:47:13]

Hi, 
I have made some simple changes on components/test_runner.
Please take a look.
Thanks!

[commit-bot: I haz the power, 2016-12-16 18:50:00]

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

[commit-bot: I haz the power, 2016-12-16 18:50:02]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[zhihuang1, 2016-12-17 20:00:38]

Patchset #7 (id:160001) has been deleted

[zhihuang1, 2016-12-18 19:02:47]

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

[commit-bot: I haz the power, 2016-12-18 19:02:52]

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

[zhihuang1, 2016-12-18 19:02:53]

The CQ bit was unchecked by zhihuang@chromium.org

[zhihuang1, 2016-12-19 01:31:45]

Patchset #7 (id:180001) has been deleted

[zhihuang1, 2016-12-19 01:31:54]

Patchset #7 (id:200001) has been deleted

[zhihuang1, 2016-12-19 07:41:00]

Patchset #8 (id:240001) has been deleted

[zhihuang1, 2016-12-19 07:41:06]

Patchset #7 (id:220001) has been deleted

[zhihuang1, 2016-12-19 18:26:46]

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

[commit-bot: I haz the power, 2016-12-19 18:27:08]

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-12-19 20:13:51]

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

[commit-bot: I haz the power, 2016-12-19 20:13:52]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2016-12-19 21:38:19]

components/test_runner lgtm

[zhihuang1, 2016-12-20 18:27:22]

The CQ bit was checked by zhihuang@chromium.org

[zhihuang1, 2016-12-20 18:27:23]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
honghaiz@webrtc.org, holte@chromium.org, pthatcher@chromium.org,
hbos@chromium.org, foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2055553003/#ps260001
(title: "fix test")

[commit-bot: I haz the power, 2016-12-20 18:27:59]

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-12-20 22:39:38]

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

[commit-bot: I haz the power, 2016-12-20 22:39:40]

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

[zhihuang1, 2016-12-20 23:28:47]

The CQ bit was checked by zhihuang@chromium.org

[commit-bot: I haz the power, 2016-12-20 23:29:53]

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-12-21 00:47:27]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1482276527765670,
"parent_rev": "cf39daf1f2c5c3b2bb76280a94fb41befb6b2180", "commit_rev":
"3a8cfebe33388f12f24a1ca61e63db96f6383d61"}

[commit-bot: I haz the power, 2016-12-21 00:47:58]

Description was changed from

==========
Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.
According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy which means nearly all the users who are using "negotiate"
policy would be affected by this change. Because among all the users with media,
only 3%  disabled the rtcp mux so it's not risky to make this change.

The UMA "SelectedRtcpMuxPolicy" is removed in this CL since it is only used to
gauge the risk of this change.

BUG=webrtc:6030
==========

to

==========
Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.
According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy which means nearly all the users who are using "negotiate"
policy would be affected by this change. Because among all the users with media,
only 3%  disabled the rtcp mux so it's not risky to make this change.

The UMA "SelectedRtcpMuxPolicy" is removed in this CL since it is only used to
gauge the risk of this change.

BUG=webrtc:6030

Review-Url: https://codereview.chromium.org/2055553003
==========

[commit-bot: I haz the power, 2016-12-21 00:47:59]

Committed patchset #7 (id:260001)

[commit-bot: I haz the power, 2016-12-21 00:50:36]

Description was changed from

==========
Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.
According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy which means nearly all the users who are using "negotiate"
policy would be affected by this change. Because among all the users with media,
only 3%  disabled the rtcp mux so it's not risky to make this change.

The UMA "SelectedRtcpMuxPolicy" is removed in this CL since it is only used to
gauge the risk of this change.

BUG=webrtc:6030

Review-Url: https://codereview.chromium.org/2055553003
==========

to

==========
Change the default rtcp mux policy from negotiate to require.

After monitoring the UMA stats, the plan is to change the default rtcp mux
policy from negotiated to require.
According to the UMA data in 28 days, 99.96% users choose the default value of
rtcp mux policy which means nearly all the users who are using "negotiate"
policy would be affected by this change. Because among all the users with media,
only 3%  disabled the rtcp mux so it's not risky to make this change.

The UMA "SelectedRtcpMuxPolicy" is removed in this CL since it is only used to
gauge the risk of this change.

BUG=webrtc:6030

Committed: https://crrev.com/3291edb6105e4f7e39a0b4e81da9c24e234ad185
Cr-Commit-Position: refs/heads/master@{#439955}
==========

[commit-bot: I haz the power, 2016-12-21 00:50:37]

Patchset 7 (id:??) landed as
https://crrev.com/3291edb6105e4f7e39a0b4e81da9c24e234ad185
Cr-Commit-Position: refs/heads/master@{#439955}