Add a flag to filter out high-cost networks.

Add a flag to filter out high-cost networks.
This allows webrtc to not gather on cellular networks if wifi or
other low cost networks are present.
BUG=

Committed: https://crrev.com/603470576ec4043e868c72f38bf7644831a38dc3
Cr-Commit-Position: refs/heads/master@{#12979}

[honghaiz3, 2016-05-17 19:23:06]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2016-05-17 19:23:13]

Patchset #1 (id:20001) has been deleted

[honghaiz3, 2016-05-17 19:23:19]

Patchset #1 (id:40001) has been deleted

[honghaiz3, 2016-05-17 19:23:48]

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

[pthatcher1, 2016-05-17 20:44:28]

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:2118: if
(configuration.gather_low_cost_network_only) {
It seems like we should make this an enum policy like tcp_candidate_policy. 
Perhaps candidate_network_policy == kCandidateNetworkPolicyLowCost (default
would be kCandidateNetworkPolicyAll

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/base/network.h#new...
webrtc/base/network.h:41: static const uint16_t kNetworkCostForUnknownType = 10;
I think kNetworkCostMax and kNetworkCostUnknown  would be a good pair of names.

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/base/network.h#new...
webrtc/base/network.h:356: return kMaxNetworkCost;
Should we use kNetworkCostHigh == 900 or so just in case later we want something
with even higher cost?

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/base/network.h#new...
webrtc/base/network.h:358: return 0;
Wouldn't it be better to have the default be "unknown", with something like
this?

case rtc::ADAPTER_TYPE_CELLULAR:
   return kNetworkCostHigh;
case rtc::ADAPTER_TYPE_WIFI:
   return kNetworkCostLow;
case rtc::ADAPTER_TYPE_ETHERNET:
     return kNetworkCostMin;
default:
     return kNetworkCostUnknown;

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/base/network.h#new...
webrtc/base/network.h:358: return 0;
Would a kNetworkCostMin be a good constant?

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/base/network.h#new...
webrtc/base/network.h:359: }
Would it make sense to make WiFi kNetworkCostLow == 1 an Ethernet
kNetworkCostMin so we slightly prefer Ethernet over WiFi?

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/base/portalloc...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:66: // the lowest cost. So if both Wifi and
Cellular networks exist, gather
Cellular => cellular 
Wifi => wifi (or Wi-Fi)

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:69: // exists, Cellular networks will not be
gathered on and if a Wifi network is
gathered on => gathered on it

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:70: // present, "unknown" networks will not be
gathered on. Doing so ensures that
networks will not be gathered on => networks will not be used for gathering

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:73: PORTALLOCATOR_GATHER_LOW_COST_NETWORK_ONLY =
0x2000,
Following the existing naming scheme, would this be
PORTALLOCATOR_DISABLE_COSTLY_NETWORKS?

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:404: }
std::min might work here

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:409: networks->end());
We could use a threshold greater than zero (perhaps 50), such as "return
network->GetCost() > lowest_cost + kNetworkCostLow"

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator_unittest.cc (right):

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator_unittest.cc:502: // Unknown interfaces.
Unknown => unknown
 (and Cellular and Wifi here also)

[honghaiz3, 2016-05-18 05:55:03]

PTAL.

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:2118: if
(configuration.gather_low_cost_network_only) {
On 2016/05/17 20:44:27, pthatcher1 wrote:
> It seems like we should make this an enum policy like tcp_candidate_policy. 
> Perhaps candidate_network_policy == kCandidateNetworkPolicyLowCost (default
> would be kCandidateNetworkPolicyAll

Done.

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/base/network.h#new...
webrtc/base/network.h:41: static const uint16_t kNetworkCostForUnknownType = 10;
On 2016/05/17 20:44:28, pthatcher1 wrote:
> I think kNetworkCostMax and kNetworkCostUnknown  would be a good pair of
names. 
> 

Done.

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/base/network.h#new...
webrtc/base/network.h:356: return kMaxNetworkCost;
On 2016/05/17 20:44:28, pthatcher1 wrote:
> Should we use kNetworkCostHigh == 900 or so just in case later we want
something
> with even higher cost?

Done.

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/base/network.h#new...
webrtc/base/network.h:356: return kMaxNetworkCost;
On 2016/05/17 20:44:28, pthatcher1 wrote:
> Should we use kNetworkCostHigh == 900 or so just in case later we want
something
> with even higher cost?
Done.
I initially chose cellular network cost to be the maximum cost 999 so that one
side can not choose a higher value (say 901) than the other side (choosing 900)
to use the other-side's cellular more often (when both cell-wifi and wifi-cell
connections exist). That may rarely happen because in most cases, we (webrtc)
control both sides. If you think that may still be an issue, let me know. I can
change it back.

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/base/network.h#new...
webrtc/base/network.h:358: return 0;
On 2016/05/17 20:44:28, pthatcher1 wrote:
> Would a kNetworkCostMin be a good constant?

Done.

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/base/network.h#new...
webrtc/base/network.h:358: return 0;
On 2016/05/17 20:44:28, pthatcher1 wrote:
> Wouldn't it be better to have the default be "unknown", with something like
> this?
> 
> case rtc::ADAPTER_TYPE_CELLULAR:
>    return kNetworkCostHigh;
> case rtc::ADAPTER_TYPE_WIFI:
>    return kNetworkCostLow;
> case rtc::ADAPTER_TYPE_ETHERNET:
>      return kNetworkCostMin;
> default:
>      return kNetworkCostUnknown;

Done. I just listed all types we may have now.

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/base/network.h#new...
webrtc/base/network.h:359: }
On 2016/05/17 20:44:28, pthatcher1 wrote:
> Would it make sense to make WiFi kNetworkCostLow == 1 an Ethernet
> kNetworkCostMin so we slightly prefer Ethernet over WiFi?

