Relanding: Move "max IPv6 networks" logic to BasicPortAllocator, and fix sorting.

Relanding: Move "max IPv6 networks" logic to BasicPortAllocator, and fix sorting.

Relanding because the broken chromium test has been fixed:
https://chromium-review.googlesource.com/582196

This CL moves the responsibility for restricting the number of IPv6
interfaces used for ICE to BasicPortAllocator. This is the right place
to do it in the first place; it's where all the rest of the filtering
occurs. And NetworkManager shouldn't need to know about ICE limitations;
only the ICE classes should.

Part of the reason I'm doing this is that I want to add a
"max_ipv6_networks" API to RTCConfiguration, so that applications can
override the default easily (see linked bug). But that means that
PeerConnection would need to be able to call "set_max_ipv6_networks" on
the underlying object that does the filtering, and that method isn't
available on the "NetworkManager" base class. So rather than adding
another method to a place it doesn't belong, I'm moving it to the place
it does belong.

In the process, I noticed that "CompareNetworks" is inconsistent with
"SortNetworks"; the former orders interfaces alphabetically, and the
latter reverse-alphabetically. I believe this was unintentional, and
results in undesirable behavior (like "eth1" being preferred over
"eth0"), so I'm fixing it and adding a test.

BUG=webrtc:7703
Review-Url: https://codereview.webrtc.org/2983213002
Cr-Original-Commit-Position: refs/heads/master@{#19112}
Committed: https://chromium.googlesource.com/external/webrtc/+/ad9561404c0cc007574eaedade581928aa3d24ef
Review-Url: https://codereview.webrtc.org/2983213002
Cr-Commit-Position: refs/heads/master@{#19159}
Committed: https://chromium.googlesource.com/external/webrtc/+/3427f538de4fce254ae6625d57b87aa4c6f66f5c

[Taylor Brandstetter, 2017-07-20 23:36:10]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org, zhihuang@webrtc.org

[Taylor Brandstetter, 2017-07-20 23:36:12]

Zhi, could you take a look at this since Peter is OOO?

https://codereview.webrtc.org/2983213002/diff/1/webrtc/rtc_base/network_unitt...
File webrtc/rtc_base/network_unittest.cc (left):

https://codereview.webrtc.org/2983213002/diff/1/webrtc/rtc_base/network_unitt...
webrtc/rtc_base/network_unittest.cc:471: }
This functionality is now covered by the two new BasicPortAllocator tests.

[Zhi Huang, 2017-07-21 06:11:50]

On 2017/07/20 23:36:12, Taylor Brandstetter wrote:
> Zhi, could you take a look at this since Peter is OOO?
> 
>
https://codereview.webrtc.org/2983213002/diff/1/webrtc/rtc_base/network_unitt...
> File webrtc/rtc_base/network_unittest.cc (left):
> 
>
https://codereview.webrtc.org/2983213002/diff/1/webrtc/rtc_base/network_unitt...
> webrtc/rtc_base/network_unittest.cc:471: }
> This functionality is now covered by the two new BasicPortAllocator tests.

lgtm.

[Zhi Huang, 2017-07-21 06:13:46]

https://codereview.webrtc.org/2983213002/diff/1/webrtc/p2p/base/portallocator.h
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/2983213002/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:110: constexpr int kDefaultMaxIPv6Networks = 5;
Maybe use uint32_t like other constants?

[Taylor Brandstetter, 2017-07-21 16:18:18]

https://codereview.webrtc.org/2983213002/diff/1/webrtc/p2p/base/portallocator.h
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/2983213002/diff/1/webrtc/p2p/base/portallocator...
webrtc/p2p/base/portallocator.h:110: constexpr int kDefaultMaxIPv6Networks = 5;
On 2017/07/21 06:13:45, Zhi Huang wrote:
> Maybe use uint32_t like other constants?

It was an int before, and it's generally preferable to use ints:
https://google.github.io/styleguide/cppguide.html#Integer_Types

[Taylor Brandstetter, 2017-07-21 16:18:21]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2017-07-21 16:18:30]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-07-21 16:26:59]

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

[commit-bot: I haz the power, 2017-07-21 16:27:01]

Try jobs failed on following builders:
  linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
  win_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/28018)
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/27401)

[Taylor Brandstetter, 2017-07-21 17:21:30]

The CQ bit was checked by deadbeef@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-21 17:21:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Taylor Brandstetter, 2017-07-21 17:22:58]

I had to fix an ICE test that was relying on behavior it shouldn't have been
(that IP addresses were ordered relative to each other). Can you take another
quick look before I submit?

[commit-bot: I haz the power, 2017-07-21 17:50:29]

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

[commit-bot: I haz the power, 2017-07-21 17:50:30]

Dry run: This issue passed the CQ dry run.

[Zhi Huang, 2017-07-21 17:52:59]

On 2017/07/21 17:22:58, Taylor Brandstetter wrote:
> I had to fix an ICE test that was relying on behavior it shouldn't have been
> (that IP addresses were ordered relative to each other). Can you take another
> quick look before I submit?

