Implement net::GetNetworkList() for Android.

Implement net::GetNetworkList() for Android.

* Implement net::GetNetworkList() by java.net.NetworkInterface through JNI.
* Remove NetUtilTest.GetNetworkList from net_unittests_apk disabled list.

BUG=
TEST=net_unittests_apk --gtest-filter=NetUtilTest.GetNetworkList


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156257

[Shouqun Liu, 2012-09-11 07:55:22]

hi, this patch implements net::GetNetworkList() for Android also remove
NetUtilTest.GetNetworkList from disabled cases list.

[bulach, 2012-09-11 10:16:36]

lgtm for android, you'll need owners approval for net.

just nits and small suggestions below, thanks for implementing this!

http://codereview.chromium.org/10905207/diff/1001/net/android/java/src/org/ch...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

http://codereview.chromium.org/10905207/diff/1001/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:92: * @return
the network interfaces list (if any) string. the items in
nit: s/. the/. The/

http://codereview.chromium.org/10905207/diff/1001/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:93: *        
the list string is delimited by a semicolon ";", each item
nit: s/string is/string are/

http://codereview.chromium.org/10905207/diff/1001/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:106: Log.w(TAG,
"counld not get network interfaces: " + e);
nit: s/counld not/Could not/ (or perhaps "Unable to")

http://codereview.chromium.org/10905207/diff/1001/net/android/network_library.h
File net/android/network_library.h (right):

http://codereview.chromium.org/10905207/diff/1001/net/android/network_library...
net/android/network_library.h:47: // Return a string contains network list
items, each item is a network name and
nit: Returns a string containing

http://codereview.chromium.org/10905207/diff/1001/net/android/network_library...
net/android/network_library.h:49: // string contains two items.
nit: containing

http://codereview.chromium.org/10905207/diff/1001/net/base/net_util_posix.cc
File net/base/net_util_posix.cc (right):

http://codereview.chromium.org/10905207/diff/1001/net/base/net_util_posix.cc#...
net/base/net_util_posix.cc:68: StringTokenizer t(network_list, ";");
nit: s/t/network_interfaces/

http://codereview.chromium.org/10905207/diff/1001/net/base/net_util_posix.cc#...
net/base/net_util_posix.cc:69: while(t.GetNext()) {
nit: space before first parens, "while (network_interfaces.GetNext()) {"

[bulach, 2012-09-11 10:16:36]

lgtm for android, you'll need owners approval for net.

just nits and small suggestions below, thanks for implementing this!

http://codereview.chromium.org/10905207/diff/1001/net/android/java/src/org/ch...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

http://codereview.chromium.org/10905207/diff/1001/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:92: * @return
the network interfaces list (if any) string. the items in
nit: s/. the/. The/

http://codereview.chromium.org/10905207/diff/1001/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:93: *        
the list string is delimited by a semicolon ";", each item
nit: s/string is/string are/

http://codereview.chromium.org/10905207/diff/1001/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:106: Log.w(TAG,
"counld not get network interfaces: " + e);
nit: s/counld not/Could not/ (or perhaps "Unable to")

http://codereview.chromium.org/10905207/diff/1001/net/android/network_library.h
File net/android/network_library.h (right):

http://codereview.chromium.org/10905207/diff/1001/net/android/network_library...
net/android/network_library.h:47: // Return a string contains network list
items, each item is a network name and
nit: Returns a string containing

http://codereview.chromium.org/10905207/diff/1001/net/android/network_library...
net/android/network_library.h:49: // string contains two items.
nit: containing

http://codereview.chromium.org/10905207/diff/1001/net/base/net_util_posix.cc
File net/base/net_util_posix.cc (right):

http://codereview.chromium.org/10905207/diff/1001/net/base/net_util_posix.cc#...
net/base/net_util_posix.cc:68: StringTokenizer t(network_list, ";");
nit: s/t/network_interfaces/

http://codereview.chromium.org/10905207/diff/1001/net/base/net_util_posix.cc#...
net/base/net_util_posix.cc:69: while(t.GetNext()) {
nit: space before first parens, "while (network_interfaces.GetNext()) {"

[Shouqun Liu, 2012-09-11 11:53:28]

Nits fixed and patch updated, thanks for carefully review:)

