Bind a socket to a network if the network handle is set.

Add a new interface for creating a udp socket in which it binds the socket to a network if the network handle is set.
Plus, in stunport, turnport and allocation sequence, create a socket using the new interface.

BUG=

Committed: https://crrev.com/cec0a08275f767e665287dbf04fd9d04648a7d67
Cr-Commit-Position: refs/heads/master@{#11279}

[honghaiz3, 2015-12-30 23:33:00]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2015-12-30 23:33:08]

Patchset #1 (id:20001) has been deleted

[honghaiz3, 2016-01-05 23:57:31]

Patchset #2 (id:60001) has been deleted

[honghaiz3, 2016-01-06 00:00:01]

Patchset #1 (id:40001) has been deleted

[honghaiz3, 2016-01-06 00:03:50]

Description was changed from

==========
Bind a socket to a network if the network handle is set.

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

to

==========
Add a new interface for creating a udp socket in which it binds the socket to a
network if the network handle is set.
Plus, in stunport, turnport and sequence allocator, create a socket using the
new interface. 

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

[honghaiz3, 2016-01-06 00:31:54]

Patchset #1 (id:80001) has been deleted

[honghaiz3, 2016-01-06 00:45:38]

Patchset #1 (id:100001) has been deleted

[honghaiz3, 2016-01-06 00:52:59]

Description was changed from

==========
Add a new interface for creating a udp socket in which it binds the socket to a
network if the network handle is set.
Plus, in stunport, turnport and sequence allocator, create a socket using the
new interface. 

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

to

==========
Add a new interface for creating a udp socket in which it binds the socket to a
network if the network handle is set.
Plus, in stunport, turnport and allocation sequence, create a socket using the
new interface. 

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

[honghaiz3, 2016-01-06 01:05:09]

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

[honghaiz3, 2016-01-06 01:05:10]

This may need to be merged with the other CL for multi-networking on Android. If
it is landed on its own, it should not do anything extra.

[pthatcher1, 2016-01-06 21:58:47]

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/asyncsocket.h
File webrtc/base/asyncsocket.h (right):

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/asyncsocket....
webrtc/base/asyncsocket.h:30: virtual int BindToNetwork(NetworkHandle handle) =
0;
Can you put a comment explaining when this should be called and what effect it
will have (Android needs it to specify the network it will be bound to).

Actually, now I think about it, I think we could avoid needing this at all.  See
my later comment for how.

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/basictypes.h
File webrtc/base/basictypes.h (right):

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/basictypes.h...
webrtc/base/basictypes.h:57: 
Why is this in basictypes.h?   Shouldn't it be in something like network.h?

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/network.h#ne...
webrtc/base/network.h:359: bool IsHandleValid() const { return false; }
Can you leave a comment about what these are how they should be used?

Also, would it make sense to return an Optional<NetworkHandle>?  It seems weird
to have two methods for basically the same thing.


Actually, the more I think about it, the more I think we can avoid even having
this.  See more comments later.

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/physicalsock...
File webrtc/base/physicalsocketserver.cc (right):

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:105: static const uint32_t
INVALID_NETWORK_HANDLE = 0xFFFFFFFF;
Where do these values come from?

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:225: return ERR_FAILED;
For readability, can you move this into its own Android-specific function,
something like BindAndroidSocketToNetwork?

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/p2p/base/basicpac...
File webrtc/p2p/base/basicpacketsocketfactory.cc (right):

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/p2p/base/basicpac...
webrtc/p2p/base/basicpacketsocketfactory.cc:212: return ret;
So, I think we can do something here to avoid needing to have two bind methods
on a socket and a NetworkHandle on Network.  

Here's how:

1.  Define a SocketBinderInterface which has own method:  BindSocket(int fd,
const SocketAddress& local_address);

2.  Inside of PhysicalSocket::Bind, call socket_binder_->BindSocket(s_,
local_address).  If no socket_binder_ is set, it can call ::bind.

3.  Inject an AndroidSocketBinder into the PhysicalSocket as the socket_binder_,
and the AndroidSocketBinder will do all the Android-specific stuff.  The
AndroidSocketBinder can have access to an AndroidNetworkEnumerator or be an
AndroidNetworkEnumerator.