updated tests lgtm.

[Taylor Brandstetter, 2017-07-21 18:00:19]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2017-07-21 18:00:30]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-07-21 18:03:53]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1500660019869850,
"parent_rev": "a3251dd83f2e3d988e32504a62ec2cf9bba06585", "commit_rev":
"ad9561404c0cc007574eaedade581928aa3d24ef"}

[commit-bot: I haz the power, 2017-07-21 18:04:01]

Description was changed from

==========
Move "max IPv6 networks" logic to BasicPortAllocator, and fix sorting.

This CL moves the responsibility for restricting the number of IPv6
interfaces used for ICE to BasicPortAllocator. This is the right place
to do it in the first place; it's where all the rest of the filtering
occurs. And NetworkManager shouldn't need to know about ICE limitations;
only the ICE classes should.

Part of the reason I'm doing this is that I want to add a
"max_ipv6_networks" API to RTCConfiguration, so that applications can
override the default easily (see linked bug). But that means that
PeerConnection would need to be able to call "set_max_ipv6_networks" on
the underlying object that does the filtering, and that method isn't
available on the "NetworkManager" base class. So rather than adding
another method to a place it doesn't belong, I'm moving it to the place
it does belong.

In the process, I noticed that "CompareNetworks" is inconsistent with
"SortNetworks"; the former orders interfaces alphabetically, and the
latter reverse-alphabetically. I believe this was unintentional, and
results in undesirable behavior (like "eth1" being preferred over
"eth0"), so I'm fixing it and adding a test.

BUG=webrtc:7703
==========

to

==========
Move "max IPv6 networks" logic to BasicPortAllocator, and fix sorting.

This CL moves the responsibility for restricting the number of IPv6
interfaces used for ICE to BasicPortAllocator. This is the right place
to do it in the first place; it's where all the rest of the filtering
occurs. And NetworkManager shouldn't need to know about ICE limitations;
only the ICE classes should.

Part of the reason I'm doing this is that I want to add a
"max_ipv6_networks" API to RTCConfiguration, so that applications can
override the default easily (see linked bug). But that means that
PeerConnection would need to be able to call "set_max_ipv6_networks" on
the underlying object that does the filtering, and that method isn't
available on the "NetworkManager" base class. So rather than adding
another method to a place it doesn't belong, I'm moving it to the place
it does belong.

In the process, I noticed that "CompareNetworks" is inconsistent with
"SortNetworks"; the former orders interfaces alphabetically, and the
latter reverse-alphabetically. I believe this was unintentional, and
results in undesirable behavior (like "eth1" being preferred over
"eth0"), so I'm fixing it and adding a test.

BUG=webrtc:7703

Review-Url: https://codereview.webrtc.org/2983213002
Cr-Commit-Position: refs/heads/master@{#19112}
Committed:
https://chromium.googlesource.com/external/webrtc/+/ad9561404c0cc007574eaedad...
==========

[commit-bot: I haz the power, 2017-07-21 18:04:02]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/ad9561404c0cc007574eaedad...

[Taylor Brandstetter, 2017-07-21 19:59:32]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.webrtc.org/2984853002/ by deadbeef@webrtc.org.

The reason for reverting is: Breaks IpcNetworkManagerTest.TestMergeNetworkList,
because it has built-in assumptions about network ordering that it shouldn't
have. Will reland after fixing that test..

[Taylor Brandstetter, 2017-07-26 22:12:44]

Description was changed from

==========
Move "max IPv6 networks" logic to BasicPortAllocator, and fix sorting.

This CL moves the responsibility for restricting the number of IPv6
interfaces used for ICE to BasicPortAllocator. This is the right place
to do it in the first place; it's where all the rest of the filtering
occurs. And NetworkManager shouldn't need to know about ICE limitations;
only the ICE classes should.

Part of the reason I'm doing this is that I want to add a
"max_ipv6_networks" API to RTCConfiguration, so that applications can
override the default easily (see linked bug). But that means that
PeerConnection would need to be able to call "set_max_ipv6_networks" on
the underlying object that does the filtering, and that method isn't
available on the "NetworkManager" base class. So rather than adding
another method to a place it doesn't belong, I'm moving it to the place
it does belong.

In the process, I noticed that "CompareNetworks" is inconsistent with
"SortNetworks"; the former orders interfaces alphabetically, and the
latter reverse-alphabetically. I believe this was unintentional, and
results in undesirable behavior (like "eth1" being preferred over
"eth0"), so I'm fixing it and adding a test.

BUG=webrtc:7703

Review-Url: https://codereview.webrtc.org/2983213002
Cr-Commit-Position: refs/heads/master@{#19112}
Committed:
https://chromium.googlesource.com/external/webrtc/+/ad9561404c0cc007574eaedad...
==========

to

==========
Relanding: Move "max IPv6 networks" logic to BasicPortAllocator, and fix
sorting.

Relanding because the broken chromium test has been fixed:
https://chromium-review.googlesource.com/582196

This CL moves the responsibility for restricting the number of IPv6
interfaces used for ICE to BasicPortAllocator. This is the right place
to do it in the first place; it's where all the rest of the filtering
occurs. And NetworkManager shouldn't need to know about ICE limitations;
only the ICE classes should.

Part of the reason I'm doing this is that I want to add a
"max_ipv6_networks" API to RTCConfiguration, so that applications can
override the default easily (see linked bug). But that means that
PeerConnection would need to be able to call "set_max_ipv6_networks" on
the underlying object that does the filtering, and that method isn't
available on the "NetworkManager" base class. So rather than adding
another method to a place it doesn't belong, I'm moving it to the place
it does belong.

In the process, I noticed that "CompareNetworks" is inconsistent with
"SortNetworks"; the former orders interfaces alphabetically, and the
latter reverse-alphabetically. I believe this was unintentional, and
results in undesirable behavior (like "eth1" being preferred over
"eth0"), so I'm fixing it and adding a test.

BUG=webrtc:7703

Review-Url: https://codereview.webrtc.org/2983213002
Cr-Commit-Position: refs/heads/master@{#19112}
Committed:
https://chromium.googlesource.com/external/webrtc/+/ad9561404c0cc007574eaedad...
==========

[Taylor Brandstetter, 2017-07-26 22:13:12]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2017-07-26 22:13:18]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-07-26 23:09:33]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1501107192848430,
"parent_rev": "58f1725ff1feeb665aaa1bc222e6d36148d8fcb7", "commit_rev":
"3427f538de4fce254ae6625d57b87aa4c6f66f5c"}