On 2012/09/11 10:16:36, bulach wrote:
> lgtm for android, you'll need owners approval for net.
> 
> just nits and small suggestions below, thanks for implementing this!
> 
>
http://codereview.chromium.org/10905207/diff/1001/net/android/java/src/org/ch...
> File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):
> 
>
http://codereview.chromium.org/10905207/diff/1001/net/android/java/src/org/ch...
> net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:92: * @return
> the network interfaces list (if any) string. the items in
> nit: s/. the/. The/
> 
>
http://codereview.chromium.org/10905207/diff/1001/net/android/java/src/org/ch...
> net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:93: *        
> the list string is delimited by a semicolon ";", each item
> nit: s/string is/string are/
> 
>
http://codereview.chromium.org/10905207/diff/1001/net/android/java/src/org/ch...
> net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:106:
Log.w(TAG,
> "counld not get network interfaces: " + e);
> nit: s/counld not/Could not/ (or perhaps "Unable to")
> 
>
http://codereview.chromium.org/10905207/diff/1001/net/android/network_library.h
> File net/android/network_library.h (right):
> 
>
http://codereview.chromium.org/10905207/diff/1001/net/android/network_library...
> net/android/network_library.h:47: // Return a string contains network list
> items, each item is a network name and
> nit: Returns a string containing
> 
>
http://codereview.chromium.org/10905207/diff/1001/net/android/network_library...
> net/android/network_library.h:49: // string contains two items.
> nit: containing
> 
> http://codereview.chromium.org/10905207/diff/1001/net/base/net_util_posix.cc
> File net/base/net_util_posix.cc (right):
> 
>
http://codereview.chromium.org/10905207/diff/1001/net/base/net_util_posix.cc#...
> net/base/net_util_posix.cc:68: StringTokenizer t(network_list, ";");
> nit: s/t/network_interfaces/
> 
>
http://codereview.chromium.org/10905207/diff/1001/net/base/net_util_posix.cc#...
> net/base/net_util_posix.cc:69: while(t.GetNext()) {
> nit: space before first parens, "while (network_interfaces.GetNext()) {"

[wtc, 2012-09-11 23:04:49]

Patch set 2 LGTM. I suggest some changes.

http://codereview.chromium.org/10905207/diff/8002/net/android/java/src/org/ch...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

http://codereview.chromium.org/10905207/diff/8002/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:95: *        
as "name,address". i.e.

Nit: change "i.e." to "For example" or "e.g.".

http://codereview.chromium.org/10905207/diff/8002/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:135:
result.append(";");

This may result in extraneous semicolons because the next
element on addressList may be a loopback interface or an
interface that is down. We would also need to check
list.hasMoreElements().

I think it is better to replace lines 133-135 by the
following (pseudo Java code):
    if (result.length() != 0)
        result.append(";");
    result.append(addressString.toString());

http://codereview.chromium.org/10905207/diff/8002/net/android/network_library.h
File net/android/network_library.h (right):

http://codereview.chromium.org/10905207/diff/8002/net/android/network_library...
net/android/network_library.h:47: // Return a string containing network list
items, each item is a network name

Change "network list items" to "network interface items"
or just "network items".  I would probably use "a list of
network interfaces".

http://codereview.chromium.org/10905207/diff/8002/net/android/network_library...
net/android/network_library.h:48: // and address pair. i.e.
"eth0,10.0.0.2;eth0,fe80::5054:ff:fe12:3456" is a

Nit: I think "i.e." should be "e.g." because this is an example.

http://codereview.chromium.org/10905207/diff/8002/net/base/net_util_posix.cc
File net/base/net_util_posix.cc (right):

http://codereview.chromium.org/10905207/diff/8002/net/base/net_util_posix.cc#...
net/base/net_util_posix.cc:18: #endif

The Chromium Coding Style page recommends that platform-specific
headers be included last:
http://dev.chromium.org/developers/coding-style#TOC-Platform-defines

I suggest that you move this after
  #include <netinet/in.h>
separated with a blank line.

[Shouqun Liu, 2012-09-12 01:06:07]

wtc, thanks for your comments, I have fixed the issues and nits based on your
comments.

http://codereview.chromium.org/10905207/diff/8002/net/android/java/src/org/ch...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

http://codereview.chromium.org/10905207/diff/8002/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:95: *        
as "name,address". i.e.
On 2012/09/11 23:04:49, wtc wrote:
> 
> Nit: change "i.e." to "For example" or "e.g.".

Done.

http://codereview.chromium.org/10905207/diff/8002/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:135:
result.append(";");
oh, yes, I did not consider all the cases, fixed.
On 2012/09/11 23:04:49, wtc wrote:
> 
> This may result in extraneous semicolons because the next
> element on addressList may be a loopback interface or an
> interface that is down. We would also need to check
> list.hasMoreElements().
> 
> I think it is better to replace lines 133-135 by the
> following (pseudo Java code):
>     if (result.length() != 0)
>         result.append(";");
>     result.append(addressString.toString());

http://codereview.chromium.org/10905207/diff/8002/net/android/network_library.h
File net/android/network_library.h (right):

http://codereview.chromium.org/10905207/diff/8002/net/android/network_library...
net/android/network_library.h:47: // Return a string containing network list
items, each item is a network name
On 2012/09/11 23:04:49, wtc wrote:
> 
> Change "network list items" to "network interface items"
> or just "network items".  I would probably use "a list of
> network interfaces".

Done.

http://codereview.chromium.org/10905207/diff/8002/net/android/network_library...
net/android/network_library.h:48: // and address pair. i.e.
"eth0,10.0.0.2;eth0,fe80::5054:ff:fe12:3456" is a
On 2012/09/11 23:04:49, wtc wrote:
> 
> Nit: I think "i.e." should be "e.g." because this is an example.

Done.

http://codereview.chromium.org/10905207/diff/8002/net/base/net_util_posix.cc
File net/base/net_util_posix.cc (right):

http://codereview.chromium.org/10905207/diff/8002/net/base/net_util_posix.cc#...
net/base/net_util_posix.cc:18: #endif
On 2012/09/11 23:04:49, wtc wrote:
> 
> The Chromium Coding Style page recommends that platform-specific
> headers be included last:
> http://dev.chromium.org/developers/coding-style#TOC-Platform-defines
> 
> I suggest that you move this after
>   #include <netinet/in.h>
> separated with a blank line.

Done.

[wtc, 2012-09-12 01:48:47]

Patch set 3 LGTM. Thanks!

[commit-bot: I haz the power, 2012-09-12 01:53:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shouqun.liu@intel.com/10905207/12

[commit-bot: I haz the power, 2012-09-12 04:21:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shouqun.liu@intel.com/10905207/12

[commit-bot: I haz the power, 2012-09-12 08:43:28]

Change committed as 156257