Add NetworkList/NetworkMonitor hooks and C++ wrappers.

Add NetworkList/NetworkMonitor hooks and C++ wrappers.

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

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

[Sergey Ulanov, 2012-03-01 00:54:55]

I'll send a separate CL with NetworkManager implementation.

http://codereview.chromium.org/9545010/diff/1/ppapi/api/private/ppb_network_l...
File ppapi/api/private/ppb_network_list_private.idl (right):

http://codereview.chromium.org/9545010/diff/1/ppapi/api/private/ppb_network_l...
ppapi/api/private/ppb_network_list_private.idl:116: [inout, size_is(count)]
PP_NetAddress_Private[] addresses,
changed this to inout because the caller is supposed to allocate the memory.
Otherwise the C interface is generated as pointer to array
(PP_NetAddress_Private* addresses[]), which is not what we need here.

[dmichael (off chromium), 2012-03-01 18:29:54]

Can you write a test yet, or will that require NetworkMonitor?

http://codereview.chromium.org/9545010/diff/1/ppapi/api/private/ppb_network_l...
File ppapi/api/private/ppb_network_list_private.idl (right):

http://codereview.chromium.org/9545010/diff/1/ppapi/api/private/ppb_network_l...
ppapi/api/private/ppb_network_list_private.idl:20: * Wired Ethernet network.
comment seems wrong

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_list_...
File ppapi/cpp/private/network_list_private.cc (right):

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_list_...
ppapi/cpp/private/network_list_private.cc:71: if (result < 0) {
Why not <= 0?

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_list_...
ppapi/cpp/private/network_list_private.cc:76: if (result <
static_cast<int32_t>(addresses->size())) {
Why not <=? Right now, if you get 3 back, you fall through to the case where you
call GetIpAddresses again, which seems wasteful.

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_list_...
File ppapi/cpp/private/network_list_private.h (right):

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_list_...
ppapi/cpp/private/network_list_private.h:20: // Returns true if the required
interface is available.
// -> ///

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_list_...
ppapi/cpp/private/network_list_private.h:23: /// @return Returns number of
available network interfaces or 0 if
Returns "the" number

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_list_...
ppapi/cpp/private/network_list_private.h:27: /// @return Returns name for the
network interface with the specified
"the" name (and similar for the other functions below)

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_monit...
File ppapi/cpp/private/network_monitor_private.h (right):

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_monit...
ppapi/cpp/private/network_monitor_private.h:17: explicit
NetworkMonitorPrivate(Instance* instance,
explicit isn't necessary.
Instance* should be InstanceHandle

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_monit...
ppapi/cpp/private/network_monitor_private.h:19: void* user_data);
You may want to put a class in ppapi/utility that defines a base class and a
virtual method to override. This class could take care of the user_data and
upcall part.

http://codereview.chromium.org/9545010/diff/1/ppapi/shared_impl/ppb_network_l...
File ppapi/shared_impl/ppb_network_list_private_shared.cc (right):

http://codereview.chromium.org/9545010/diff/1/ppapi/shared_impl/ppb_network_l...
ppapi/shared_impl/ppb_network_list_private_shared.cc:46: return
StringVar::StringToPPVar("");
you might consider returning an Undefined Var to signal error

http://codereview.chromium.org/9545010/diff/1/ppapi/shared_impl/ppb_network_l...
ppapi/shared_impl/ppb_network_list_private_shared.cc:59: // Currently we get
only interfaces that are active.
Then it doesn't really need to be an error condition, does it?

http://codereview.chromium.org/9545010/diff/1/ppapi/shared_impl/ppb_network_l...
ppapi/shared_impl/ppb_network_list_private_shared.cc:75:
memcpy(addresses[0].data, &(list_->at(index).address.front()),
You should probably return early if the address doesn't fit, since DCHECK only
protects debug builds. Since this could over-write memory if it goes wrong, I
think we should be really careful.

http://codereview.chromium.org/9545010/diff/1/ppapi/shared_impl/ppb_network_l...
ppapi/shared_impl/ppb_network_list_private_shared.cc:83: return
StringVar::StringToPPVar("");
It's probably better to return PP_MakeUndefined() for an error condition.

http://codereview.chromium.org/9545010/diff/1/ppapi/shared_impl/ppb_network_l...
File ppapi/shared_impl/ppb_network_list_private_shared.h (right):

http://codereview.chromium.org/9545010/diff/1/ppapi/shared_impl/ppb_network_l...
ppapi/shared_impl/ppb_network_list_private_shared.h:12: #include
"net/base/net_util.h"
You should try to avoid using this include if possible. We aren't relying on
anything from "net" yet, and we want to build the plugin side of the proxy in
NaCl eventually. The more dependencies we add, the harder that will be. You can
probably make your own simple struct and convert to/from it, right?

[Sergey Ulanov, 2012-03-02 03:01:37]

> Can you write a test yet, or will that require NetworkMonitor?

There is no way to create NetworkList without NetworkMonitor. I could write
unittests for PPB_NetworkList_Private_Shared, but not sure it will be very
useful (the class is very simple) and there aren't many other tests for shared
implementations.

http://codereview.chromium.org/9545010/diff/1/ppapi/api/private/ppb_network_l...
File ppapi/api/private/ppb_network_list_private.idl (right):

http://codereview.chromium.org/9545010/diff/1/ppapi/api/private/ppb_network_l...
ppapi/api/private/ppb_network_list_private.idl:20: * Wired Ethernet network.
On 2012/03/01 18:29:54, dmichael wrote:
> comment seems wrong

Done.

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_list_...
File ppapi/cpp/private/network_list_private.cc (right):

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_list_...
ppapi/cpp/private/network_list_private.cc:71: if (result < 0) {
On 2012/03/01 18:29:54, dmichael wrote:
> Why not <= 0?

"result = 0" is a not an error condition - it just means that there are no IP
addresses and this if is here to handle errors. Yes, it would work just fine if
I wrote "<=0" here, but I think it's cleaner to keep error case separate.
result=0 is handled by the code below.

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_list_...
ppapi/cpp/private/network_list_private.cc:76: if (result <
static_cast<int32_t>(addresses->size())) {
On 2012/03/01 18:29:54, dmichael wrote:
> Why not <=? Right now, if you get 3 back, you fall through to the case where
you
> call GetIpAddresses again, which seems wasteful.

Done.

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_list_...
File ppapi/cpp/private/network_list_private.h (right):

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_list_...
ppapi/cpp/private/network_list_private.h:20: // Returns true if the required
interface is available.
On 2012/03/01 18:29:54, dmichael wrote:
> // -> ///

Done.

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_list_...
ppapi/cpp/private/network_list_private.h:23: /// @return Returns number of
available network interfaces or 0 if
On 2012/03/01 18:29:54, dmichael wrote:
> Returns "the" number

Done.

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_list_...
ppapi/cpp/private/network_list_private.h:27: /// @return Returns name for the
network interface with the specified
On 2012/03/01 18:29:54, dmichael wrote:
> "the" name (and similar for the other functions below)

Done.

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_monit...
File ppapi/cpp/private/network_monitor_private.h (right):

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_monit...
ppapi/cpp/private/network_monitor_private.h:17: explicit
NetworkMonitorPrivate(Instance* instance,
On 2012/03/01 18:29:54, dmichael wrote:
> explicit isn't necessary.
> Instance* should be InstanceHandle

Done.

http://codereview.chromium.org/9545010/diff/1/ppapi/cpp/private/network_monit...
ppapi/cpp/private/network_monitor_private.h:19: void* user_data);
On 2012/03/01 18:29:54, dmichael wrote:
> You may want to put a class in ppapi/utility that defines a base class and a
> virtual method to override. This class could take care of the user_data and
> upcall part.

I agree that may be useful. Will do it later in a separate CL.

http://codereview.chromium.org/9545010/diff/1/ppapi/shared_impl/ppb_network_l...
File ppapi/shared_impl/ppb_network_list_private_shared.cc (right):

http://codereview.chromium.org/9545010/diff/1/ppapi/shared_impl/ppb_network_l...
ppapi/shared_impl/ppb_network_list_private_shared.cc:46: return
StringVar::StringToPPVar("");
On 2012/03/01 18:29:54, dmichael wrote:
> you might consider returning an Undefined Var to signal error

Done.

http://codereview.chromium.org/9545010/diff/1/ppapi/shared_impl/ppb_network_l...
ppapi/shared_impl/ppb_network_list_private_shared.cc:59: // Currently we get
only interfaces that are active.
On 2012/03/01 18:29:54, dmichael wrote:
> Then it doesn't really need to be an error condition, does it?

Done.

http://codereview.chromium.org/9545010/diff/1/ppapi/shared_impl/ppb_network_l...
ppapi/shared_impl/ppb_network_list_private_shared.cc:75:
memcpy(addresses[0].data, &(list_->at(index).address.front()),
On 2012/03/01 18:29:54, dmichael wrote:
> You should probably return early if the address doesn't fit, since DCHECK only
> protects debug builds. Since this could over-write memory if it goes wrong, I
> think we should be really careful.

Removed this code from this CL - will add it back in the CL with NeworkMonitor
implementation and fix it there.

http://codereview.chromium.org/9545010/diff/1/ppapi/shared_impl/ppb_network_l...
ppapi/shared_impl/ppb_network_list_private_shared.cc:83: return
StringVar::StringToPPVar("");
On 2012/03/01 18:29:54, dmichael wrote:
> It's probably better to return PP_MakeUndefined() for an error condition.

Done.

http://codereview.chromium.org/9545010/diff/1/ppapi/shared_impl/ppb_network_l...
File ppapi/shared_impl/ppb_network_list_private_shared.h (right):

http://codereview.chromium.org/9545010/diff/1/ppapi/shared_impl/ppb_network_l...
ppapi/shared_impl/ppb_network_list_private_shared.h:12: #include
"net/base/net_util.h"
On 2012/03/01 18:29:54, dmichael wrote:
> You should try to avoid using this include if possible. We aren't relying on
> anything from "net" yet, and we want to build the plugin side of the proxy in
> NaCl eventually. The more dependencies we add, the harder that will be. You
can
> probably make your own simple struct and convert to/from it, right?

I wanted to avoid having separate type to represent the same information, but I
agree that in that case it's the right thing to do. Done.

[dmichael (off chromium), 2012-03-02 16:43:12]

http://codereview.chromium.org/9545010/diff/5003/ppapi/cpp/private/network_mo...
File ppapi/cpp/private/network_monitor_private.h (right):

http://codereview.chromium.org/9545010/diff/5003/ppapi/cpp/private/network_mo...
ppapi/cpp/private/network_monitor_private.h:17: NetworkMonitorPrivate(Instance*
instance,
Instance* should be InstanceHandle now. See:
http://codereview.chromium.org/9381010/

http://codereview.chromium.org/9545010/diff/5003/ppapi/shared_impl/ppb_networ...
File ppapi/shared_impl/ppb_network_list_private_shared.cc (right):

http://codereview.chromium.org/9545010/diff/5003/ppapi/shared_impl/ppb_networ...
ppapi/shared_impl/ppb_network_list_private_shared.cc:14:
PPB_NetworkList_Private_Shared::NetworkInfo::NetworkInfo() {
You should probably initialize type, state, and mtu here.

http://codereview.chromium.org/9545010/diff/5003/ppapi/shared_impl/ppb_networ...
ppapi/shared_impl/ppb_network_list_private_shared.cc:80: return count;
If the caller passes a count less than the total available, don't you want to
return to them the number of addresses you have? As-is, I think you'll return
the number they passed you, and they don't have a way of knowing that there are
other addresses to get.

http://codereview.chromium.org/9545010/diff/5003/ppapi/shared_impl/ppb_networ...
ppapi/shared_impl/ppb_network_list_private_shared.cc:91: return
PP_NETWORKLIST_UNKNOWN;
This enum doesn't seem right here, since we're not talking about a
NetworkListType. I think just using 0 directly makes more sense here?

[Sergey Ulanov, 2012-03-02 18:43:03]

http://codereview.chromium.org/9545010/diff/5003/ppapi/cpp/private/network_mo...
File ppapi/cpp/private/network_monitor_private.h (right):

http://codereview.chromium.org/9545010/diff/5003/ppapi/cpp/private/network_mo...
ppapi/cpp/private/network_monitor_private.h:17: NetworkMonitorPrivate(Instance*
instance,
On 2012/03/02 16:43:12, dmichael wrote:
> Instance* should be InstanceHandle now. See:
> http://codereview.chromium.org/9381010/

Done. Other code uses const refs for InstanceHandle. What's the reason for that?
It doesn't look necessary because InstanceHandle objects are light in the
current implementation.

http://codereview.chromium.org/9545010/diff/5003/ppapi/shared_impl/ppb_networ...
File ppapi/shared_impl/ppb_network_list_private_shared.cc (right):

http://codereview.chromium.org/9545010/diff/5003/ppapi/shared_impl/ppb_networ...
ppapi/shared_impl/ppb_network_list_private_shared.cc:14:
PPB_NetworkList_Private_Shared::NetworkInfo::NetworkInfo() {
On 2012/03/02 16:43:12, dmichael wrote:
> You should probably initialize type, state, and mtu here.

Done.

http://codereview.chromium.org/9545010/diff/5003/ppapi/shared_impl/ppb_networ...
ppapi/shared_impl/ppb_network_list_private_shared.cc:80: return count;
On 2012/03/02 16:43:12, dmichael wrote:
> If the caller passes a count less than the total available, don't you want to
> return to them the number of addresses you have? As-is, I think you'll return
> the number they passed you, and they don't have a way of knowing that there
are
> other addresses to get.

Done. Thanks for catching this!

http://codereview.chromium.org/9545010/diff/5003/ppapi/shared_impl/ppb_networ...
ppapi/shared_impl/ppb_network_list_private_shared.cc:91: return
PP_NETWORKLIST_UNKNOWN;
On 2012/03/02 16:43:12, dmichael wrote:
> This enum doesn't seem right here, since we're not talking about a
> NetworkListType. I think just using 0 directly makes more sense here?

Done.

[dmichael (off chromium), 2012-03-02 19:08:51]

lgtm

http://codereview.chromium.org/9545010/diff/5003/ppapi/cpp/private/network_mo...
File ppapi/cpp/private/network_monitor_private.h (right):

http://codereview.chromium.org/9545010/diff/5003/ppapi/cpp/private/network_mo...
ppapi/cpp/private/network_monitor_private.h:17: NetworkMonitorPrivate(Instance*
instance,
On 2012/03/02 18:43:03, sergeyu wrote:
> On 2012/03/02 16:43:12, dmichael wrote:
> > Instance* should be InstanceHandle now. See:
> > http://codereview.chromium.org/9381010/
> 
> Done. Other code uses const refs for InstanceHandle. What's the reason for
that?
> It doesn't look necessary because InstanceHandle objects are light in the
> current implementation.
You could ask brettw if you want. I suspect you're right and it could just be by
value, but thanks for being consistent with the other code.

[commit-bot: I haz the power, 2012-03-02 20:37:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9545010/11006

[commit-bot: I haz the power, 2012-03-03 01:00:10]

Change committed as 124797

[commit-bot: I haz the power, 2012-03-03 02:32:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9545010/19002

[commit-bot: I haz the power, 2012-03-03 03:02:43]

Try job failure for 9545010-19002 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-03-05 06:43:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9545010/21001

[commit-bot: I haz the power, 2012-03-05 08:12:41]

Change committed as 124927