Implement PPB_NetworkMonitor_Private Pepper interface.

Implement PPB_NetworkMonitor_Private Pepper interface.

The P2PSocketDispatcher::NetworkListObserver has been moved 
to webkit_glue:: namespace in webkit/glue/network_list_observer.h . PPB_NetworkMonitor_Private_Impl uses P2PSocketDispatcher to get 
notifications about network changes.

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

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

[dmichael (off chromium), 2012-03-01 18:58:04]

A test that gets some more coverage of NetworkList (and its C++ wrapper) would
be helpful.

http://codereview.chromium.org/9557006/diff/3001/ppapi/tests/test_network_mon...
File ppapi/tests/test_network_monitor_private.cc (right):

http://codereview.chromium.org/9557006/diff/3001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:18: CallbackData(PP_Instance
instance)
nit: explicit

http://codereview.chromium.org/9557006/diff/3001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:25: PP_Resource list_resource;
You probably should add a destructor that releases list_resource, right?

http://codereview.chromium.org/9557006/diff/3001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:29: CallbackData* data =
reinterpret_cast<CallbackData*>(user_data);
nit: static_cast

http://codereview.chromium.org/9557006/diff/3001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:35:
pp::Module::Get()->core()->ReleaseResource(network_list);
Can you explain what's going on? You're saving the first network_list, but only
the first?

http://codereview.chromium.org/9557006/diff/3001/webkit/glue/network_list_obs...
File webkit/glue/network_list_observer.h (right):

http://codereview.chromium.org/9557006/diff/3001/webkit/glue/network_list_obs...
webkit/glue/network_list_observer.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: 2012

http://codereview.chromium.org/9557006/diff/3001/webkit/plugins/ppapi/ppb_net...
File webkit/plugins/ppapi/ppb_network_monitor_private_impl.cc (right):

http://codereview.chromium.org/9557006/diff/3001/webkit/plugins/ppapi/ppb_net...
webkit/plugins/ppapi/ppb_network_monitor_private_impl.cc:31: PP_Resource
PPB_NetworkMonitor_Private_Impl::Create(
nit: // static

http://codereview.chromium.org/9557006/diff/3001/webkit/plugins/ppapi/ppb_net...
webkit/plugins/ppapi/ppb_network_monitor_private_impl.cc:35:
scoped_refptr<PPB_NetworkMonitor_Private_Impl> t(
nit: t is not a very good name...

http://codereview.chromium.org/9557006/diff/3001/webkit/plugins/ppapi/ppb_net...
webkit/plugins/ppapi/ppb_network_monitor_private_impl.cc:56: const
net::NetworkInterfaceList& list) {
nit: 4-space indent

http://codereview.chromium.org/9557006/diff/3001/webkit/plugins/ppapi/ppb_net...
webkit/plugins/ppapi/ppb_network_monitor_private_impl.cc:58: new
net::NetworkInterfaceList(list));
optional suggestion: If you have to copy it anyway, maybe it would suffice to
just pass the whole list in as a const-ref, and let the
PPB_NetworkList_Private_Shared do the copy in its constructor?

[Sergey Ulanov, 2012-03-03 02:26:07]

http://codereview.chromium.org/9557006/diff/3001/ppapi/tests/test_network_mon...
File ppapi/tests/test_network_monitor_private.cc (right):

http://codereview.chromium.org/9557006/diff/3001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:18: CallbackData(PP_Instance
instance)
On 2012/03/01 18:58:04, dmichael wrote:
> nit: explicit

Done.

http://codereview.chromium.org/9557006/diff/3001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:25: PP_Resource list_resource;
On 2012/03/01 18:58:04, dmichael wrote:
> You probably should add a destructor that releases list_resource, right?

Done.

http://codereview.chromium.org/9557006/diff/3001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:29: CallbackData* data =
reinterpret_cast<CallbackData*>(user_data);
On 2012/03/01 18:58:04, dmichael wrote:
> nit: static_cast

Done.

http://codereview.chromium.org/9557006/diff/3001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:35:
pp::Module::Get()->core()->ReleaseResource(network_list);
On 2012/03/01 18:58:04, dmichael wrote:
> Can you explain what's going on? You're saving the first network_list, but
only
> the first?

Yes, that was the idea, but now I changed it to always save the resource.

http://codereview.chromium.org/9557006/diff/3001/webkit/glue/network_list_obs...
File webkit/glue/network_list_observer.h (right):

