WebRTC should generate default private address even when adapter enumeration is disabled.

WebRTC should generate default private address even when adapter enumeration is disabled.

Introduce a DefaultAddressProvider such that rtc::Network can't access other part of NetworkManager.

This also removes the hack of generating the loopback address. The dependency has been removed by https://codereview.chromium.org/1417023003/

BUG=webrtc:5061
R=pthatcher@webrtc.org

Committed: https://crrev.com/9af97f89103d8f1f77b52a6ae77b8b7bcdc23f71
Cr-Commit-Position: refs/heads/master@{#10590}

[guoweis_webrtc, 2015-10-27 22:02:04]

Description was changed from

==========
WebRTC should generate default private address even when adapter enumeration is
disabled.

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

to

==========
WebRTC should generate default private address even when adapter enumeration is
disabled.

This also removes the hack of generating the loopback address. The dependency
has been removed by https://codereview.chromium.org/1417023003/

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

[guoweis_webrtc, 2015-10-27 22:03:11]

Description was changed from

==========
WebRTC should generate default private address even when adapter enumeration is
disabled.

This also removes the hack of generating the loopback address. The dependency
has been removed by https://codereview.chromium.org/1417023003/

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

to

==========
WebRTC should generate default private address even when adapter enumeration is
disabled.

Introduce a DefaultAddressProvider such that rtc::Network can't access other
part of NetworkManager.

This also removes the hack of generating the loopback address. The dependency
has been removed by https://codereview.chromium.org/1417023003/

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

[guoweis_webrtc, 2015-10-28 05:50:42]

Description was changed from

==========
WebRTC should generate default private address even when adapter enumeration is
disabled.

Introduce a DefaultAddressProvider such that rtc::Network can't access other
part of NetworkManager.

This also removes the hack of generating the loopback address. The dependency
has been removed by https://codereview.chromium.org/1417023003/

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

to

==========
WebRTC should generate default private address even when adapter enumeration is
disabled.

Introduce a DefaultAddressProvider such that rtc::Network can't access other
part of NetworkManager.

This also removes the hack of generating the loopback address. The dependency
has been removed by https://codereview.chromium.org/1417023003/

BUG=webrtc:5061
==========

[guoweis_webrtc, 2015-10-28 05:51:06]

guoweis@webrtc.org changed reviewers:
+ juberti@webrtc.org, pthatcher@webrtc.org

[guoweis_webrtc, 2015-10-28 05:51:07]

PTAL.

[juberti, 2015-10-28 23:57:39]

juberti@google.com changed reviewers:
+ juberti@google.com

[juberti, 2015-10-28 23:57:40]

Reviewed the first half of the code. Main concern is around the sync socket call
in NetworkManager.

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/ipaddress.h
File webrtc/base/ipaddress.h (right):

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/ipaddress.h#...
webrtc/base/ipaddress.h:183: IPAddress GetWildcardIP(int family);
probably should call this GetAnyIP, to match the IsAny API above

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc
File webrtc/base/network.cc (right):

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
webrtc/base/network.cc:154: char kPublicIPv4Host[] = "8.8.8.8";
const char.

This also needs explanatory comments.

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
webrtc/base/network.cc:754: IPAddress
BasicNetworkManager::UpdateDefaultPrivateAddress(int family) {
This isn't really an Update, since it doesn't change any state.

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
webrtc/base/network.cc:763: family == AF_INET ? kPublicIPv4Host :
kPublicIPv6Host, 3478)) == 0) {
3478 should be a constant as well. I would pick the DNS port, given the choice
of addresses.

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
webrtc/base/network.cc:764: return socket->GetLocalAddress().ipaddr();
Not sure this will work in Chromium, where GetLocalAddress isn't filled in until
the OnAddressReady callback. This may need to be async.

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.h#ne...
webrtc/base/network.h:31: extern char kPublicIPv4Host[];
Do these need to be exported? They seem like implementation details.

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.h#ne...
webrtc/base/network.h:60: virtual bool GetDefaultPrivateAddress(int family,
The address may not be private. Perhaps GetDefaultLocalAddress, to match
AsyncSocket::GetLocalAddress?

Some commentary on what "default" means in this context would also be helpful.

[guoweis_webrtc, 2015-10-29 03:41:00]

PTAL.

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/ipaddress.h
File webrtc/base/ipaddress.h (right):

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/ipaddress.h#...
webrtc/base/ipaddress.h:183: IPAddress GetWildcardIP(int family);
On 2015/10/28 23:57:40, juberti wrote:
> probably should call this GetAnyIP, to match the IsAny API above

Done.

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc
File webrtc/base/network.cc (right):

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
webrtc/base/network.cc:154: char kPublicIPv4Host[] = "8.8.8.8";
On 2015/10/28 23:57:40, juberti wrote:
> const char.
> 
> This also needs explanatory comments.

Done.

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
webrtc/base/network.cc:754: IPAddress
BasicNetworkManager::UpdateDefaultPrivateAddress(int family) {
On 2015/10/28 23:57:40, juberti wrote:
> This isn't really an Update, since it doesn't change any state.

renamed as QueryDefaultAddress

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
webrtc/base/network.cc:763: family == AF_INET ? kPublicIPv4Host :
kPublicIPv6Host, 3478)) == 0) {
On 2015/10/28 23:57:40, juberti wrote:
> 3478 should be a constant as well. I would pick the DNS port, given the choice
> of addresses.

Done.

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
webrtc/base/network.cc:764: return socket->GetLocalAddress().ipaddr();
On 2015/10/28 23:57:40, juberti wrote:
> Not sure this will work in Chromium, where GetLocalAddress isn't filled in
until
> the OnAddressReady callback. This may need to be async.

Not an issue since IpcNetworkManager will be used instead.

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.h#ne...
webrtc/base/network.h:31: extern char kPublicIPv4Host[];
On 2015/10/28 23:57:40, juberti wrote:
> Do these need to be exported? They seem like implementation details.

I made them exported so chrome could use the same IP here.

https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.h#ne...
webrtc/base/network.h:60: virtual bool GetDefaultPrivateAddress(int family,
On 2015/10/28 23:57:40, juberti wrote:
> The address may not be private. Perhaps GetDefaultLocalAddress, to match
> AsyncSocket::GetLocalAddress?
> 
> Some commentary on what "default" means in this context would also be helpful.

Done.

[guoweis_webrtc, 2015-10-29 03:41:15]

Patchset #12 (id:200001) has been deleted

[guoweis_webrtc, 2015-10-30 16:38:13]

On 2015/10/29 03:41:00, guoweis wrote:
> PTAL.
> 
> https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/ipaddress.h
> File webrtc/base/ipaddress.h (right):
> 
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/ipaddress.h#...
> webrtc/base/ipaddress.h:183: IPAddress GetWildcardIP(int family);
> On 2015/10/28 23:57:40, juberti wrote:
> > probably should call this GetAnyIP, to match the IsAny API above
> 
> Done.
> 
> https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc
> File webrtc/base/network.cc (right):
> 
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
> webrtc/base/network.cc:154: char kPublicIPv4Host[] = "8.8.8.8";
> On 2015/10/28 23:57:40, juberti wrote:
> > const char.
> > 
> > This also needs explanatory comments.
> 
> Done.
> 
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
> webrtc/base/network.cc:754: IPAddress
> BasicNetworkManager::UpdateDefaultPrivateAddress(int family) {
> On 2015/10/28 23:57:40, juberti wrote:
> > This isn't really an Update, since it doesn't change any state.
> 
> renamed as QueryDefaultAddress
> 
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
> webrtc/base/network.cc:763: family == AF_INET ? kPublicIPv4Host :
> kPublicIPv6Host, 3478)) == 0) {
> On 2015/10/28 23:57:40, juberti wrote:
> > 3478 should be a constant as well. I would pick the DNS port, given the
choice
> > of addresses.
> 
> Done.
> 
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
> webrtc/base/network.cc:764: return socket->GetLocalAddress().ipaddr();
> On 2015/10/28 23:57:40, juberti wrote:
> > Not sure this will work in Chromium, where GetLocalAddress isn't filled in
> until
> > the OnAddressReady callback. This may need to be async.
> 
> Not an issue since IpcNetworkManager will be used instead.
> 
> https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.h
> File webrtc/base/network.h (right):
> 
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.h#ne...
> webrtc/base/network.h:31: extern char kPublicIPv4Host[];
> On 2015/10/28 23:57:40, juberti wrote:
> > Do these need to be exported? They seem like implementation details.
> 
> I made them exported so chrome could use the same IP here.
> 
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.h#ne...
> webrtc/base/network.h:60: virtual bool GetDefaultPrivateAddress(int family,
> On 2015/10/28 23:57:40, juberti wrote:
> > The address may not be private. Perhaps GetDefaultLocalAddress, to match
> > AsyncSocket::GetLocalAddress?
> > 
> > Some commentary on what "default" means in this context would also be
helpful.
> 
> Done.

Ping.

[guoweis_webrtc, 2015-10-30 16:56:18]

On 2015/10/30 16:38:13, guoweis wrote:
> On 2015/10/29 03:41:00, guoweis wrote:
> > PTAL.
> > 
> > https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/ipaddress.h
> > File webrtc/base/ipaddress.h (right):
> > 
> >
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/ipaddress.h#...
> > webrtc/base/ipaddress.h:183: IPAddress GetWildcardIP(int family);
> > On 2015/10/28 23:57:40, juberti wrote:
> > > probably should call this GetAnyIP, to match the IsAny API above
> > 
> > Done.
> > 
> > https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc
> > File webrtc/base/network.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
> > webrtc/base/network.cc:154: char kPublicIPv4Host[] = "8.8.8.8";
> > On 2015/10/28 23:57:40, juberti wrote:
> > > const char.
> > > 
> > > This also needs explanatory comments.
> > 
> > Done.
> > 
> >
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
> > webrtc/base/network.cc:754: IPAddress
> > BasicNetworkManager::UpdateDefaultPrivateAddress(int family) {
> > On 2015/10/28 23:57:40, juberti wrote:
> > > This isn't really an Update, since it doesn't change any state.
> > 
> > renamed as QueryDefaultAddress
> > 
> >
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
> > webrtc/base/network.cc:763: family == AF_INET ? kPublicIPv4Host :
> > kPublicIPv6Host, 3478)) == 0) {
> > On 2015/10/28 23:57:40, juberti wrote:
> > > 3478 should be a constant as well. I would pick the DNS port, given the
> choice
> > > of addresses.
> > 
> > Done.
> > 
> >
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
> > webrtc/base/network.cc:764: return socket->GetLocalAddress().ipaddr();
> > On 2015/10/28 23:57:40, juberti wrote:
> > > Not sure this will work in Chromium, where GetLocalAddress isn't filled in
> > until
> > > the OnAddressReady callback. This may need to be async.
> > 
> > Not an issue since IpcNetworkManager will be used instead.
> > 
> > https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.h
> > File webrtc/base/network.h (right):
> > 
> >
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.h#ne...
> > webrtc/base/network.h:31: extern char kPublicIPv4Host[];
> > On 2015/10/28 23:57:40, juberti wrote:
> > > Do these need to be exported? They seem like implementation details.
> > 
> > I made them exported so chrome could use the same IP here.
> > 
> >
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.h#ne...
> > webrtc/base/network.h:60: virtual bool GetDefaultPrivateAddress(int family,
> > On 2015/10/28 23:57:40, juberti wrote:
> > > The address may not be private. Perhaps GetDefaultLocalAddress, to match
> > > AsyncSocket::GetLocalAddress?
> > > 
> > > Some commentary on what "default" means in this context would also be
> helpful.
> > 
> > Done.
> 
> Ping.

Btw, Peter raised this concern that whether it's possible that the default
address change is not associated with network changes? In the current design,
they are coupled. In theory, someone in admin mode could change the routing
table and cause the default address change but I feel in reality, this is very
unlikely.

The alternative design would be always to go down to the socket layer (which is
IPC for chromium) and retrieve it at the time when needed. However, that's a
more involved code change and I'd like to only do that if necessary, especially,
in general, default private address is a property of the whole network, not a
property of a socket.

[guoweis_webrtc, 2015-11-10 17:31:49]

On 2015/10/30 16:56:18, guoweis wrote:
> On 2015/10/30 16:38:13, guoweis wrote:
> > On 2015/10/29 03:41:00, guoweis wrote:
> > > PTAL.
> > > 
> > >
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/ipaddress.h
> > > File webrtc/base/ipaddress.h (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/ipaddress.h#...
> > > webrtc/base/ipaddress.h:183: IPAddress GetWildcardIP(int family);
> > > On 2015/10/28 23:57:40, juberti wrote:
> > > > probably should call this GetAnyIP, to match the IsAny API above
> > > 
> > > Done.
> > > 
> > >
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc
> > > File webrtc/base/network.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
> > > webrtc/base/network.cc:154: char kPublicIPv4Host[] = "8.8.8.8";
> > > On 2015/10/28 23:57:40, juberti wrote:
> > > > const char.
> > > > 
> > > > This also needs explanatory comments.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
> > > webrtc/base/network.cc:754: IPAddress
> > > BasicNetworkManager::UpdateDefaultPrivateAddress(int family) {
> > > On 2015/10/28 23:57:40, juberti wrote:
> > > > This isn't really an Update, since it doesn't change any state.
> > > 
> > > renamed as QueryDefaultAddress
> > > 
> > >
> >
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
> > > webrtc/base/network.cc:763: family == AF_INET ? kPublicIPv4Host :
> > > kPublicIPv6Host, 3478)) == 0) {
> > > On 2015/10/28 23:57:40, juberti wrote:
> > > > 3478 should be a constant as well. I would pick the DNS port, given the
> > choice
> > > > of addresses.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.cc#n...
> > > webrtc/base/network.cc:764: return socket->GetLocalAddress().ipaddr();
> > > On 2015/10/28 23:57:40, juberti wrote:
> > > > Not sure this will work in Chromium, where GetLocalAddress isn't filled
in
> > > until
> > > > the OnAddressReady callback. This may need to be async.
> > > 
> > > Not an issue since IpcNetworkManager will be used instead.
> > > 
> > > https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.h
> > > File webrtc/base/network.h (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.h#ne...
> > > webrtc/base/network.h:31: extern char kPublicIPv4Host[];
> > > On 2015/10/28 23:57:40, juberti wrote:
> > > > Do these need to be exported? They seem like implementation details.
> > > 
> > > I made them exported so chrome could use the same IP here.
> > > 
> > >
> >
>
https://codereview.webrtc.org/1411253008/diff/180001/webrtc/base/network.h#ne...
> > > webrtc/base/network.h:60: virtual bool GetDefaultPrivateAddress(int
family,
> > > On 2015/10/28 23:57:40, juberti wrote:
> > > > The address may not be private. Perhaps GetDefaultLocalAddress, to match
> > > > AsyncSocket::GetLocalAddress?
> > > > 
> > > > Some commentary on what "default" means in this context would also be
> > helpful.
> > > 
> > > Done.
> > 
> > Ping.
> 
> Btw, Peter raised this concern that whether it's possible that the default
> address change is not associated with network changes? In the current design,
> they are coupled. In theory, someone in admin mode could change the routing
> table and cause the default address change but I feel in reality, this is very
> unlikely.
> 
> The alternative design would be always to go down to the socket layer (which
is
> IPC for chromium) and retrieve it at the time when needed. However, that's a
> more involved code change and I'd like to only do that if necessary,
especially,
> in general, default private address is a property of the whole network, not a
> property of a socket.

Ping.

[pthatcher1, 2015-11-10 18:52:35]

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.cc
File webrtc/base/network.cc (right):

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.cc#n...
webrtc/base/network.cc:204:
ipv4_any_address_network_->set_default_address_provider(this);
Why not just set the provider at the very beginning when the classes are
constructed?

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.cc#n...
webrtc/base/network.cc:768: }
It might be more clear as:

if (!socket) {
  return IPAddress();
}

auto remote_address = family == AF_INET ? kPublicIPv4Host : kPublicIPv6Host;
if (!socket->Connect(remote_address, kPublicPort)) {
  return IPAddress();
}

return socket->GetLocalAddress().ipaddr();

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.cc#n...
webrtc/base/network.cc:787:
set_default_local_address(QueryDefaultAddress(AF_INET6));
Why bother checking the stats?  Why not just query both all the time?

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
webrtc/base/network.h:57: class DefaultAddressProvider {
The DefaultAddressProvider has a GetDefaultLocalAddress.  Does that mean the
class name should be DefaultLocalAddressProviders, or that mean the method name
should be GetDefaultAddress, or does that mean we will have more methods in the
future to get other default addresses other than than the local one?

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
webrtc/base/network.h:61: // a multi-homed endpoint.
This could be more clear.  Perhaps "The default local address is the local
address used in multi-homed endpoint when the any address (0.0.0.0 or ::) is
used as the local address".

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
webrtc/base/network.h:62: virtual bool GetDefaultLocalAddress(int family,
IPAddress* ipaddr) const = 0;
Would this be more simple as GetDefaultLocalAddresses(IPAddress* ipv4,
IPAddress* ipv6) ?

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
webrtc/base/network.h:164: void set_default_local_address(const IPAddress& ip);
This is kind of awkward that we have a "set_foo" which actually sets two
different "foo"s.  Should this be set_default_local_addresses(const IPAddress&
ipv4, const IPAddress& ipv6)?

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
webrtc/base/network.h:181: IPAddress default_ipv6_address_;
To be consistent, this should be default_local_ipX_address_.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
webrtc/base/network.h:243: IPAddress QueryDefaultAddress(int family) const;
This could use a comment to make clear the difference between "Get" and "Query".
 And it should probably be "QueryDefaultLocalAddress".

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
webrtc/base/network.h:284: const DefaultAddressProvider*
default_address_provider() { return provider_; }
Between Network and DefaultAddressProvider, you embedded the provider in the
network.  But between NetworkManager and DefaultAddressProvider, you had manager
subclass the provider.  Why did you do it one way one place and another way the
other place?  Why not just embed in both places?

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
webrtc/base/network.h:358: const DefaultAddressProvider* provider_ = nullptr;
To be consistent, this should be default_address_provider_.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/p2p/base/portallo...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/p2p/base/portallo...
webrtc/p2p/base/portallocator.h:49: // When specified, we'll only generate the
default public candidate as used by
This might be more clear as "Only allocate STUN and HOST candidates for the
default local address of each IP family."

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/p2p/base/portallo...
webrtc/p2p/base/portallocator.h:52: // When specified, the default private
candidate will not be generated when
This might be more clear as "Only allocate STUN candidates for local addresses,
not HOST candidates."

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/p2p/base/stunport.cc
File webrtc/p2p/base/stunport.cc (right):

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/p2p/base/stunport...
webrtc/p2p/base/stunport.cc:307: Network()->default_address_provider()) {
Actually, not it seems like it would make sense to subclass in both cases so you
can just call Network()->GetLocalAddress(...) here.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/p2p/base/stunport...
webrtc/p2p/base/stunport.cc:435: related_address.SetIP(default_address);
Seems like we have duplicate logic here.  Can we just have a
MaybeSetDefaultLocalAddress(&related_address)?

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/p2p/client/basicp...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/p2p/client/basicp...
webrtc/p2p/client/basicportallocator.cc:440: (flags() &
PORTALLOCATOR_DISABLE_LOCAL_CANDIDATE)) {
What if  PORTALLOCATOR_DISABLE_LOCAL_CANDIDATE is disabled but not 
PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION?  Shouldn't se still filter out
CF_HOST?

[guoweis_webrtc, 2015-11-10 20:37:19]

Patchset #15 (id:280001) has been deleted

[guoweis_webrtc, 2015-11-10 20:37:30]

PTAL.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.cc
File webrtc/base/network.cc (right):

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.cc#n...
webrtc/base/network.cc:204:
ipv4_any_address_network_->set_default_address_provider(this);
On 2015/11/10 18:52:34, pthatcher1 wrote:
> Why not just set the provider at the very beginning when the classes are
> constructed?

Because chromium code calls the constructor as well and I can't modify them in
one batch.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.cc#n...
webrtc/base/network.cc:768: }
On 2015/11/10 18:52:34, pthatcher1 wrote:
> It might be more clear as:
> 
> if (!socket) {
>   return IPAddress();
> }
> 
> auto remote_address = family == AF_INET ? kPublicIPv4Host : kPublicIPv6Host;
> if (!socket->Connect(remote_address, kPublicPort)) {
>   return IPAddress();
> }
> 
> return socket->GetLocalAddress().ipaddr();

Done.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.cc#n...
webrtc/base/network.cc:787:
set_default_local_address(QueryDefaultAddress(AF_INET6));
On 2015/11/10 18:52:34, pthatcher1 wrote:
> Why bother checking the stats?  Why not just query both all the time?

If there is no ipv6 address at all, there is no point to query interface here
for that address family.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
webrtc/base/network.h:57: class DefaultAddressProvider {
On 2015/11/10 18:52:34, pthatcher1 wrote:
> The DefaultAddressProvider has a GetDefaultLocalAddress.  Does that mean the
> class name should be DefaultLocalAddressProviders, or that mean the method
name
> should be GetDefaultAddress, or does that mean we will have more methods in
the
> future to get other default addresses other than than the local one?

Done.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
webrtc/base/network.h:61: // a multi-homed endpoint.
On 2015/11/10 18:52:34, pthatcher1 wrote:
> This could be more clear.  Perhaps "The default local address is the local
> address used in multi-homed endpoint when the any address (0.0.0.0 or ::) is
> used as the local address".

Done.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
webrtc/base/network.h:62: virtual bool GetDefaultLocalAddress(int family,
IPAddress* ipaddr) const = 0;
On 2015/11/10 18:52:34, pthatcher1 wrote:
> Would this be more simple as GetDefaultLocalAddresses(IPAddress* ipv4,
> IPAddress* ipv6) ?

I like returning a bool which makes the caller to check the result more
explicit.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
webrtc/base/network.h:164: void set_default_local_address(const IPAddress& ip);
On 2015/11/10 18:52:34, pthatcher1 wrote:
> This is kind of awkward that we have a "set_foo" which actually sets two
> different "foo"s.  Should this be set_default_local_addresses(const IPAddress&
> ipv4, const IPAddress& ipv6)?

Done.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
webrtc/base/network.h:181: IPAddress default_ipv6_address_;
On 2015/11/10 18:52:34, pthatcher1 wrote:
> To be consistent, this should be default_local_ipX_address_.

Done.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
webrtc/base/network.h:243: IPAddress QueryDefaultAddress(int family) const;
On 2015/11/10 18:52:34, pthatcher1 wrote:
> This could use a comment to make clear the difference between "Get" and
"Query".
>  And it should probably be "QueryDefaultLocalAddress". 

Done.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
webrtc/base/network.h:284: const DefaultAddressProvider*
default_address_provider() { return provider_; }
On 2015/11/10 18:52:34, pthatcher1 wrote:
> Between Network and DefaultAddressProvider, you embedded the provider in the
> network.  But between NetworkManager and DefaultAddressProvider, you had
manager
> subclass the provider.  Why did you do it one way one place and another way
the
> other place?  Why not just embed in both places?

This is to reflect the fact that a single network shouldn't know the global
property like default local address.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
webrtc/base/network.h:358: const DefaultAddressProvider* provider_ = nullptr;
On 2015/11/10 18:52:34, pthatcher1 wrote:
> To be consistent, this should be default_address_provider_.

Done.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/p2p/base/portallo...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/p2p/base/portallo...
webrtc/p2p/base/portallocator.h:49: // When specified, we'll only generate the
default public candidate as used by
On 2015/11/10 18:52:34, pthatcher1 wrote:
> This might be more clear as "Only allocate STUN and HOST candidates for the
> default local address of each IP family."
> 

Done.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/p2p/base/portallo...
webrtc/p2p/base/portallocator.h:52: // When specified, the default private
candidate will not be generated when
On 2015/11/10 18:52:34, pthatcher1 wrote:
> This might be more clear as "Only allocate STUN candidates for local
addresses,
> not HOST candidates."

Done.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/p2p/base/stunport.cc
File webrtc/p2p/base/stunport.cc (right):

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/p2p/base/stunport...
webrtc/p2p/base/stunport.cc:307: Network()->default_address_provider()) {
On 2015/11/10 18:52:34, pthatcher1 wrote:
> Actually, not it seems like it would make sense to subclass in both cases so
you
> can just call Network()->GetLocalAddress(...) here.

should not be a network property.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/p2p/base/stunport...
webrtc/p2p/base/stunport.cc:435: related_address.SetIP(default_address);
On 2015/11/10 18:52:34, pthatcher1 wrote:
> Seems like we have duplicate logic here.  Can we just have a
> MaybeSetDefaultLocalAddress(&related_address)?

Done.

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/p2p/client/basicp...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/p2p/client/basicp...
webrtc/p2p/client/basicportallocator.cc:440: (flags() &
PORTALLOCATOR_DISABLE_LOCAL_CANDIDATE)) {
On 2015/11/10 18:52:34, pthatcher1 wrote:
> What if  PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION is disabled but not 
> PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION?  Shouldn't se still filter out
> CF_HOST?

Rename it to PORTALLOCATOR_DISABLE_DEFAULT_LOCAL_CANDIDATE

[pthatcher1, 2015-11-10 21:29:01]

lgtm, with comment nits

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
webrtc/base/network.h:61: // a multi-homed endpoint.
On 2015/11/10 20:37:29, guoweis wrote:
> On 2015/11/10 18:52:34, pthatcher1 wrote:
> > This could be more clear.  Perhaps "The default local address is the local
> > address used in multi-homed endpoint when the any address (0.0.0.0 or ::) is
> > used as the local address".
> 
> Done.

It doesn't appear changed.

https://codereview.webrtc.org/1411253008/diff/300001/webrtc/p2p/base/stunport.h
File webrtc/p2p/base/stunport.h (right):

https://codereview.webrtc.org/1411253008/diff/300001/webrtc/p2p/base/stunport...
webrtc/p2p/base/stunport.h:152: // |emit_local_for_anyaddress_| is true.
and the |addr| is currently the any address, right?

https://codereview.webrtc.org/1411253008/diff/300001/webrtc/p2p/client/portal...
File webrtc/p2p/client/portallocator_unittest.cc (right):

https://codereview.webrtc.org/1411253008/diff/300001/webrtc/p2p/client/portal...
webrtc/p2p/client/portallocator_unittest.cc:117: // Default route is the public
address that STUN server will observe when the
Default route => The default route

[guoweis_webrtc, 2015-11-10 21:33:35]

On 2015/11/10 21:29:01, pthatcher1 wrote:
> lgtm, with comment nits
> 
> https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h
> File webrtc/base/network.h (right):
> 
>
https://codereview.webrtc.org/1411253008/diff/260001/webrtc/base/network.h#ne...
> webrtc/base/network.h:61: // a multi-homed endpoint.
> On 2015/11/10 20:37:29, guoweis wrote:
> > On 2015/11/10 18:52:34, pthatcher1 wrote:
> > > This could be more clear.  Perhaps "The default local address is the local
> > > address used in multi-homed endpoint when the any address (0.0.0.0 or ::)
is
> > > used as the local address".
> > 
> > Done.
> 
> It doesn't appear changed.
> 
>
https://codereview.webrtc.org/1411253008/diff/300001/webrtc/p2p/base/stunport.h
> File webrtc/p2p/base/stunport.h (right):
> 
>
https://codereview.webrtc.org/1411253008/diff/300001/webrtc/p2p/base/stunport...
> webrtc/p2p/base/stunport.h:152: // |emit_local_for_anyaddress_| is true.
> and the |addr| is currently the any address, right?
> 
>
https://codereview.webrtc.org/1411253008/diff/300001/webrtc/p2p/client/portal...
> File webrtc/p2p/client/portallocator_unittest.cc (right):
> 
>
https://codereview.webrtc.org/1411253008/diff/300001/webrtc/p2p/client/portal...
> webrtc/p2p/client/portallocator_unittest.cc:117: // Default route is the
public
> address that STUN server will observe when the
> Default route => The default route

Really modified them this time. Landing now.

[guoweis_webrtc, 2015-11-10 21:33:38]

The CQ bit was checked by guoweis@webrtc.org

[guoweis_webrtc, 2015-11-10 21:33:39]

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/1411253008/#ps320001 (title:
"missing some changes.")

[commit-bot: I haz the power, 2015-11-10 21:33:43]

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

[guoweis_webrtc, 2015-11-10 22:48:08]

Committed patchset #17 (id:340001) manually as
9af97f89103d8f1f77b52a6ae77b8b7bcdc23f71 (presubmit successful).

[commit-bot: I haz the power, 2015-11-10 22:48:13]

Patchset 17 (id:??) landed as
https://crrev.com/9af97f89103d8f1f77b52a6ae77b8b7bcdc23f71
Cr-Commit-Position: refs/heads/master@{#10590}