4.  Add SocketServer::SetSocketBinder(SocketBinderInterface* binder) and call it
with the AndroidNetworkEnumerator.  


5.  Maybe inside of PeerConnectionFactory::Initialize(), do something like this:

default_network_manager_.reset(new rtc::BasicNetworkManager());
default_socket_factory_.reset(new
rtc::BasicPacketSocketFactory(worker_thread_));
if (default_network_manager_.socket_binder()) {
 
default_socket_factory_.SetSocketBinder(default_network_manager_.socket_binder());
}

And then SocketPacketFactory::SetSocketBinder calls
SocketServer::SetSocketBinder.

Something like that :).

[honghaiz3, 2016-01-12 03:58:08]

Patchset #4 (id:180001) has been deleted

[honghaiz3, 2016-01-12 05:06:54]

Patchset #5 (id:220001) has been deleted

[honghaiz3, 2016-01-12 05:07:07]

Patchset #4 (id:200001) has been deleted

[honghaiz3, 2016-01-12 19:28:22]

Patchset #4 (id:240001) has been deleted

[honghaiz3, 2016-01-12 19:28:33]

Patchset #4 (id:200002) has been deleted

[honghaiz3, 2016-01-12 20:28:49]

Patchset #5 (id:290001) has been deleted

[honghaiz3, 2016-01-12 20:29:17]

Patchset #3 (id:160001) has been deleted

[honghaiz3, 2016-01-12 20:36:42]

I merged with the CL to pass down the NetworkInformation. 
Plus, I think at least in this CL, we do not need to Enumerate all networks, so
I keep the name. 
PTAL.

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/asyncsocket.h
File webrtc/base/asyncsocket.h (right):

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/asyncsocket....
webrtc/base/asyncsocket.h:30: virtual int BindToNetwork(NetworkHandle handle) =
0;
On 2016/01/06 21:58:46, pthatcher1 wrote:
> Can you put a comment explaining when this should be called and what effect it
> will have (Android needs it to specify the network it will be bound to).
> 
> Actually, now I think about it, I think we could avoid needing this at all. 
See
> my later comment for how.

Acknowledged.

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/basictypes.h
File webrtc/base/basictypes.h (right):

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/basictypes.h...
webrtc/base/basictypes.h:57: 
On 2016/01/06 21:58:46, pthatcher1 wrote:
> Why is this in basictypes.h?   Shouldn't it be in something like network.h?

Done.

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/network.h#ne...
webrtc/base/network.h:359: bool IsHandleValid() const { return false; }
On 2016/01/06 21:58:46, pthatcher1 wrote:
> Can you leave a comment about what these are how they should be used?
> 
> Also, would it make sense to return an Optional<NetworkHandle>?  It seems
weird
> to have two methods for basically the same thing.
> 
> 
> Actually, the more I think about it, the more I think we can avoid even having
> this.  See more comments later.

Acknowledged.

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/physicalsock...
File webrtc/base/physicalsocketserver.cc (right):

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:105: static const uint32_t
INVALID_NETWORK_HANDLE = 0xFFFFFFFF;
On 2016/01/06 21:58:46, pthatcher1 wrote:
> Where do these values come from?
From the Chromium code. I changed them to an enum now.

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:225: return ERR_FAILED;
On 2016/01/06 21:58:46, pthatcher1 wrote:
> For readability, can you move this into its own Android-specific function,
> something like BindAndroidSocketToNetwork?

Done.

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/p2p/base/basicpac...
File webrtc/p2p/base/basicpacketsocketfactory.cc (right):