http://codereview.chromium.org/9557006/diff/3001/webkit/glue/network_list_obs...
webkit/glue/network_list_observer.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2012/03/01 18:58:04, dmichael wrote:
> nit: 2012

Done.

http://codereview.chromium.org/9557006/diff/3001/webkit/plugins/ppapi/ppb_net...
File webkit/plugins/ppapi/ppb_network_monitor_private_impl.cc (right):

http://codereview.chromium.org/9557006/diff/3001/webkit/plugins/ppapi/ppb_net...
webkit/plugins/ppapi/ppb_network_monitor_private_impl.cc:31: PP_Resource
PPB_NetworkMonitor_Private_Impl::Create(
On 2012/03/01 18:58:04, dmichael wrote:
> nit: // static

Done.

http://codereview.chromium.org/9557006/diff/3001/webkit/plugins/ppapi/ppb_net...
webkit/plugins/ppapi/ppb_network_monitor_private_impl.cc:35:
scoped_refptr<PPB_NetworkMonitor_Private_Impl> t(
On 2012/03/01 18:58:04, dmichael wrote:
> nit: t is not a very good name...

Done.

http://codereview.chromium.org/9557006/diff/3001/webkit/plugins/ppapi/ppb_net...
webkit/plugins/ppapi/ppb_network_monitor_private_impl.cc:56: const
net::NetworkInterfaceList& list) {
On 2012/03/01 18:58:04, dmichael wrote:
> nit: 4-space indent

Done.

http://codereview.chromium.org/9557006/diff/3001/webkit/plugins/ppapi/ppb_net...
webkit/plugins/ppapi/ppb_network_monitor_private_impl.cc:58: new
net::NetworkInterfaceList(list));
On 2012/03/01 18:58:04, dmichael wrote:
> optional suggestion: If you have to copy it anyway, maybe it would suffice to
> just pass the whole list in as a const-ref, and let the
> PPB_NetworkList_Private_Shared do the copy in its constructor?

Now we need to convert it to a different type, so I left copying here.

[dmichael (off chromium), 2012-03-05 19:22:00]

https://chromiumcodereview.appspot.com/9557006/diff/7001/ppapi/tests/test_net...
File ppapi/tests/test_network_monitor_private.cc (right):

https://chromiumcodereview.appspot.com/9557006/diff/7001/ppapi/tests/test_net...
ppapi/tests/test_network_monitor_private.cc:17: #define
ASSERT_SUCCESS(error_message) \
Please comment to explain what "SUCCESS" means here (empty string) and/or what
you're using this for (calling a separate method that follows the PPAPI test
convention of returning a non-empty string on error). I almost suggested a name
like "ASSERT_STRING_NONEMPTY", but after seeing ASSERT_SUCCESS in context, I
understand why you chose this name.

https://chromiumcodereview.appspot.com/9557006/diff/7001/ppapi/tests/test_net...
ppapi/tests/test_network_monitor_private.cc:19: return (error_message);
Maybe worth putting in a "do {...} while (false)" to
(a) force consistent use of a semi-colon and
(b) disallow weird constructs like:
  if (p)
    ASSERT_SUCCESS(foo);
  else
    DoSomethingElse();
In this case, the else would actually apply to the "if
(!(error_message).empty())" in your macro.

Alternatively, you could get rid of the macro...  it doesn't save that much code
or greatly improve readability.

https://chromiumcodereview.appspot.com/9557006/diff/7001/ppapi/tests/test_net...
ppapi/tests/test_network_monitor_private.cc:73: // Verify that there is at least
one interface
nit: might be clearer to say "network interface" or something, to distinguish
from PPAPI interfaces.

https://chromiumcodereview.appspot.com/9557006/diff/7001/ppapi/tests/test_net...
ppapi/tests/test_network_monitor_private.cc:77: // Iterate over all interface
and verify their properties.
nit: interface->interfaces

https://chromiumcodereview.appspot.com/9557006/diff/7001/ppapi/tests/test_net...
ppapi/tests/test_network_monitor_private.cc:79: 
style nit: extra carriage return

https://chromiumcodereview.appspot.com/9557006/diff/7001/ppapi/tests/test_net...
ppapi/tests/test_network_monitor_private.cc:93: ASSERT_TRUE(j !=
addresses[i].size);
A quick comment above this loop saying "Make sure the address is non-zero" or
whatever might be nice.

