Get the adapter type information from Android OS.

Get the adapter type information from Android OS.

BUG=

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

[honghaiz3, 2016-01-16 01:36:52]

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

[honghaiz3, 2016-01-16 01:36:52]

I will add a test to check that the adapter type can be passed from the network
monitor to the network manager. 
Meanwhile, you may want to take a look at the CL. 

Peter, this is a little different from what we have discussed. I feel that it is
OK to do a sync-call here because worker thread is not a UI thread and it should
only be called once or twice in a call, because the mapping from interface name
to type rarely changes ever.

[honghaiz3, 2016-01-16 01:36:58]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2016-01-16 01:38:55]

Patchset #1 (id:20001) has been deleted

[pthatcher1, 2016-01-19 02:13:07]

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

https://codereview.webrtc.org/1594673002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:75: switch (network_type)
{
I'd prefer AdapterTypeFromNetworkType

https://codereview.webrtc.org/1594673002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:90: return
rtc::ADAPTER_TYPE_VPN;
Would UNKNOWN be a better map?  What does bluetooth have to do with a VPN?

https://codereview.webrtc.org/1594673002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:335: return
adapter_types_[if_name];
You can avoid too lookups by storing the result of adapter_types_.find.

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

https://codereview.webrtc.org/1594673002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.h:94: std::map<std::string,
rtc::AdapterType> adapter_types_;
What is the key here?

https://codereview.webrtc.org/1594673002/diff/40001/webrtc/base/network.cc
File webrtc/base/network.cc (right):

https://codereview.webrtc.org/1594673002/diff/40001/webrtc/base/network.cc#ne...
webrtc/base/network.cc:445: adapter_type =
network_monitor_->GetAdapterType(cursor->ifa_name);
I'm still concerned about this being a blocking call.  If a new network coming
up causes a 20ms freeze in the network thread, that's 20ms of packets not
getting out.  I'd like to see if there's a way we can avoid this somehow.

[Taylor Brandstetter, 2016-01-20 01:51:57]

I don't think I completely understand how the caching of NetworkInformations
works (see my comments). I'm assuming the caching is done because enumerating
all of the networks is expensive, but if that's true, why is the enumeration
being done in "GetAdapterType"?

https://codereview.chromium.org/1594673002/diff/40001/talk/app/webrtc/java/jn...
File talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc (right):

https://codereview.chromium.org/1594673002/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:222:
OnNetworkAvailable_w(info);
This will add networks to the maps, but not remove them; is that intentional?

https://codereview.chromium.org/1594673002/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:327: }
It seems redundant to have two data structures that store the same information.
You could just store a vector of NetworkInformations, and have helper methods
like "GetNetworkInformationForAddress(...)",
"GetNetworkInformationForInterface(...)" as needed.

https://codereview.chromium.org/1594673002/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:333: UpdateNetworkInfos();
Why is UpdateNetworkInfos necessary here? Won't nativeNotifyOfNetworkConnect
ensure that the cached information is up-to-date?

https://codereview.chromium.org/1594673002/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:350: JOW(void,
NetworkMonitor_nativeNotifyOfNetworkConnect)(
Why is there not a corresponding "nativeNotifyOfNetworkLost"? If a network goes
away, shouldn't the map entry be removed?

https://codereview.chromium.org/1594673002/diff/40001/talk/app/webrtc/java/jn...
File talk/app/webrtc/java/jni/androidnetworkmonitor_jni.h (right):

https://codereview.chromium.org/1594673002/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.h:84: void
GetAllNetworkInfos(std::vector<NetworkInformation>* network_infos);
This method could simply return an std::vector, and rely on compiler
optimization to avoid extra copies being made.

[honghaiz3, 2016-01-21 21:25:50]

Patchset #2 (id:60001) has been deleted

[honghaiz3, 2016-01-21 21:38:36]

I made a few changes here:
1. Signals network information for all types of networks and signals network
disconnect. 
2. Does not call getAllNetwork, but instead push that information down from java
when NetworkMonitorAutoDetect is created. 
3. Do not do blocked call when getting adapter type. I will see how good it is
(how often we may still get unknown network type). In the worst case, we may get
unknown network type occasionally (at the beginning of the call). But that is
not worse than the existing code. We can decide what to do next in subsequent
CLs if this is not good enough. In my own measurement with apprtc, it has never
got unknown adapter type yet.

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

https://codereview.webrtc.org/1594673002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:75: switch (network_type)
{
On 2016/01/19 02:13:07, pthatcher1 wrote:
> I'd prefer AdapterTypeFromNetworkType

Done.

https://codereview.webrtc.org/1594673002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:90: return
rtc::ADAPTER_TYPE_VPN;
On 2016/01/19 02:13:07, pthatcher1 wrote:
> Would UNKNOWN be a better map?  What does bluetooth have to do with a VPN?
It has nothing to do with VPN, except perhaps, VPN and Bluetooth may have
similar priority ranks compared to other networks. The integer value of the enum
will be used for representing the network priority. 

I have never seen BLUETOOTH shown up as a network/connection type even if I
tried to set up a bluetooth connectivity. 

In the unlikely situation that Bluetooth network shows up, putting it as a VPN
type may give it similar rank of priority levels.

In fact, perhaps, we should just remove the whole AdapterType and use connection
type. That is what Chrome has done. WDYT?

https://codereview.webrtc.org/1594673002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:222:
OnNetworkAvailable_w(info);
On 2016/01/20 01:51:57, Taylor Brandstetter wrote:
> This will add networks to the maps, but not remove them; is that intentional?
Partially it was because we do not have the signaling of removing them yet. 
It was not critical because most likely it will be overwritten by a new network
info. 
And if it was not, it does not create issues except that the binding will fail.

Now I have changed to add the signaling and removing the network information.

https://codereview.webrtc.org/1594673002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:327: }
On 2016/01/20 01:51:57, Taylor Brandstetter wrote:
> It seems redundant to have two data structures that store the same
information.
> You could just store a vector of NetworkInformations, and have helper methods
> like "GetNetworkInformationForAddress(...)",
> "GetNetworkInformationForInterface(...)" as needed.
I used two structures so speed up the lookup. But perhaps you are right
considering that the size of the vector is typically small even if considering
that one network may have more than one IP addresses.
After second thought, I think we should still two maps. One for updating. Using
a map of network info keyed by handle makes it much easier to update the map. 
The other map adapter type by the name is kept because it can hold the
information longer. It is unlikely that the adapter type will change when the
network goes up and down.

https://codereview.webrtc.org/1594673002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:333: UpdateNetworkInfos();
On 2016/01/20 01:51:57, Taylor Brandstetter wrote:
> Why is UpdateNetworkInfos necessary here? Won't nativeNotifyOfNetworkConnect
> ensure that the cached information is up-to-date?

There might be some short-term inconsistency between Android Java and the
native. 
Occasionally, the native code may find the network but the adapter type is not
populated yet. 
But this is heavy weighted, sometimes when network switches, this can take
100ms. 
So I am removing this, but we do not delete the mapping from name to type once
it is found.

https://codereview.webrtc.org/1594673002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:335: return
adapter_types_[if_name];
On 2016/01/19 02:13:07, pthatcher1 wrote:
> You can avoid too lookups by storing the result of adapter_types_.find.
I am removing the update here. 
It was a little complicated here because UpdateNetowrkInfos may change the map
if it is called.

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

https://codereview.webrtc.org/1594673002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.h:84: void
GetAllNetworkInfos(std::vector<NetworkInformation>* network_infos);
On 2016/01/20 01:51:57, Taylor Brandstetter wrote:
> This method could simply return an std::vector, and rely on compiler
> optimization to avoid extra copies being made.

Thanks! Good to know that.

https://codereview.webrtc.org/1594673002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.h:94: std::map<std::string,
rtc::AdapterType> adapter_types_;
On 2016/01/19 02:13:07, pthatcher1 wrote:
> What is the key here?
Interface name. changed name.

https://codereview.webrtc.org/1594673002/diff/40001/webrtc/base/network.cc
File webrtc/base/network.cc (right):

https://codereview.webrtc.org/1594673002/diff/40001/webrtc/base/network.cc#ne...
webrtc/base/network.cc:445: adapter_type =
network_monitor_->GetAdapterType(cursor->ifa_name);
On 2016/01/19 02:13:07, pthatcher1 wrote:
> I'm still concerned about this being a blocking call.  If a new network coming
> up causes a 20ms freeze in the network thread, that's 20ms of packets not
> getting out.  I'd like to see if there's a way we can avoid this somehow.

Removed the blocking part except that when the NetworkMonitor is started, it
will collect the network information once.

[honghaiz3, 2016-01-21 21:40:50]

Thanks. PTAL.

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

https://codereview.webrtc.org/1594673002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:350: JOW(void,
NetworkMonitor_nativeNotifyOfNetworkConnect)(
On 2016/01/20 01:51:57, Taylor Brandstetter wrote:
> Why is there not a corresponding "nativeNotifyOfNetworkLost"? If a network
goes
> away, shouldn't the map entry be removed?
Added now (although BindNetworkToSocket will return -1 if the address is mapped
to an old handle).

[Taylor Brandstetter, 2016-01-26 02:35:09]

lgtm; thanks for addressing my comments.

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

https://codereview.webrtc.org/1594673002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:327: }
On 2016/01/21 21:38:36, honghaiz3 wrote:
> On 2016/01/20 01:51:57, Taylor Brandstetter wrote:
> > It seems redundant to have two data structures that store the same
> information.
> > You could just store a vector of NetworkInformations, and have helper
methods
> > like "GetNetworkInformationForAddress(...)",
> > "GetNetworkInformationForInterface(...)" as needed.
> I used two structures so speed up the lookup. But perhaps you are right
> considering that the size of the vector is typically small even if considering
> that one network may have more than one IP addresses.
> After second thought, I think we should still two maps. One for updating.
Using
> a map of network info keyed by handle makes it much easier to update the map. 
> The other map adapter type by the name is kept because it can hold the
> information longer. It is unlikely that the adapter type will change when the
> network goes up and down. 

Ah; so the second map exists so you can remember the adapter type even if the
network is currently down. Understood.

https://codereview.webrtc.org/1594673002/diff/80001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java
(right):

https://codereview.webrtc.org/1594673002/diff/80001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:476:
connectivityManagerDelegate.releaseCallback(mobileNetworkCallback);
nit: missing a space of indentation

[honghaiz3, 2016-01-26 23:31:54]

Thanks!

https://codereview.webrtc.org/1594673002/diff/80001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java
(right):

https://codereview.webrtc.org/1594673002/diff/80001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:476:
connectivityManagerDelegate.releaseCallback(mobileNetworkCallback);
On 2016/01/26 02:35:09, Taylor Brandstetter wrote:
> nit: missing a space of indentation

Done.

[pthatcher1, 2016-01-30 02:22:07]

Mostly nits, except that I'd like to see us use another map rather then doing a
reverse iteration of the first map.

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

https://codereview.webrtc.org/1594673002/diff/100001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:238: // Network handles
(IDs) are created by the Android OS in the increasing
in the increasing order => in increasing order

https://codereview.webrtc.org/1594673002/diff/100001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:245: [address](const
std::pair<NetworkHandle, NetworkInformation>& pos) {
"pos"?  Why not "it" or "kv"?

https://codereview.webrtc.org/1594673002/diff/100001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:249: });
Could we not have both a network_info_by_address_ and a network_info_by_handle_?
 It seems like it would be easy to update both when we add/remove, and it would
make this the address => info lookup a lot easier and faster.

https://codereview.webrtc.org/1594673002/diff/100001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:322:
network_info_by_handle_.erase(handle);
You'd just need to do:

iter = network_info_by_handle_.find(handle);
if (iter != newtork_info_by_handle_.end()) {
 for (rtc::IPAddress address : iter.second.ip_addresses) {
    network_info_by_address_.erase(address);
  }
}

https://codereview.webrtc.org/1594673002/diff/100001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:337: return
adapter_type_by_name_[if_name];
I think that's a little too subtle.  Doing something like:

iter = adapter_type_by_name_.find(if_name);
if (iter == .end()) {
  return ADAPTER_TYPE_UNKNOWN;
}
return iter.second;

[honghaiz3, 2016-02-02 04:07:42]

Thanks! PTAL.

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

https://codereview.webrtc.org/1594673002/diff/100001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:238: // Network handles
(IDs) are created by the Android OS in the increasing
On 2016/01/30 02:22:07, pthatcher1 wrote:
> in the increasing order => in increasing order

This is removed.

https://codereview.webrtc.org/1594673002/diff/100001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:245: [address](const
std::pair<NetworkHandle, NetworkInformation>& pos) {
On 2016/01/30 02:22:07, pthatcher1 wrote:
> "pos"?  Why not "it" or "kv"?

This part is removed.

https://codereview.webrtc.org/1594673002/diff/100001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:249: });
On 2016/01/30 02:22:07, pthatcher1 wrote:
> Could we not have both a network_info_by_address_ and a
network_info_by_handle_?
>  It seems like it would be easy to update both when we add/remove, and it
would
> make this the address => info lookup a lot easier and faster. 

Done.

https://codereview.webrtc.org/1594673002/diff/100001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:322:
network_info_by_handle_.erase(handle);
On 2016/01/30 02:22:07, pthatcher1 wrote:
> You'd just need to do:
> 
> iter = network_info_by_handle_.find(handle);
> if (iter != newtork_info_by_handle_.end()) {
>  for (rtc::IPAddress address : iter.second.ip_addresses) {
>     network_info_by_address_.erase(address);
>   }
> }
> 

Done.

https://codereview.webrtc.org/1594673002/diff/100001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:337: return
adapter_type_by_name_[if_name];
On 2016/01/30 02:22:07, pthatcher1 wrote:
> I think that's a little too subtle.  Doing something like:
> 
> iter = adapter_type_by_name_.find(if_name);
> if (iter == .end()) {
>   return ADAPTER_TYPE_UNKNOWN;
> }
> return iter.second;

Done.

[honghaiz3, 2016-02-02 04:09:14]

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

[honghaiz3, 2016-02-02 17:44:13]

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

https://codereview.webrtc.org/1594673002/diff/100001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidnetworkmonitor_jni.cc:249: });
On 2016/02/02 04:07:42, honghaiz3 wrote:
> On 2016/01/30 02:22:07, pthatcher1 wrote:
> > Could we not have both a network_info_by_address_ and a
> network_info_by_handle_?
> >  It seems like it would be easy to update both when we add/remove, and it
> would
> > make this the address => info lookup a lot easier and faster. 
> 
> Done.
Note, I used network_handle_by_address instead of the whole network information
by address.

[pthatcher1, 2016-02-02 18:51:16]

lgtm

[honghaiz3, 2016-02-02 20:42:57]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-02-02 20:42:57]

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/1594673002/#ps120001 (title:
"Addressed comments")

[commit-bot: I haz the power, 2016-02-02 20:43:22]

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

[commit-bot: I haz the power, 2016-02-02 20:54:23]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2016-02-02 20:54:34]

Description was changed from

==========
Get the adapter type information from Android OS.

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

to

==========
Get the adapter type information from Android OS.

BUG=

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

[commit-bot: I haz the power, 2016-02-02 20:54:36]

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