https://codereview.webrtc.org/1556743002/diff/140001/webrtc/p2p/base/basicpac...
webrtc/p2p/base/basicpacketsocketfactory.cc:212: return ret;
On 2016/01/06 21:58:46, pthatcher1 wrote:
> So, I think we can do something here to avoid needing to have two bind methods
> on a socket and a NetworkHandle on Network.  
> 
> Here's how:
> 
> 1.  Define a SocketBinderInterface which has own method:  BindSocket(int fd,
> const SocketAddress& local_address);
> 
> 2.  Inside of PhysicalSocket::Bind, call socket_binder_->BindSocket(s_,
> local_address).  If no socket_binder_ is set, it can call ::bind.
> 
> 3.  Inject an AndroidSocketBinder into the PhysicalSocket as the
socket_binder_,
> and the AndroidSocketBinder will do all the Android-specific stuff.  The
> AndroidSocketBinder can have access to an AndroidNetworkEnumerator or be an
> AndroidNetworkEnumerator.
> 
> 4.  Add SocketServer::SetSocketBinder(SocketBinderInterface* binder) and call
it
> with the AndroidNetworkEnumerator.  
> 
> 
> 5.  Maybe inside of PeerConnectionFactory::Initialize(), do something like
this:
> 
> default_network_manager_.reset(new rtc::BasicNetworkManager());
> default_socket_factory_.reset(new
> rtc::BasicPacketSocketFactory(worker_thread_));
> if (default_network_manager_.socket_binder()) {
>  
>
default_socket_factory_.SetSocketBinder(default_network_manager_.socket_binder());
> }
> 
> And then SocketPacketFactory::SetSocketBinder calls
> SocketServer::SetSocketBinder.
> 
> Something like that :).
Step 2: I think ::bind should be called anyway, not?
Step 3: I implemented the NetworkBinderInterface in NetworkMonitor. 
I called it NetworkBinderInterface since it provides more information (binding
to network). 
Step 4,5, I just used the global access to the network monitor.

[honghaiz3, 2016-01-13 19:08:57]

Patchset #5 (id:330001) has been deleted

[honghaiz3, 2016-01-13 19:09:08]

Patchset #3 (id:270001) has been deleted

[honghaiz3, 2016-01-13 21:02:36]

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

[pthatcher1, 2016-01-14 20:07:24]

This looks *much* better.  The only big thing I'm not sure about is how we pass
the monitor as a binder into the socket server.  The way you have it adds quite
a bit of complexity and change.  I'm hoping we can reduce that.

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/and...
File talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java
(left):

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:229: ?
(WifiManager) context.getSystemService(Context.WIFI_SERVICE) : null;
Did this work before Lollipop?  Will Kitkat and previous be adversely affected
by removing this?

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/and...
File talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java
(right):

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:269: }
Is it necessary to override these last two?  Doesn't the default call the
superclass?

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc (right):

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:197: return
rtc::ERR_ADDRESS_NOT_FOUND;
If you store the result of the .find() you won't have to do two map look ups:

auto it = network_info_map_.find(address)
if (it == ...end()) {
 return not found;
}
NetworkInformation network = network.second;

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/androidnetworkmonitor_jni.h (right):

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.h:53: // c++ equivalent of
NetworkMonitorAutoDetect.NetworkInformation.
The comments are mutually recursive.  Could one of them add a little more
information about what this is and how it's used?

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.h:83:
std::map<rtc::IPAddress, NetworkInformation> network_info_map_;
A better name might be networy_info_by_address_.  "_map" is kind of redundant
with the type and "network_info_" only gives the value, not the key.

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/network.cc
File webrtc/base/network.cc (right):

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/network.cc#n...
webrtc/base/network.cc:734: network_monitor_ =
factory->GetOrCreateNetworkMonitor();
Why is it necessary for the factory to own the network monitor?

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/networkmonit...
File webrtc/base/networkmonitor.cc (right):

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.cc:32: void NetworkMonitorBase::Start() {
Should we check to make sure Start() isn't called more than once?

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/networkmonit...
File webrtc/base/networkmonitor.h (right):

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:21: typedef uint32_t NetworkHandle;
Is this ever used?

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:37: // packets on the socket |socket_fd| will be
sent via that network.
Can you explain why we need this (because some operating systems, like Android,
are silly, and require a second bind call).

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/physicalsock...
File webrtc/base/physicalsocketserver.cc (right):

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:1166: }
I'm not sure if this is the best way to get it connected.  It requires changing
the ownership of the monitor, and it leads to a funny method call.  Why not just
have SetNetworkBinder() and let the caller pass in a NetworkMonitor?

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/physicalsock...
File webrtc/base/physicalsocketserver.h (right):

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.h:78: void MaybeSetNetworkBinder() override;
This could use a comment, about what "maybe" means, and where the NetworkBinder
comes from, and who should call it and when.

[honghaiz3, 2016-01-15 01:00:38]