https://chromiumcodereview.appspot.com/9557006/diff/7001/ppapi/tests/test_net...
ppapi/tests/test_network_monitor_private.cc:98:
ASSERT_TRUE(!network_list.GetDisplayName(iface).empty());
why not use ASSERT_FALSE?

https://chromiumcodereview.appspot.com/9557006/diff/7001/ppapi/tests/test_net...
ppapi/tests/test_network_monitor_private.cc:130: pos !=
reinterpret_cast<char*>(&*addresses.end()); ++pos) {
I think dereferencing end() is technically undefined (though it should be fine
in practice), so it might be better to do something like:
reinterpret_cast<char*>(&addresses[0] + addresses.size())

[Sergey Ulanov, 2012-03-06 02:11:06]

http://codereview.chromium.org/9557006/diff/7001/ppapi/tests/test_network_mon...
File ppapi/tests/test_network_monitor_private.cc (right):

http://codereview.chromium.org/9557006/diff/7001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:17: #define
ASSERT_SUCCESS(error_message) \
On 2012/03/05 19:22:00, dmichael wrote:
> Please comment to explain what "SUCCESS" means here (empty string) and/or what
> you're using this for (calling a separate method that follows the PPAPI test
> convention of returning a non-empty string on error). I almost suggested a
name
> like "ASSERT_STRING_NONEMPTY", but after seeing ASSERT_SUCCESS in context, I
> understand why you chose this name.

That was actually copied from the TCPServerSocket tests. There is also
RUN_SUBTEST() in test_transport.cc that is almost the same as this one. Maybe we
should just move this macro to test_case.h? When I wrote test_transport.cc I
initially put RUN_SUBTEST() there, but then Brett advised against it because
that was the only test that needed that macro.

http://codereview.chromium.org/9557006/diff/7001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:19: return (error_message);
On 2012/03/05 19:22:00, dmichael wrote:
> Maybe worth putting in a "do {...} while (false)" to
> (a) force consistent use of a semi-colon and
> (b) disallow weird constructs like:
>   if (p)
>     ASSERT_SUCCESS(foo);
>   else
>     DoSomethingElse();
> In this case, the else would actually apply to the "if
> (!(error_message).empty())" in your macro.
> 
> Alternatively, you could get rid of the macro...  it doesn't save that much
code
> or greatly improve readability.

Done, but see my comment about RUN_SUBTEST() above.

http://codereview.chromium.org/9557006/diff/7001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:73: // Verify that there is at least
one interface
On 2012/03/05 19:22:00, dmichael wrote:
> nit: might be clearer to say "network interface" or something, to distinguish
> from PPAPI interfaces.

Done.

http://codereview.chromium.org/9557006/diff/7001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:77: // Iterate over all interface
and verify their properties.
On 2012/03/05 19:22:00, dmichael wrote:
> nit: interface->interfaces

Done.

http://codereview.chromium.org/9557006/diff/7001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:79: 
On 2012/03/05 19:22:00, dmichael wrote:
> style nit: extra carriage return

Done.

http://codereview.chromium.org/9557006/diff/7001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:93: ASSERT_TRUE(j !=
addresses[i].size);
On 2012/03/05 19:22:00, dmichael wrote:
> A quick comment above this loop saying "Make sure the address is non-zero" or
> whatever might be nice.

Done.

http://codereview.chromium.org/9557006/diff/7001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:98:
ASSERT_TRUE(!network_list.GetDisplayName(iface).empty());
On 2012/03/05 19:22:00, dmichael wrote:
> why not use ASSERT_FALSE?

Done.