I choose Wifi cost to be 10 to have slightly more difference, although it won't
matter for now.
May be easier in the future if we need to add rtt as part of the cost.

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/base/portalloc...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:66: // the lowest cost. So if both Wifi and
Cellular networks exist, gather
On 2016/05/17 20:44:28, pthatcher1 wrote:
> Cellular => cellular 
> Wifi => wifi (or Wi-Fi)

Done.

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:69: // exists, Cellular networks will not be
gathered on and if a Wifi network is
On 2016/05/17 20:44:28, pthatcher1 wrote:
> gathered on => gathered on it

Revised this.

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:70: // present, "unknown" networks will not be
gathered on. Doing so ensures that
On 2016/05/17 20:44:28, pthatcher1 wrote:
> networks will not be gathered on => networks will not be used for gathering

Done.

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:73: PORTALLOCATOR_GATHER_LOW_COST_NETWORK_ONLY =
0x2000,
On 2016/05/17 20:44:28, pthatcher1 wrote:
> Following the existing naming scheme, would this be
> PORTALLOCATOR_DISABLE_COSTLY_NETWORKS?
> 

Done.

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:404: }
On 2016/05/17 20:44:28, pthatcher1 wrote:
> std::min might work here

Done.

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:409: networks->end());
On 2016/05/17 20:44:28, pthatcher1 wrote:
> We could use a threshold greater than zero (perhaps 50), such as "return
> network->GetCost() > lowest_cost + kNetworkCostLow"

Done.

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator_unittest.cc (right):

https://codereview.webrtc.org/1987833002/diff/60001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator_unittest.cc:502: // Unknown interfaces.
On 2016/05/17 20:44:28, pthatcher1 wrote:
> Unknown => unknown
>  (and Cellular and Wifi here also)

Done.

[honghaiz3, 2016-05-19 18:06:49]

Please hold this until the other one
https://codereview.webrtc.org/1976683003/ is resolved because these two CLs have
several duplicate changes.

[honghaiz3, 2016-05-23 16:54:09]

Patchset #3 (id:100001) has been deleted

[honghaiz3, 2016-05-23 16:54:21]

Patchset #3 (id:120001) has been deleted

[honghaiz3, 2016-05-23 18:10:07]

It has been merged with the head. PTAL.

[honghaiz3, 2016-05-23 18:10:15]

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

[commit-bot: I haz the power, 2016-05-23 18:10:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987833002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1987833002/140001

[honghaiz3, 2016-05-31 18:59:57]

Just a friendly reminder for the review.

[pthatcher1, 2016-05-31 19:59:43]

lgtm

[honghaiz3, 2016-05-31 23:31:28]

Patchset #6 (id:200001) has been deleted

[honghaiz3, 2016-05-31 23:34:58]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-05-31 23:34:59]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1987833002/#ps220001 (title:
"merge with head")

[commit-bot: I haz the power, 2016-05-31 23:35:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987833002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1987833002/220001

[commit-bot: I haz the power, 2016-06-01 01:29:17]

Committed patchset #6 (id:220001)

[commit-bot: I haz the power, 2016-06-01 01:29:26]

Description was changed from

==========
Add a flag to filter out high-cost networks.
This allows webrtc to not gather on cellular networks if wifi or
other low cost networks are present.
BUG=
==========

to

==========
Add a flag to filter out high-cost networks.
This allows webrtc to not gather on cellular networks if wifi or
other low cost networks are present.
BUG=

Committed: https://crrev.com/603470576ec4043e868c72f38bf7644831a38dc3
Cr-Commit-Position: refs/heads/master@{#12979}
==========

[commit-bot: I haz the power, 2016-06-01 01:29:27]

Patchset 6 (id:??) landed as
https://crrev.com/603470576ec4043e868c72f38bf7644831a38dc3
Cr-Commit-Position: refs/heads/master@{#12979}