I now pass the NetworkMonitor as a networkbinder to the socket server in the
network manager. 
It removed all the hassles of changing the PhysicalSocketServer to set that.

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/and...
File talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java
(left):

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:229: ?
(WifiManager) context.getSystemService(Context.WIFI_SERVICE) : null;
On 2016/01/14 20:07:23, pthatcher1 wrote:
> Did this work before Lollipop?  Will Kitkat and previous be adversely affected
> by removing this?
Those are not really used before. There used to be some methods that need these
variables and objects but all those methods have been removed.

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/and...
File talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java
(right):

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:269: }
On 2016/01/14 20:07:24, pthatcher1 wrote:
> Is it necessary to override these last two?  Doesn't the default call the
> superclass?

Done.

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc (right):

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:197: return
rtc::ERR_ADDRESS_NOT_FOUND;
On 2016/01/14 20:07:24, pthatcher1 wrote:
> If you store the result of the .find() you won't have to do two map look ups:
> 
> auto it = network_info_map_.find(address)
> if (it == ...end()) {
>  return not found;
> }
> NetworkInformation network = network.second;

Done.

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/androidnetworkmonitor_jni.h (right):

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.h:53: // c++ equivalent of
NetworkMonitorAutoDetect.NetworkInformation.
On 2016/01/14 20:07:24, pthatcher1 wrote:
> The comments are mutually recursive.  Could one of them add a little more
> information about what this is and how it's used?

Done.

https://codereview.webrtc.org/1556743002/diff/370001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.h:83:
std::map<rtc::IPAddress, NetworkInformation> network_info_map_;
On 2016/01/14 20:07:24, pthatcher1 wrote:
> A better name might be networy_info_by_address_.  "_map" is kind of redundant
> with the type and "network_info_" only gives the value, not the key.

Done.

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/network.cc
File webrtc/base/network.cc (right):

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/network.cc#n...
webrtc/base/network.cc:734: network_monitor_ =
factory->GetOrCreateNetworkMonitor();
On 2016/01/14 20:07:24, pthatcher1 wrote:
> Why is it necessary for the factory to own the network monitor?

It is a good point. I revised it to own the monitor only at the network manager.

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/networkmonit...
File webrtc/base/networkmonitor.cc (right):

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.cc:32: void NetworkMonitorBase::Start() {
On 2016/01/14 20:07:24, pthatcher1 wrote:
> Should we check to make sure Start() isn't called more than once?
Removed this change.

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/networkmonit...
File webrtc/base/networkmonitor.h (right):

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:21: typedef uint32_t NetworkHandle;
On 2016/01/14 20:07:24, pthatcher1 wrote:
> Is this ever used?
Good catch. Move this androidnetworkmonitor_jni.h

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:37: // packets on the socket |socket_fd| will be
sent via that network.
On 2016/01/14 20:07:24, pthatcher1 wrote:
> Can you explain why we need this (because some operating systems, like
Android,
> are silly, and require a second bind call).

Done.

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/physicalsock...
File webrtc/base/physicalsocketserver.cc (right):

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:1166: }
On 2016/01/14 20:07:24, pthatcher1 wrote:
> I'm not sure if this is the best way to get it connected.  It requires
changing
> the ownership of the monitor, and it leads to a funny method call.  Why not
just
> have SetNetworkBinder() and let the caller pass in a NetworkMonitor?
Removed this. Set Networkbinder from the network manager.

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/physicalsock...
File webrtc/base/physicalsocketserver.h (right):

https://codereview.webrtc.org/1556743002/diff/370001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.h:78: void MaybeSetNetworkBinder() override;
On 2016/01/14 20:07:24, pthatcher1 wrote:
> This could use a comment, about what "maybe" means, and where the
NetworkBinder
> comes from, and who should call it and when.

Removed this.