http://codereview.chromium.org/9557006/diff/7001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:130: pos !=
reinterpret_cast<char*>(&*addresses.end()); ++pos) {
On 2012/03/05 19:22:00, dmichael wrote:
> I think dereferencing end() is technically undefined (though it should be fine
> in practice), so it might be better to do something like:
> reinterpret_cast<char*>(&addresses[0] + addresses.size())

Done.

[dmichael (off chromium), 2012-03-06 16:11:28]

I don't see your latest changes. Can you please re-upload?

http://codereview.chromium.org/9557006/diff/7001/ppapi/tests/test_network_mon...
File ppapi/tests/test_network_monitor_private.cc (right):

http://codereview.chromium.org/9557006/diff/7001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:17: #define
ASSERT_SUCCESS(error_message) \
On 2012/03/06 02:11:06, sergeyu wrote:
> On 2012/03/05 19:22:00, dmichael wrote:
> > Please comment to explain what "SUCCESS" means here (empty string) and/or
what
> > you're using this for (calling a separate method that follows the PPAPI test
> > convention of returning a non-empty string on error). I almost suggested a
> name
> > like "ASSERT_STRING_NONEMPTY", but after seeing ASSERT_SUCCESS in context, I
> > understand why you chose this name.
> 
> That was actually copied from the TCPServerSocket tests. There is also
> RUN_SUBTEST() in test_transport.cc that is almost the same as this one. Maybe
we
> should just move this macro to test_case.h? When I wrote test_transport.cc I
> initially put RUN_SUBTEST() there, but then Brett advised against it because
> that was the only test that needed that macro. 
Sure, we can do that since there are several uses. The tricky part will be
coming up with the right name. Maybe "ASSERT_SUBTEST_SUCCESS" would be
descriptive & short enough?

I think it makes sense to put that in to test_case.h in a separate CL, though.

[Sergey Ulanov, 2012-03-06 23:38:08]

Uploaded the new patchset now. Sorry.

http://codereview.chromium.org/9557006/diff/7001/ppapi/tests/test_network_mon...
File ppapi/tests/test_network_monitor_private.cc (right):

http://codereview.chromium.org/9557006/diff/7001/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:17: #define
ASSERT_SUCCESS(error_message) \
On 2012/03/06 16:11:28, dmichael wrote:
> On 2012/03/06 02:11:06, sergeyu wrote:
> > On 2012/03/05 19:22:00, dmichael wrote:
> > > Please comment to explain what "SUCCESS" means here (empty string) and/or
> what
> > > you're using this for (calling a separate method that follows the PPAPI
test
> > > convention of returning a non-empty string on error). I almost suggested a
> > name
> > > like "ASSERT_STRING_NONEMPTY", but after seeing ASSERT_SUCCESS in context,
I
> > > understand why you chose this name.
> > 
> > That was actually copied from the TCPServerSocket tests. There is also
> > RUN_SUBTEST() in test_transport.cc that is almost the same as this one.
Maybe
> we
> > should just move this macro to test_case.h? When I wrote test_transport.cc I
> > initially put RUN_SUBTEST() there, but then Brett advised against it because
> > that was the only test that needed that macro. 
> Sure, we can do that since there are several uses. The tricky part will be
> coming up with the right name. Maybe "ASSERT_SUBTEST_SUCCESS" would be
> descriptive & short enough?
> 
> I think it makes sense to put that in to test_case.h in a separate CL, though.

Done. I sent you a separate CL for that change.

[dmichael (off chromium), 2012-03-07 03:24:02]

lgtm

[commit-bot: I haz the power, 2012-03-07 22:12:07]

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

[commit-bot: I haz the power, 2012-03-07 22:12:17]

Presubmit check for 9557006-13006 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
webkit/glue/network_list_observer.h,content/renderer/pepper/pepper_plugin_delegate_impl.cc,webkit/glue/webkit_glue.gypi,content/renderer/pepper/pepper_plugin_delegate_impl.h

Presubmit checks took 2.2s to calculate.

[Sergey Ulanov, 2012-03-07 22:21:56]

+brettw for missing OWNER approvals.

[brettw, 2012-03-08 05:40:01]

LGTM rubberstamp

[commit-bot: I haz the power, 2012-03-08 06:30:42]

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

[commit-bot: I haz the power, 2012-03-08 06:30:50]

Presubmit check for 9557006-13006 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
webkit/glue/network_list_observer.h,webkit/glue/webkit_glue.gypi

Presubmit checks took 1.6s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Sergey Ulanov, 2012-03-08 06:39:01]

+darin for webkit/glue

[darin (slow to review), 2012-03-08 22:33:02]

webkit/glue/ LGTM

[commit-bot: I haz the power, 2012-03-08 22:36:28]

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

[commit-bot: I haz the power, 2012-03-09 01:35:29]

Change committed as 125761

[commit-bot: I haz the power, 2012-03-12 19:11:54]

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

[commit-bot: I haz the power, 2012-03-12 20:09:07]

Try job failure for 9557006-37001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-03-12 20:16:31]

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

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

Change committed as 126274