Add support for default local address in IpcNetworkManager

Add support for default local address in IpcNetworkManager

One of the modes of ip handling policy is to allow the associated default local address to be exposed along with the public interface. This requires Connect() to a public server (using the same ones from WebRTC) and GetLocalAddress() from a UDP socket. (Similar to https://code.google.com/p/chromium/codesearch#chromium/src/net/dns/host_resolver_impl.cc&rcl=1446089237&l=195)

This mode will also be in effect when permission check on mic/camera fails or not involved (like a data channel).

Default local address is really a global property of a network configuration, instead of individual network. They are most likely changed when interface changes. (Yes, people with admin right could change the routing table which changes the default local interface without network configuration change but I consider it is rare and not a supported scenario)

This depends on the ongoing CL: https://codereview.webrtc.org/1411253008

BUG=webrtc:5061
Committed: https://crrev.com/a8c3d89c8fbbb8910665f194efe33747cd5b6d48
Cr-Commit-Position: refs/heads/master@{#359360}

[guoweis_webrtc, 2015-11-10 19:00:47]

Description was changed from

==========
fix

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

to

==========
Add support for default local address in IpcNetworkManager

One of the modes of ip handling policy is to allow the associated default local
address to be exposed along with the public interface. This requires Connect()
to a public server (using the same ones from WebRTC) and GetLocalAddress() from
a UDP server.

This depends on the ongoing CL: https://codereview.webrtc.org/1411253008

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

[guoweis_webrtc, 2015-11-10 21:54:54]

Patchset #2 (id:20001) has been deleted

[guoweis_webrtc, 2015-11-10 21:55:31]

guoweis@webrtc.org changed reviewers:
+ guoweis@webrtc.org, pthatcher@chromium.org, sergeyu@chromium.org,
wfh@chromium.org

[guoweis_webrtc, 2015-11-10 21:55:32]

wfh@chromium.org: Please review changes in p2p_message.h

sergeyu@chromium.org: Please review changes in the rest.

[guoweis_webrtc, 2015-11-10 21:56:43]

Description was changed from

==========
Add support for default local address in IpcNetworkManager

One of the modes of ip handling policy is to allow the associated default local
address to be exposed along with the public interface. This requires Connect()
to a public server (using the same ones from WebRTC) and GetLocalAddress() from
a UDP server.

This depends on the ongoing CL: https://codereview.webrtc.org/1411253008

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

to

==========
Add support for default local address in IpcNetworkManager

One of the modes of ip handling policy is to allow the associated default local
address to be exposed along with the public interface. This requires Connect()
to a public server (using the same ones from WebRTC) and GetLocalAddress() from
a UDP socket.

Default local address is really a global property of a network configuration,
instead of individual network. They are most likely changed when interface
changes.

This depends on the ongoing CL: https://codereview.webrtc.org/1411253008

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

[guoweis_webrtc, 2015-11-10 21:57:31]

Description was changed from

==========
Add support for default local address in IpcNetworkManager

One of the modes of ip handling policy is to allow the associated default local
address to be exposed along with the public interface. This requires Connect()
to a public server (using the same ones from WebRTC) and GetLocalAddress() from
a UDP socket.

Default local address is really a global property of a network configuration,
instead of individual network. They are most likely changed when interface
changes.

This depends on the ongoing CL: https://codereview.webrtc.org/1411253008

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

to

==========
Add support for default local address in IpcNetworkManager

One of the modes of ip handling policy is to allow the associated default local
address to be exposed along with the public interface. This requires Connect()
to a public server (using the same ones from WebRTC) and GetLocalAddress() from
a UDP socket.

Default local address is really a global property of a network configuration,
instead of individual network. They are most likely changed when interface
changes. (Yes, people with admin right could change the routing table which
changes the default local interface without network configuration change but I
consider it is rare and not a supported scenario)

This depends on the ongoing CL: https://codereview.webrtc.org/1411253008

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

[guoweis_webrtc, 2015-11-10 22:01:23]

Description was changed from

==========
Add support for default local address in IpcNetworkManager

One of the modes of ip handling policy is to allow the associated default local
address to be exposed along with the public interface. This requires Connect()
to a public server (using the same ones from WebRTC) and GetLocalAddress() from
a UDP socket.

Default local address is really a global property of a network configuration,
instead of individual network. They are most likely changed when interface
changes. (Yes, people with admin right could change the routing table which
changes the default local interface without network configuration change but I
consider it is rare and not a supported scenario)

This depends on the ongoing CL: https://codereview.webrtc.org/1411253008

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

to

==========
Add support for default local address in IpcNetworkManager

One of the modes of ip handling policy is to allow the associated default local
address to be exposed along with the public interface. This requires Connect()
to a public server (using the same ones from WebRTC) and GetLocalAddress() from
a UDP socket. (Similar to
https://code.google.com/p/chromium/codesearch#chromium/src/net/dns/host_resol...)

This mode will also be in effect when permission check on mic/camera fails or
not involved (like a data channel).

Default local address is really a global property of a network configuration,
instead of individual network. They are most likely changed when interface
changes. (Yes, people with admin right could change the routing table which
changes the default local interface without network configuration change but I
consider it is rare and not a supported scenario)


This depends on the ongoing CL: https://codereview.webrtc.org/1411253008

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

[Sergey Ulanov, 2015-11-10 22:44:17]

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
File content/browser/renderer_host/p2p/socket_dispatcher_host.cc (right):

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:355: // Please see
crbug.com/455942. We want to make sure we're not on these 2
This bug reference is confusing here - the bug is about DNS resolution being
janky. Just say in the comments that creation and connection of UDP socket may
be janky. The bug you are referring to is just an example of such jank.

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:357:
DCHECK(!::content::BrowserThread::CurrentlyOn(BrowserThread::UI));
This code is always called on the FILE thread, so maybe just DCHECK it here?

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:360: net::IPEndPoint
local_address;
nit: Move this just before GetLocalAddress() call below.

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:363:
scoped_ptr<net::UDPClientSocket> socket(new net::UDPClientSocket(
Please use
net::ClientSocketFactory::GetDefaultFactory()->CreateDatagramClientSocket() to
create UDP sockets.

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:367: if
(!net::ParseURLHostnameToNumber(
net::IPAddressNumber is just an std::vector<uint8_t>. So instead of parsing the
strings you can store the target addresses as arrays of integers and initialize
the addresses from these arrays.

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:370: return
net::IPEndPoint();
this would need to be marked NOTREACHED(), but it doesn't matter if you take my
suggestion above.

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
File content/browser/renderer_host/p2p/socket_dispatcher_host.h (right):

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_dispatcher_host.h:117: net::IPEndPoint
default_ipv4_local_address_, default_ipv6_local_address_;
nit: Please separate these into two lines for readability:
   net::IPEndPoint default_ipv4_local_address_;
   net::IPEndPoint default_ipv6_local_address_;

https://codereview.chromium.org/1405963021/diff/60001/content/common/p2p_mess...
File content/common/p2p_messages.h (right):

https://codereview.chromium.org/1405963021/diff/60001/content/common/p2p_mess...
content/common/p2p_messages.h:63: net::IPEndPoint /* default_ipv4_local_address
*/,
Does this need to be IPEndPoint instead of IPAddressNumber? IPEndPoint is an IP
address with a port number and I don't think you need port number here.

https://codereview.chromium.org/1405963021/diff/60001/content/renderer/p2p/ne...
File content/renderer/p2p/network_list_observer.h (right):

https://codereview.chromium.org/1405963021/diff/60001/content/renderer/p2p/ne...
content/renderer/p2p/network_list_observer.h:10: #include
"net/base/ip_endpoint.h"
nit: forward declare IPEndPoint. No need to include this file here.

[guoweis_webrtc, 2015-11-11 00:11:51]

PTAL.

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
File content/browser/renderer_host/p2p/socket_dispatcher_host.cc (right):

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:355: // Please see
crbug.com/455942. We want to make sure we're not on these 2
On 2015/11/10 22:44:17, Sergey Ulanov wrote:
> This bug reference is confusing here - the bug is about DNS resolution being
> janky. Just say in the comments that creation and connection of UDP socket may
> be janky. The bug you are referring to is just an example of such jank.

Done.

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:357:
DCHECK(!::content::BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2015/11/10 22:44:16, Sergey Ulanov wrote:
> This code is always called on the FILE thread, so maybe just DCHECK it here?

Done.

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:360: net::IPEndPoint
local_address;
On 2015/11/10 22:44:16, Sergey Ulanov wrote:
> nit: Move this just before GetLocalAddress() call below.

Done.

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:363:
scoped_ptr<net::UDPClientSocket> socket(new net::UDPClientSocket(
On 2015/11/10 22:44:16, Sergey Ulanov wrote:
> Please use
> net::ClientSocketFactory::GetDefaultFactory()->CreateDatagramClientSocket() to
> create UDP sockets.

Done.

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:367: if
(!net::ParseURLHostnameToNumber(
On 2015/11/10 22:44:16, Sergey Ulanov wrote:
> net::IPAddressNumber is just an std::vector<uint8_t>. So instead of parsing
the
> strings you can store the target addresses as arrays of integers and
initialize
> the addresses from these arrays.

Done.

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:370: return
net::IPEndPoint();
On 2015/11/10 22:44:16, Sergey Ulanov wrote:
> this would need to be marked NOTREACHED(), but it doesn't matter if you take
my
> suggestion above.

Done.

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
File content/browser/renderer_host/p2p/socket_dispatcher_host.h (right):

https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_dispatcher_host.h:117: net::IPEndPoint
default_ipv4_local_address_, default_ipv6_local_address_;
On 2015/11/10 22:44:17, Sergey Ulanov wrote:
> nit: Please separate these into two lines for readability:
>    net::IPEndPoint default_ipv4_local_address_;
>    net::IPEndPoint default_ipv6_local_address_;

Done.

https://codereview.chromium.org/1405963021/diff/60001/content/common/p2p_mess...
File content/common/p2p_messages.h (right):

https://codereview.chromium.org/1405963021/diff/60001/content/common/p2p_mess...
content/common/p2p_messages.h:63: net::IPEndPoint /* default_ipv4_local_address
*/,
On 2015/11/10 22:44:17, Sergey Ulanov wrote:
> Does this need to be IPEndPoint instead of IPAddressNumber? IPEndPoint is an
IP
> address with a port number and I don't think you need port number here.

Done.

https://codereview.chromium.org/1405963021/diff/60001/content/renderer/p2p/ne...
File content/renderer/p2p/network_list_observer.h (right):

https://codereview.chromium.org/1405963021/diff/60001/content/renderer/p2p/ne...
content/renderer/p2p/network_list_observer.h:10: #include
"net/base/ip_endpoint.h"
On 2015/11/10 22:44:17, Sergey Ulanov wrote:
> nit: forward declare IPEndPoint. No need to include this file here.

Moved to IPAddressNumber and it's a typedef so keeping this around.

[guoweis_webrtc, 2015-11-11 18:13:45]

Patchset #4 (id:80001) has been deleted

[guoweis_webrtc, 2015-11-11 18:14:16]

On 2015/11/11 00:11:51, guoweis wrote:
> PTAL.
> 
>
https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
> File content/browser/renderer_host/p2p/socket_dispatcher_host.cc (right):
> 
>
https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
> content/browser/renderer_host/p2p/socket_dispatcher_host.cc:355: // Please see
> crbug.com/455942. We want to make sure we're not on these 2
> On 2015/11/10 22:44:17, Sergey Ulanov wrote:
> > This bug reference is confusing here - the bug is about DNS resolution being
> > janky. Just say in the comments that creation and connection of UDP socket
may
> > be janky. The bug you are referring to is just an example of such jank.
> 
> Done.
> 
>
https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
> content/browser/renderer_host/p2p/socket_dispatcher_host.cc:357:
> DCHECK(!::content::BrowserThread::CurrentlyOn(BrowserThread::UI));
> On 2015/11/10 22:44:16, Sergey Ulanov wrote:
> > This code is always called on the FILE thread, so maybe just DCHECK it here?
> 
> Done.
> 
>
https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
> content/browser/renderer_host/p2p/socket_dispatcher_host.cc:360:
net::IPEndPoint
> local_address;
> On 2015/11/10 22:44:16, Sergey Ulanov wrote:
> > nit: Move this just before GetLocalAddress() call below.
> 
> Done.
> 
>
https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
> content/browser/renderer_host/p2p/socket_dispatcher_host.cc:363:
> scoped_ptr<net::UDPClientSocket> socket(new net::UDPClientSocket(
> On 2015/11/10 22:44:16, Sergey Ulanov wrote:
> > Please use
> > net::ClientSocketFactory::GetDefaultFactory()->CreateDatagramClientSocket()
to
> > create UDP sockets.
> 
> Done.
> 
>
https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
> content/browser/renderer_host/p2p/socket_dispatcher_host.cc:367: if
> (!net::ParseURLHostnameToNumber(
> On 2015/11/10 22:44:16, Sergey Ulanov wrote:
> > net::IPAddressNumber is just an std::vector<uint8_t>. So instead of parsing
> the
> > strings you can store the target addresses as arrays of integers and
> initialize
> > the addresses from these arrays.
> 
> Done.
> 
>
https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
> content/browser/renderer_host/p2p/socket_dispatcher_host.cc:370: return
> net::IPEndPoint();
> On 2015/11/10 22:44:16, Sergey Ulanov wrote:
> > this would need to be marked NOTREACHED(), but it doesn't matter if you take
> my
> > suggestion above.
> 
> Done.
> 
>
https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
> File content/browser/renderer_host/p2p/socket_dispatcher_host.h (right):
> 
>
https://codereview.chromium.org/1405963021/diff/60001/content/browser/rendere...
> content/browser/renderer_host/p2p/socket_dispatcher_host.h:117:
net::IPEndPoint
> default_ipv4_local_address_, default_ipv6_local_address_;
> On 2015/11/10 22:44:17, Sergey Ulanov wrote:
> > nit: Please separate these into two lines for readability:
> >    net::IPEndPoint default_ipv4_local_address_;
> >    net::IPEndPoint default_ipv6_local_address_;
> 
> Done.
> 
>
https://codereview.chromium.org/1405963021/diff/60001/content/common/p2p_mess...
> File content/common/p2p_messages.h (right):
> 
>
https://codereview.chromium.org/1405963021/diff/60001/content/common/p2p_mess...
> content/common/p2p_messages.h:63: net::IPEndPoint /*
default_ipv4_local_address
> */,
> On 2015/11/10 22:44:17, Sergey Ulanov wrote:
> > Does this need to be IPEndPoint instead of IPAddressNumber? IPEndPoint is an
> IP
> > address with a port number and I don't think you need port number here.
> 
> Done.
> 
>
https://codereview.chromium.org/1405963021/diff/60001/content/renderer/p2p/ne...
> File content/renderer/p2p/network_list_observer.h (right):
> 
>
https://codereview.chromium.org/1405963021/diff/60001/content/renderer/p2p/ne...
> content/renderer/p2p/network_list_observer.h:10: #include
> "net/base/ip_endpoint.h"
> On 2015/11/10 22:44:17, Sergey Ulanov wrote:
> > nit: forward declare IPEndPoint. No need to include this file here.
> 
> Moved to IPAddressNumber and it's a typedef so keeping this around.

Found an issue with the code. Fixing it now.

[Sergey Ulanov, 2015-11-11 18:23:48]

please upload the latest patchset

[Sergey Ulanov, 2015-11-11 18:56:33]

https://codereview.chromium.org/1405963021/diff/100001/content/renderer/p2p/i...
File content/renderer/p2p/ipc_network_manager.cc (right):

https://codereview.chromium.org/1405963021/diff/100001/content/renderer/p2p/i...
content/renderer/p2p/ipc_network_manager.cc:85:
rtc::IPFromString(net::IPAddressToString(default_ipv4_local_address),
Using strings to convert address types may be fragile and doesn't look like the
right thing to do it. Also the code below already has to convert IPAddressNumber
to rtc::IPAddress. I suggest you add a helper in /jingle/glue/utils.h for this
operation and use it here.

[guoweis_webrtc, 2015-11-11 19:53:53]

Patchset #5 (id:120001) has been deleted

[Sergey Ulanov, 2015-11-11 21:10:03]

https://codereview.chromium.org/1405963021/diff/140001/content/renderer/p2p/i...
File content/renderer/p2p/ipc_network_manager.cc (right):

https://codereview.chromium.org/1405963021/diff/140001/content/renderer/p2p/i...
content/renderer/p2p/ipc_network_manager.cc:97: if
(!jingle_glue::IPAddressNumberToIPAddress(it->address, &ip_address))
This line is duplicated for ipv6 addresses below. Move this out of the if block.

https://codereview.chromium.org/1405963021/diff/140001/content/renderer/p2p/i...
content/renderer/p2p/ipc_network_manager.cc:118: rtc::IPAddress prefix =
rtc::TruncateIP(ip6_addr, it->prefix_length);
All this code (lines 118-124) is the same as for IPv4 addresses except that you
need rtc::InterfaceAddress needs to have address flags set. Move it out of the
if block. The only part here that is special for IPv6 addresses in
rtc::InterfaceAddress creation.

https://codereview.chromium.org/1405963021/diff/140001/jingle/glue/utils.cc
File jingle/glue/utils.cc (right):

https://codereview.chromium.org/1405963021/diff/140001/jingle/glue/utils.cc#n...
jingle/glue/utils.cc:37: bool IPAddressNumberToIPAddress(const
net::IPAddressNumber& ip_address_number,
Don't really need to return bool here. rtc::IPAddress has null/nil value (see
IsNil()). So you can return rtc::IPAddress value

https://codereview.chromium.org/1405963021/diff/140001/jingle/glue/utils.cc#n...
jingle/glue/utils.cc:40: uint32 address;
Please change this to uint32_t

[guoweis_webrtc, 2015-11-11 22:04:47]

PTAL.

[Will Harris, 2015-11-11 22:38:33]

content/common/p2p_messages.h security lgtm - message from browser to renderer
with fully validated fields.

[Sergey Ulanov, 2015-11-11 23:23:26]

https://codereview.chromium.org/1405963021/diff/160001/content/renderer/p2p/i...
File content/renderer/p2p/ipc_network_manager.cc (right):

https://codereview.chromium.org/1405963021/diff/160001/content/renderer/p2p/i...
content/renderer/p2p/ipc_network_manager.cc:84: rtc::IPAddress ipv4 =
nit: Don't think you need these as variables. Move IpAddressNumberToIPAddress()
call inside the set_default_local_addresses() argument list

https://codereview.chromium.org/1405963021/diff/160001/content/renderer/p2p/i...
content/renderer/p2p/ipc_network_manager.cc:97: if (ip_address.IsNil())
I think this can be DCHECK(ip_address.IsNil()). We never expect to get an
address that cannot be converted here.

https://codereview.chromium.org/1405963021/diff/160001/content/renderer/p2p/i...
content/renderer/p2p/ipc_network_manager.cc:120:
networks.push_back(network.release());
These two lines are still duplicated and that can be avoided, e.g.:

rtc::InterfaceAddress iface_addr;
if (addr is ipv4) {
 iface_addr = rtc::InterfaceAddress(addr);
} else if (addr is ipv6) {
 iface_addr = rtc::InterfaceAddress(addr, it->ip_address_attributes);
 if (addr should be scipped)
   continue;
}

network->AddIP(ip6_addr);
networks.push_back(network.release());

[guoweis_webrtc, 2015-11-11 23:42:22]

On 2015/11/11 23:23:26, Sergey Ulanov wrote:
>
https://codereview.chromium.org/1405963021/diff/160001/content/renderer/p2p/i...
> File content/renderer/p2p/ipc_network_manager.cc (right):
> 
>
https://codereview.chromium.org/1405963021/diff/160001/content/renderer/p2p/i...
> content/renderer/p2p/ipc_network_manager.cc:84: rtc::IPAddress ipv4 =
> nit: Don't think you need these as variables. Move
IpAddressNumberToIPAddress()
> call inside the set_default_local_addresses() argument list
> 
>
https://codereview.chromium.org/1405963021/diff/160001/content/renderer/p2p/i...
> content/renderer/p2p/ipc_network_manager.cc:97: if (ip_address.IsNil())
> I think this can be DCHECK(ip_address.IsNil()). We never expect to get an
> address that cannot be converted here.
> 
>
https://codereview.chromium.org/1405963021/diff/160001/content/renderer/p2p/i...
> content/renderer/p2p/ipc_network_manager.cc:120:
> networks.push_back(network.release());
> These two lines are still duplicated and that can be avoided, e.g.:
> 
> rtc::InterfaceAddress iface_addr;
> if (addr is ipv4) {
>  iface_addr = rtc::InterfaceAddress(addr);
> } else if (addr is ipv6) {
>  iface_addr = rtc::InterfaceAddress(addr, it->ip_address_attributes);
>  if (addr should be scipped)
>    continue;
> }
> 
> network->AddIP(ip6_addr);
> networks.push_back(network.release());

addressed.

[guoweis_webrtc, 2015-11-11 23:42:31]

https://codereview.chromium.org/1405963021/diff/160001/content/renderer/p2p/i...
File content/renderer/p2p/ipc_network_manager.cc (right):

https://codereview.chromium.org/1405963021/diff/160001/content/renderer/p2p/i...
content/renderer/p2p/ipc_network_manager.cc:84: rtc::IPAddress ipv4 =
On 2015/11/11 23:23:26, Sergey Ulanov wrote:
> nit: Don't think you need these as variables. Move
IpAddressNumberToIPAddress()
> call inside the set_default_local_addresses() argument list

Done.

https://codereview.chromium.org/1405963021/diff/160001/content/renderer/p2p/i...
content/renderer/p2p/ipc_network_manager.cc:97: if (ip_address.IsNil())
On 2015/11/11 23:23:26, Sergey Ulanov wrote:
> I think this can be DCHECK(ip_address.IsNil()). We never expect to get an
> address that cannot be converted here.

Done.

https://codereview.chromium.org/1405963021/diff/160001/content/renderer/p2p/i...
content/renderer/p2p/ipc_network_manager.cc:120:
networks.push_back(network.release());
On 2015/11/11 23:23:26, Sergey Ulanov wrote:
> These two lines are still duplicated and that can be avoided, e.g.:
> 
> rtc::InterfaceAddress iface_addr;
> if (addr is ipv4) {
>  iface_addr = rtc::InterfaceAddress(addr);
> } else if (addr is ipv6) {
>  iface_addr = rtc::InterfaceAddress(addr, it->ip_address_attributes);
>  if (addr should be scipped)
>    continue;
> }
> 
> network->AddIP(ip6_addr);
> networks.push_back(network.release());

Done.

[guoweis_webrtc, 2015-11-12 18:11:21]

Patchset #9 (id:220001) has been deleted

[Sergey Ulanov, 2015-11-12 18:24:43]

lgtm

[guoweis_webrtc, 2015-11-12 18:33:04]

The CQ bit was checked by guoweis@webrtc.org

[guoweis_webrtc, 2015-11-12 18:33:04]

The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1405963021/#ps200001
(title: "fix a sizeof")

[commit-bot: I haz the power, 2015-11-12 18:36:51]

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

[commit-bot: I haz the power, 2015-11-12 19:40:29]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2015-11-12 20:09:07]

Patchset 8 (id:??) landed as
https://crrev.com/a8c3d89c8fbbb8910665f194efe33747cd5b6d48
Cr-Commit-Position: refs/heads/master@{#359360}