[AlexG, 2016-01-15 18:32:37]

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
File talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java (right):

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java:160: public void
onNetworkConnect(NetworkInformation networkInfo) {
add override

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
File talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java
(right):

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:74: public
byte[] address;
final?

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:82: public
String name;
final and for all other fields?

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:258:
Log.i(TAG, "Network " + networkInformation.name + " is connected ");
Use Logging.i instead for all log messages in Java files

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:275: for
(int i = 0; i < linkAddresses.length; ++i) {
Why not just iterate through the list instead of converting list to array and
then iterating through array?

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:284:
connectivityManager.unregisterNetworkCallback(networkCallback);
Add check for Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP before using
API 21

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/jni_helpers.cc (right):

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/jni_helpers.cc:29: 
nit: extra white space?

[AlexG, 2016-01-15 18:34:12]

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
File talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java (right):

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java:35: import
android.util.Log;
use Logging for Android logs

[honghaiz3, 2016-01-15 18:53:13]

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

[honghaiz3, 2016-01-15 19:17:57]

Thanks! PTAL.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
File talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java (right):

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java:35: import
android.util.Log;
On 2016/01/15 18:34:12, AlexG wrote:
> use Logging for Android logs

Done.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java:160: public void
onNetworkConnect(NetworkInformation networkInfo) {
On 2016/01/15 18:32:37, AlexG wrote:
> add override

Done.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
File talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java
(right):

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:74: public
byte[] address;
On 2016/01/15 18:32:37, AlexG wrote:
> final?

Done.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:82: public
String name;
On 2016/01/15 18:32:37, AlexG wrote:
> final and for all other fields?

Done.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:258:
Log.i(TAG, "Network " + networkInformation.name + " is connected ");
On 2016/01/15 18:32:37, AlexG wrote:
> Use Logging.i instead for all log messages in Java files
I think Logging.d for all old Log.i.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:275: for
(int i = 0; i < linkAddresses.length; ++i) {
On 2016/01/15 18:32:37, AlexG wrote:
> Why not just iterate through the list instead of converting list to array and
> then iterating through array?

Done.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:284:
connectivityManager.unregisterNetworkCallback(networkCallback);
On 2016/01/15 18:32:37, AlexG wrote:
> Add check for Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP before
using
> API 21

Done.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/jni_helpers.cc (right):

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/jni_helpers.cc:29: 
On 2016/01/15 18:32:37, AlexG wrote:
> nit: extra white space?
Are you saying extra blank line?
We need a separate blank line to separate the header having the same file prefix
name from the rest of the headers.

[Taylor Brandstetter, 2016-01-15 19:36:13]

lgtm for C++ files, with minor nits

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc (right):

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:80: jbyte* addr_array =
jni->GetByteArrayElements(j_addresses, NULL);
nit: "nullptr" instead of "NULL", since that's what's used elsewhere in the
file.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:105: 
nit: Seems like extra whitespace here.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:108: CHECK_EXCEPTION(jni)
<< "Error during NetworkMonitor.GetIPAddressesFromJava";
Should this CHECK_EXCEPTION be right after GetArrayLength?

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:135: CHECK_EXCEPTION(jni)
<< "Error during NetworkMonitor.GetNetworkInfoFromJava";
I don't think you need this CHECK_EXCEPTION since GetIPAddressesFromjava already
calls it internally.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:144: ss <<
address.ToString() << " ";
nit: This will leave an extra space between the last address and the ']'

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:221: if
(setNetworkForSocket == nullptr) {
nit: "if (!setNetworkForSocket)", here and below.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:226: if (lib == nullptr) {
nit: "if (!lib)"

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/androidnetworkmonitor_jni.h (right):

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.h:34: 
nit: also include webrtc/base/basic_types.h, for uint32_t.

[pthatcher1, 2016-01-15 19:56:11]

https://codereview.webrtc.org/1556743002/diff/390001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc (right):

https://codereview.webrtc.org/1556743002/diff/390001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:247: return
rtc::ERR_FAILED;
This might be a little with the main exit being the good one:

if (rv == ENONET) {
  return rtc::ERR_NETWORK_CHANGED;
}
if (rv != 0) {
  return rtc::ERR_FAILED;
}
return rtc::OK;

https://codereview.webrtc.org/1556743002/diff/390001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/androidnetworkmonitor_jni.h (right):

https://codereview.webrtc.org/1556743002/diff/390001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.h:56: // the network type and
handle (Android network ID) for each interface.
is collected => collected
from Android OS => from Android

https://codereview.webrtc.org/1556743002/diff/390001/webrtc/base/network.cc
File webrtc/base/network.cc (right):

https://codereview.webrtc.org/1556743002/diff/390001/webrtc/base/network.cc#n...
webrtc/base/network.cc:744:
thread_->socketserver()->set_network_binder(network_monitor_.get());
While much less complex than before (which is good), this is still somewhat
magic behavior.  I'd like to think about this some more.

https://codereview.webrtc.org/1556743002/diff/390001/webrtc/examples/androida...
File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java
(right):

https://codereview.webrtc.org/1556743002/diff/390001/webrtc/examples/androida...
webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:460:
rtcConfig.continualGatheringPolicy =
PeerConnection.ContinualGatheringPolicy.GATHER_CONTINUALLY;
Is this an intentional change?  It doesn't seem related to this CL.

https://codereview.webrtc.org/1556743002/diff/430001/webrtc/base/network.cc
File webrtc/base/network.cc (right):

https://codereview.webrtc.org/1556743002/diff/430001/webrtc/base/network.cc#n...
webrtc/base/network.cc:742: ASSERT(network_monitor_);
Won't some factories return a null monitor?  Isn't that acceptable behavior?  It
was before.

https://codereview.webrtc.org/1556743002/diff/430001/webrtc/base/network_unit...
File webrtc/base/network_unittest.cc (right):

https://codereview.webrtc.org/1556743002/diff/430001/webrtc/base/network_unit...
webrtc/base/network_unittest.cc:942:
EXPECT_FALSE(GetNetworkMonitor(manager)->started_);
Please make a ->started() method.

https://codereview.webrtc.org/1556743002/diff/430001/webrtc/base/networkmonit...
File webrtc/base/networkmonitor.h (right):

https://codereview.webrtc.org/1556743002/diff/430001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:30: };
This should have more specific names, like:

NETWORK_BIND_SUCCESS
NETWORK_BIND_FAILURE
NETWORK_BIND_NOT_IMPLEMENTED
NETWORK_BIND_ADDRESS_NOT_FOUND
NETWORK_BIND_NETWORK_CHANGED

https://codereview.webrtc.org/1556743002/diff/430001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:60: class NetworkMonitorInterface : public
NetworkBinderInterface {
Is it necessary to mandate that *every* NetworkMonitor is also a NetworkBinder? 
I think it's the case that on Android it happens to be that way.  I was hoping
we would be able to so something like this:

AndroidNetworkMonitor::Start() {
  // Yes, this is kind of magic behavior, but ... (explain further...).
  worker_thread()->socket_server()->set_network_binder(this);
  // ...
}

And then this whole "NetworkMonitor is a NetworkBinder that has private info
that is used by SocketServer to do an extra bind call" is mostly contained to
Android, and the main NetworkMonitorInterface doesn't need to be changed.

[honghaiz3, 2016-01-15 21:37:36]

Thanks! Addressed all comments. PTAL.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc (right):

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:80: jbyte* addr_array =
jni->GetByteArrayElements(j_addresses, NULL);
On 2016/01/15 19:36:12, Taylor Brandstetter wrote:
> nit: "nullptr" instead of "NULL", since that's what's used elsewhere in the
> file.

Done.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:105: 
On 2016/01/15 19:36:12, Taylor Brandstetter wrote:
> nit: Seems like extra whitespace here.

Done.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:108: CHECK_EXCEPTION(jni)
<< "Error during NetworkMonitor.GetIPAddressesFromJava";
On 2016/01/15 19:36:12, Taylor Brandstetter wrote:
> Should this CHECK_EXCEPTION be right after GetArrayLength?

Done. Added another check after GetObjectArrayElement

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:135: CHECK_EXCEPTION(jni)
<< "Error during NetworkMonitor.GetNetworkInfoFromJava";
On 2016/01/15 19:36:12, Taylor Brandstetter wrote:
> I don't think you need this CHECK_EXCEPTION since GetIPAddressesFromjava
already
> calls it internally.

Done.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:144: ss <<
address.ToString() << " ";
On 2016/01/15 19:36:12, Taylor Brandstetter wrote:
> nit: This will leave an extra space between the last address and the ']'

Done. Thanks!

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:221: if
(setNetworkForSocket == nullptr) {
On 2016/01/15 19:36:12, Taylor Brandstetter wrote:
> nit: "if (!setNetworkForSocket)", here and below.
I feel using explicit checking is more readable and more error-prune.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:226: if (lib == nullptr) {
On 2016/01/15 19:36:12, Taylor Brandstetter wrote:
> nit: "if (!lib)"

same as above.

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/androidnetworkmonitor_jni.h (right):

https://codereview.webrtc.org/1556743002/diff/410001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.h:34: 
On 2016/01/15 19:36:13, Taylor Brandstetter wrote:
> nit: also include webrtc/base/basic_types.h, for uint32_t.

Done.

https://codereview.webrtc.org/1556743002/diff/430001/webrtc/base/network.cc
File webrtc/base/network.cc (right):

https://codereview.webrtc.org/1556743002/diff/430001/webrtc/base/network.cc#n...
webrtc/base/network.cc:742: ASSERT(network_monitor_);
On 2016/01/15 19:56:11, pthatcher1 wrote:
> Won't some factories return a null monitor?  Isn't that acceptable behavior? 
It
> was before.

Makes sense. Done.

https://codereview.webrtc.org/1556743002/diff/430001/webrtc/base/network_unit...
File webrtc/base/network_unittest.cc (right):

https://codereview.webrtc.org/1556743002/diff/430001/webrtc/base/network_unit...
webrtc/base/network_unittest.cc:942:
EXPECT_FALSE(GetNetworkMonitor(manager)->started_);
On 2016/01/15 19:56:11, pthatcher1 wrote:
> Please make a ->started() method.

Done.

https://codereview.webrtc.org/1556743002/diff/430001/webrtc/base/networkmonit...
File webrtc/base/networkmonitor.h (right):

https://codereview.webrtc.org/1556743002/diff/430001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:30: };
On 2016/01/15 19:56:11, pthatcher1 wrote:
> This should have more specific names, like:
> 
> NETWORK_BIND_SUCCESS
> NETWORK_BIND_FAILURE
> NETWORK_BIND_NOT_IMPLEMENTED
> NETWORK_BIND_ADDRESS_NOT_FOUND
> NETWORK_BIND_NETWORK_CHANGED

Done.

https://codereview.webrtc.org/1556743002/diff/430001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:60: class NetworkMonitorInterface : public
NetworkBinderInterface {
On 2016/01/15 19:56:11, pthatcher1 wrote:
> Is it necessary to mandate that *every* NetworkMonitor is also a
NetworkBinder? 
> I think it's the case that on Android it happens to be that way.  I was hoping
> we would be able to so something like this:
> 
> AndroidNetworkMonitor::Start() {
>   // Yes, this is kind of magic behavior, but ... (explain further...).
>   worker_thread()->socket_server()->set_network_binder(this);
>   // ...
> }
> 
> And then this whole "NetworkMonitor is a NetworkBinder that has private info
> that is used by SocketServer to do an extra bind call" is mostly contained to
> Android, and the main NetworkMonitorInterface doesn't need to be changed.
Good point. Thanks!

[pthatcher1, 2016-01-15 21:55:05]

lgtm

[honghaiz3, 2016-01-15 22:14:21]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-01-15 22:14:22]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1556743002/#ps450001 (title:
" ")

[commit-bot: I haz the power, 2016-01-15 22:14:32]

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

[AlexG, 2016-01-15 22:14:47]

lgtm

[commit-bot: I haz the power, 2016-01-15 22:49:12]

Description was changed from

==========
Add a new interface for creating a udp socket in which it binds the socket to a
network if the network handle is set.
Plus, in stunport, turnport and allocation sequence, create a socket using the
new interface. 

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

to

==========
Add a new interface for creating a udp socket in which it binds the socket to a
network if the network handle is set.
Plus, in stunport, turnport and allocation sequence, create a socket using the
new interface. 

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

[commit-bot: I haz the power, 2016-01-15 22:49:13]

Committed patchset #9 (id:450001)

[commit-bot: I haz the power, 2016-01-15 22:49:20]

Description was changed from

==========
Add a new interface for creating a udp socket in which it binds the socket to a
network if the network handle is set.
Plus, in stunport, turnport and allocation sequence, create a socket using the
new interface. 

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

to

==========
Add a new interface for creating a udp socket in which it binds the socket to a
network if the network handle is set.
Plus, in stunport, turnport and allocation sequence, create a socket using the
new interface.

BUG=

Committed: https://crrev.com/cec0a08275f767e665287dbf04fd9d04648a7d67
Cr-Commit-Position: refs/heads/master@{#11279}
==========

[commit-bot: I haz the power, 2016-01-15 22:49:21]

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