[commit-bot: I haz the power, 2017-07-26 23:09:39]

Description was changed from

==========
Relanding: Move "max IPv6 networks" logic to BasicPortAllocator, and fix
sorting.

Relanding because the broken chromium test has been fixed:
https://chromium-review.googlesource.com/582196

This CL moves the responsibility for restricting the number of IPv6
interfaces used for ICE to BasicPortAllocator. This is the right place
to do it in the first place; it's where all the rest of the filtering
occurs. And NetworkManager shouldn't need to know about ICE limitations;
only the ICE classes should.

Part of the reason I'm doing this is that I want to add a
"max_ipv6_networks" API to RTCConfiguration, so that applications can
override the default easily (see linked bug). But that means that
PeerConnection would need to be able to call "set_max_ipv6_networks" on
the underlying object that does the filtering, and that method isn't
available on the "NetworkManager" base class. So rather than adding
another method to a place it doesn't belong, I'm moving it to the place
it does belong.

In the process, I noticed that "CompareNetworks" is inconsistent with
"SortNetworks"; the former orders interfaces alphabetically, and the
latter reverse-alphabetically. I believe this was unintentional, and
results in undesirable behavior (like "eth1" being preferred over
"eth0"), so I'm fixing it and adding a test.

BUG=webrtc:7703

Review-Url: https://codereview.webrtc.org/2983213002
Cr-Commit-Position: refs/heads/master@{#19112}
Committed:
https://chromium.googlesource.com/external/webrtc/+/ad9561404c0cc007574eaedad...
==========

to

==========
Relanding: Move "max IPv6 networks" logic to BasicPortAllocator, and fix
sorting.

Relanding because the broken chromium test has been fixed:
https://chromium-review.googlesource.com/582196

This CL moves the responsibility for restricting the number of IPv6
interfaces used for ICE to BasicPortAllocator. This is the right place
to do it in the first place; it's where all the rest of the filtering
occurs. And NetworkManager shouldn't need to know about ICE limitations;
only the ICE classes should.

Part of the reason I'm doing this is that I want to add a
"max_ipv6_networks" API to RTCConfiguration, so that applications can
override the default easily (see linked bug). But that means that
PeerConnection would need to be able to call "set_max_ipv6_networks" on
the underlying object that does the filtering, and that method isn't
available on the "NetworkManager" base class. So rather than adding
another method to a place it doesn't belong, I'm moving it to the place
it does belong.

In the process, I noticed that "CompareNetworks" is inconsistent with
"SortNetworks"; the former orders interfaces alphabetically, and the
latter reverse-alphabetically. I believe this was unintentional, and
results in undesirable behavior (like "eth1" being preferred over
"eth0"), so I'm fixing it and adding a test.

BUG=webrtc:7703

Review-Url: https://codereview.webrtc.org/2983213002
Cr-Original-Commit-Position: refs/heads/master@{#19112}
Committed:
https://chromium.googlesource.com/external/webrtc/+/ad9561404c0cc007574eaedad...
Review-Url: https://codereview.webrtc.org/2983213002
Cr-Commit-Position: refs/heads/master@{#19159}
Committed:
https://chromium.googlesource.com/external/webrtc/+/3427f538de4fce254ae6625d5...
==========

[commit-bot: I haz the power, 2017-07-26 23:09:41]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/3427f538de4fce254ae6625d5...