Out-of-process implementation of the PPB_NetworkMonitor_Private interface.

Out-of-process implementation of the PPB_NetworkMonitor_Private interface.

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

[dmichael (off chromium), 2012-03-15 20:01:28]

http://codereview.chromium.org/9677060/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9677060/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/pepper_message_filter.cc:38: #include
"ppapi/shared_impl/api_id.h"
nit: include order

http://codereview.chromium.org/9677060/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/pepper_message_filter.cc:767: BrowserThread::FILE,
FROM_HERE,
I think we're moving away from using the FILE thread. See this chromium-dev
thread for info:
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...

http://codereview.chromium.org/9677060/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/pepper_message_filter.cc:770:
base::Passed(list.Pass())));
Seeing you pass list.get() to the first callback after seeing |list| is scoped
confused me at first. It's a nice approach, but I hadn't seen it before. Is this
a common pattern I just haven't seen before?

If not, I think it warrants a comment that the scope of the list is actually
from creation until the completion of the SendNetworkList callback.

http://codereview.chromium.org/9677060/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/pepper_message_filter.cc:789:
CHECK_LE(network.address.size(), sizeof(network_copy.addresses[0].data));
can you use arraysize instead of sizeof? sizeof will silently tell you the size
of a pointer if |data| ever changes to a pointer instead of a statically-sized
array. (I agree we shouldn't make such a change to PP_NetAddress_Private, but a
little paranoia in Browser code is healthy)

http://codereview.chromium.org/9677060/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/pepper_message_filter.cc:791:
memcpy(network_copy.addresses[0].data, &(network.address.front()),
It's probably worth a CHECK that network.address is non-empty before calling
"network.address.front()"

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/interface_list.cc
File ppapi/proxy/interface_list.cc (right):

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/interface_list.cc#n...
ppapi/proxy/interface_list.cc:92: #include
"ppapi/proxy/ppb_network_monitor_private_proxy.h"
nit: include order

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
File ppapi/proxy/ppb_network_monitor_private_proxy.cc (right):

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:30:
proxy_->OnNetworkMonitorDeleted(this, pp_instance());
By putting the bodies of these functions in the class definition, you're telling
the compiler to inline them. Is that what you want? I think the destructor at
least should not be inlined, since it's virtual.

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:39: void OnNetworkList(const
scoped_refptr<NetworkListStorage>& list) {
Can you think of a better name , like...  OnReceiveNetworkList or something, to
make it clear what's happening with the NetworkList?

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:43: callback_(user_data_,
list_resource);
Here, you need to use "CallWhileUnlocked" from ppapi/shared_impl/proxy_lock.h

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:92: if (proxy->monitors_count_
== 1) {
ObserverList has size(), so I don't think you need your own count (after you
stop using the ThreadSafe version).

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:95: proxy->current_list_ =
NULL;
It seems cleaner to me to clear the list in OnNetworkMonitorDeleted if the
number of observers goes to zero.

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:105:
result->OnNetworkList(current_list);
Shouldn't this be posted to happen asynchronously using PostTask?

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
File ppapi/proxy/ppb_network_monitor_private_proxy.h (right):

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.h:11: #include
"base/synchronization/lock.h"
You shouldn't need this include (and it's probably fine to use a non-thread-safe
observer list). See comment below.

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.h:27:
PPB_NetworkMonitor_Private_Proxy(Dispatcher* dispatcher);
nit: explicit

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.h:30: // Creates an NetworkManager
object in the plugin process.
nit: an->a

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.h:56: base::Lock lock_;
The entire proxy will be behind 1 big lock, so this lock is unnecessary. You can
enable this in your local build if you want by building with
"enable_pepper_threading=1". It's not on by default yet until it's better tested
and feature complete.

[Sergey Ulanov, 2012-03-15 22:17:59]

http://codereview.chromium.org/9677060/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9677060/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/pepper_message_filter.cc:38: #include
"ppapi/shared_impl/api_id.h"
On 2012/03/15 20:01:28, dmichael wrote:
> nit: include order

Done.

http://codereview.chromium.org/9677060/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/pepper_message_filter.cc:767: BrowserThread::FILE,
FROM_HERE,
On 2012/03/15 20:01:28, dmichael wrote:
> I think we're moving away from using the FILE thread. See this chromium-dev
> thread for info:
>
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...

Done. Changed it to PostBlockingPoolTask()

http://codereview.chromium.org/9677060/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/pepper_message_filter.cc:770:
base::Passed(list.Pass())));
On 2012/03/15 20:01:28, dmichael wrote:
> Seeing you pass list.get() to the first callback after seeing |list| is scoped
> confused me at first. It's a nice approach, but I hadn't seen it before. Is
this
> a common pattern I just haven't seen before?

Sharing arguments between task and reply closure is recommended approach in
PostTaskAndReply(), see base/task_runner.h, but the example in that header uses
ref-counted objects. Not sure if there are any cases when the ownership is
passed using scoped_ptr<> but that may be because scoped_ptr<>.Pass() is
relatively new. There is some code that uses the same ownership semantics with
PostTaskAndReply() without using scoped_ptr<>, e.g. see GetSessionName() in 
chrome/browser/sync/util/get_session_name.cc.

In any case, there is PostBlockingPoolTask() in BrowserThread, but no
PostBlockingPoolTaskAndReply(), so I changed this code to post SendNetworkList()
from DoGetNetworkList().


> If not, I think it warrants a comment that the scope of the list is actually
> from creation until the completion of the SendNetworkList callback.

http://codereview.chromium.org/9677060/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/pepper_message_filter.cc:789:
CHECK_LE(network.address.size(), sizeof(network_copy.addresses[0].data));
On 2012/03/15 20:01:28, dmichael wrote:
> can you use arraysize instead of sizeof? sizeof will silently tell you the
size
> of a pointer if |data| ever changes to a pointer instead of a statically-sized
> array. (I agree we shouldn't make such a change to PP_NetAddress_Private, but
a
> little paranoia in Browser code is healthy)

Actually this code wasn't converting address correctly. I changed it to use
NetAddressPrivateImpl::IPEndPointToNetAddress().

http://codereview.chromium.org/9677060/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/pepper_message_filter.cc:791:
memcpy(network_copy.addresses[0].data, &(network.address.front()),
On 2012/03/15 20:01:28, dmichael wrote:
> It's probably worth a CHECK that network.address is non-empty before calling
> "network.address.front()"

NetAddressPrivateImpl::IPEndPointToNetAddress() is used instead now.

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/interface_list.cc
File ppapi/proxy/interface_list.cc (right):

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/interface_list.cc#n...
ppapi/proxy/interface_list.cc:92: #include
"ppapi/proxy/ppb_network_monitor_private_proxy.h"
On 2012/03/15 20:01:28, dmichael wrote:
> nit: include order

Done.

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
File ppapi/proxy/ppb_network_monitor_private_proxy.cc (right):

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:30:
proxy_->OnNetworkMonitorDeleted(this, pp_instance());
On 2012/03/15 20:01:28, dmichael wrote:
> By putting the bodies of these functions in the class definition, you're
telling
> the compiler to inline them. Is that what you want? I think the destructor at
> least should not be inlined, since it's virtual.

The compiler will inline these methods only when it is possible, and it is never
possible for virtual methods. It just makes this code more readable. I'll move
them if you disagree.

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:39: void OnNetworkList(const
scoped_refptr<NetworkListStorage>& list) {
On 2012/03/15 20:01:28, dmichael wrote:
> Can you think of a better name , like...  OnReceiveNetworkList or something,
to
> make it clear what's happening with the NetworkList?

Renamed to OnNetworkListReceived()

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:43: callback_(user_data_,
list_resource);
On 2012/03/15 20:01:28, dmichael wrote:
> Here, you need to use "CallWhileUnlocked" from ppapi/shared_impl/proxy_lock.h

Ah, that makes sense now!

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:92: if (proxy->monitors_count_
== 1) {
On 2012/03/15 20:01:28, dmichael wrote:
> ObserverList has size(), so I don't think you need your own count (after you
> stop using the ThreadSafe version).

I believe that we need ThreadSafe observer list because we want to invoke
callbacks on the same threads on which the network monitor is created.

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:95: proxy->current_list_ =
NULL;
On 2012/03/15 20:01:28, dmichael wrote:
> It seems cleaner to me to clear the list in OnNetworkMonitorDeleted if the
> number of observers goes to zero.

Done.

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:105:
result->OnNetworkList(current_list);
On 2012/03/15 20:01:28, dmichael wrote:
> Shouldn't this be posted to happen asynchronously using PostTask?

Done.

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
File ppapi/proxy/ppb_network_monitor_private_proxy.h (right):

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.h:11: #include
"base/synchronization/lock.h"
On 2012/03/15 20:01:28, dmichael wrote:
> You shouldn't need this include (and it's probably fine to use a
non-thread-safe
> observer list). See comment below.

Done.

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.h:27:
PPB_NetworkMonitor_Private_Proxy(Dispatcher* dispatcher);
On 2012/03/15 20:01:28, dmichael wrote:
> nit: explicit

Done.

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.h:30: // Creates an NetworkManager
object in the plugin process.
On 2012/03/15 20:01:28, dmichael wrote:
> nit: an->a

Done.

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.h:56: base::Lock lock_;
On 2012/03/15 20:01:28, dmichael wrote:
> The entire proxy will be behind 1 big lock, so this lock is unnecessary. You
can
> enable this in your local build if you want by building with
> "enable_pepper_threading=1". It's not on by default yet until it's better
tested
> and feature complete.

I didn't know about it. Will it properly handle the case when a pepper interface
method is invoked from a callback, i.e. when the lock is already acquired by the
same thread?

Removed the lock.

[dmichael (off chromium), 2012-03-16 19:55:22]

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
File ppapi/proxy/ppb_network_monitor_private_proxy.cc (right):

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:30:
proxy_->OnNetworkMonitorDeleted(this, pp_instance());
On 2012/03/15 22:18:00, sergeyu wrote:
> On 2012/03/15 20:01:28, dmichael wrote:
> > By putting the bodies of these functions in the class definition, you're
> telling
> > the compiler to inline them. Is that what you want? I think the destructor
at
> > least should not be inlined, since it's virtual.
> 
> The compiler will inline these methods only when it is possible, and it is
never
> possible for virtual methods. It just makes this code more readable. I'll move
> them if you disagree.
Yes, please move the destructor implementation out. It is possible in some cases
for the compiler to inline virtual functions. In any case, our Clang style
checker should flag it if you don't move it:
http://www.chromium.org/developers/coding-style/chromium-style-checker-errors
It's a relatively minor thing, but it is agreed-on chromium style.
See also:
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...

http://codereview.chromium.org/9677060/diff/5021/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9677060/diff/5021/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:758: if
(!monitoring_networks_) {
It might be cleaner to get rid of monitoring_networks_ and just use whether the
set is empty, so you can't make a mistake where the bool is inconsistent with
the state of the set.

http://codereview.chromium.org/9677060/diff/5021/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:856: void
PepperMessageFilter::GetAndSendNetworkList() {
since you're posting the reply back to the IO thread, do you want to DCHECK that
you're on the IO thread here?

http://codereview.chromium.org/9677060/diff/5021/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.h (right):

http://codereview.chromium.org/9677060/diff/5021/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.h:109: typedef
std::map<uint32, linked_ptr<PepperTCPSocket> > TCPSocketMap;
It would be good to comment what the uint32 key represents for these (or typedef
to something self-explanatory, if you prefer)

http://codereview.chromium.org/9677060/diff/5021/ppapi/proxy/ppb_network_moni...
File ppapi/proxy/ppb_network_monitor_private_proxy.cc (right):

http://codereview.chromium.org/9677060/diff/5021/ppapi/proxy/ppb_network_moni...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:38: return this;
ditto here

http://codereview.chromium.org/9677060/diff/5021/ppapi/proxy/ppb_network_moni...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:119:
monitors_->Notify(&NetworkMonitor::OnNetworkListReceived, list_storage);
Have you thought about corner cases due to the asynchronous nature of the
Start/Stop/NetworkList messages?

E.g., it seems there's a bug if you have a sequence of events like this:
0) Suppose you have 1 NetworkMonitor.
1) The browser sends a message that the NetworkList was updated.
2) Before that message is received, the 1 NetworkMonitor goes away, so we send
"Stop" to the browser.
3) The OnPluginMsgNetworkList message arrives here, so we set current_list_ to
this new list. The Notify does nothing, since there are no observers.
4) Some time later, another NetworkMonitor is created. Because current_list_ is
valid, we send that to the NetworkMonitor. So before it gets the "real" network
list, it gets one that's possibly out-of-date.

I guess you do need to set proxy->current_list_ to NULL above. (It probably
makes sense to NULL it in both). Sorry about that.

You also could have a slightly weird situation if the first NetworkMonitor is
created and destroyed quickly, before it receives its network list. Another one
might register afterwards and receive the NetworkList that was intended for the
first NetworkMonitor. I can't really see the harm in this case, though. (it
would matter if we had to make sure that we never got non-consecutive
"updates").

Basically, I want to make sure you've thought through the cases. Doing an
observer distributed across processes gets tricky.

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

http://codereview.chromium.org/9677060/diff/5021/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:102: for (j = 0; j < sizeof(ip[i]);
++j) {
I think you mean sizeof(ip)?

http://codereview.chromium.org/9677060/diff/5021/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:194: instance_, &TestCallback,
reinterpret_cast<void*>(&callback_data));
is the reinterpret_cast necessary?

[dmichael (off chromium), 2012-03-16 20:07:25]

BTW, you'll need an owner for the content/browser stuff. That's probably a good
thing, since I'm not that familiar with that area anyway.

[dmichael (off chromium), 2012-03-16 20:27:17]

http://codereview.chromium.org/9677060/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9677060/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/pepper_message_filter.cc:770:
base::Passed(list.Pass())));
On 2012/03/15 22:18:00, sergeyu wrote:
> On 2012/03/15 20:01:28, dmichael wrote:
> > Seeing you pass list.get() to the first callback after seeing |list| is
scoped
> > confused me at first. It's a nice approach, but I hadn't seen it before. Is
> this
> > a common pattern I just haven't seen before?
> 
> Sharing arguments between task and reply closure is recommended approach in
> PostTaskAndReply(), see base/task_runner.h, but the example in that header
uses
> ref-counted objects. Not sure if there are any cases when the ownership is
> passed using scoped_ptr<> but that may be because scoped_ptr<>.Pass() is
> relatively new. There is some code that uses the same ownership semantics with
> PostTaskAndReply() without using scoped_ptr<>, e.g. see GetSessionName() in 
> chrome/browser/sync/util/get_session_name.cc.
> 
> In any case, there is PostBlockingPoolTask() in BrowserThread, but no
> PostBlockingPoolTaskAndReply(), so I changed this code to post
SendNetworkList()
> from DoGetNetworkList().
> 
> 
> > If not, I think it warrants a comment that the scope of the list is actually
> > from creation until the completion of the SendNetworkList callback.
> 

You might be able to use base/task_runner.h (I'm not that familiar with this
stuff, though).

[Sergey Ulanov, 2012-03-16 23:37:27]

http://codereview.chromium.org/9677060/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9677060/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/pepper_message_filter.cc:770:
base::Passed(list.Pass())));
On 2012/03/16 20:27:17, dmichael wrote:
> On 2012/03/15 22:18:00, sergeyu wrote:
> > On 2012/03/15 20:01:28, dmichael wrote:
> > > Seeing you pass list.get() to the first callback after seeing |list| is
> scoped
> > > confused me at first. It's a nice approach, but I hadn't seen it before.
Is
> > this
> > > a common pattern I just haven't seen before?
> > 
> > Sharing arguments between task and reply closure is recommended approach in
> > PostTaskAndReply(), see base/task_runner.h, but the example in that header
> uses
> > ref-counted objects. Not sure if there are any cases when the ownership is
> > passed using scoped_ptr<> but that may be because scoped_ptr<>.Pass() is
> > relatively new. There is some code that uses the same ownership semantics
with
> > PostTaskAndReply() without using scoped_ptr<>, e.g. see GetSessionName() in 
> > chrome/browser/sync/util/get_session_name.cc.
> > 
> > In any case, there is PostBlockingPoolTask() in BrowserThread, but no
> > PostBlockingPoolTaskAndReply(), so I changed this code to post
> SendNetworkList()
> > from DoGetNetworkList().
> > 
> > 
> > > If not, I think it warrants a comment that the scope of the list is
actually
> > > from creation until the completion of the SendNetworkList callback.
> > 
> 
> You might be able to use base/task_runner.h (I'm not that familiar with this
> stuff, though).

Done.

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
File ppapi/proxy/ppb_network_monitor_private_proxy.cc (right):

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:30:
proxy_->OnNetworkMonitorDeleted(this, pp_instance());
On 2012/03/16 19:55:22, dmichael wrote:
> On 2012/03/15 22:18:00, sergeyu wrote:
> > On 2012/03/15 20:01:28, dmichael wrote:
> > > By putting the bodies of these functions in the class definition, you're
> > telling
> > > the compiler to inline them. Is that what you want? I think the destructor
> at
> > > least should not be inlined, since it's virtual.
> > 
> > The compiler will inline these methods only when it is possible, and it is
> never
> > possible for virtual methods. It just makes this code more readable. I'll
move
> > them if you disagree.
> Yes, please move the destructor implementation out. It is possible in some
cases
> for the compiler to inline virtual functions. In any case, our Clang style
> checker should flag it if you don't move it:
> http://www.chromium.org/developers/coding-style/chromium-style-checker-errors
> It's a relatively minor thing, but it is agreed-on chromium style.
> See also:
>
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...

Clang style checker only checks that constructor/destructor are not inlined when
the class is declared in a header. That makes sense because header may be
included it multiple files, and so the compiler would compile each
constructor/destructor more than once. That slows down build. It's not a problem
when the class is declared in a .cc file because it's always compiled only once.
Anyway I moved all methods outside the class declaration.

http://codereview.chromium.org/9677060/diff/5021/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9677060/diff/5021/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:758: if
(!monitoring_networks_) {
On 2012/03/16 19:55:22, dmichael wrote:
> It might be cleaner to get rid of monitoring_networks_ and just use whether
the
> set is empty, so you can't make a mistake where the bool is inconsistent with
> the state of the set.

Done.

http://codereview.chromium.org/9677060/diff/5021/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:856: void
PepperMessageFilter::GetAndSendNetworkList() {
On 2012/03/16 19:55:22, dmichael wrote:
> since you're posting the reply back to the IO thread, do you want to DCHECK
that
> you're on the IO thread here?

Done.

http://codereview.chromium.org/9677060/diff/5021/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.h (right):

http://codereview.chromium.org/9677060/diff/5021/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.h:109: typedef
std::map<uint32, linked_ptr<PepperTCPSocket> > TCPSocketMap;
On 2012/03/16 19:55:22, dmichael wrote:
> It would be good to comment what the uint32 key represents for these (or
typedef
> to something self-explanatory, if you prefer)

Done.

http://codereview.chromium.org/9677060/diff/5021/ppapi/proxy/ppb_network_moni...
File ppapi/proxy/ppb_network_monitor_private_proxy.cc (right):

http://codereview.chromium.org/9677060/diff/5021/ppapi/proxy/ppb_network_moni...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:38: return this;
On 2012/03/16 19:55:22, dmichael wrote:
> ditto here

Done.

http://codereview.chromium.org/9677060/diff/5021/ppapi/proxy/ppb_network_moni...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:119:
monitors_->Notify(&NetworkMonitor::OnNetworkListReceived, list_storage);
On 2012/03/16 19:55:22, dmichael wrote:
> Have you thought about corner cases due to the asynchronous nature of the
> Start/Stop/NetworkList messages?
> 
> E.g., it seems there's a bug if you have a sequence of events like this:
> 0) Suppose you have 1 NetworkMonitor.
> 1) The browser sends a message that the NetworkList was updated.
> 2) Before that message is received, the 1 NetworkMonitor goes away, so we send
> "Stop" to the browser.
> 3) The OnPluginMsgNetworkList message arrives here, so we set current_list_ to
> this new list. The Notify does nothing, since there are no observers.
> 4) Some time later, another NetworkMonitor is created. Because current_list_
is
> valid, we send that to the NetworkMonitor. So before it gets the "real"
network
> list, it gets one that's possibly out-of-date.

Heh, that scenario is the reason I initially had the code that would resets
|current_list_| when sending Start() message (patch set 1). I've put it back
with comments.


> 
> I guess you do need to set proxy->current_list_ to NULL above. (It probably
> makes sense to NULL it in both). Sorry about that.
> 
> You also could have a slightly weird situation if the first NetworkMonitor is
> created and destroyed quickly, before it receives its network list. Another
one
> might register afterwards and receive the NetworkList that was intended for
the
> first NetworkMonitor. I can't really see the harm in this case, though. (it
> would matter if we had to make sure that we never got non-consecutive
> "updates").
> 
> Basically, I want to make sure you've thought through the cases. Doing an
> observer distributed across processes gets tricky.

Yes, I thought about these cases. Because the interface is asynchronous it is
never possible to guarantee that the NetworkList we receive here represents the
current state of network configuration. More than that, it is not possible to
make this interface synchronous because there is no way for an application to
lock network interfaces configuration. It can change the moment getifaddrs()
returns, so NetworkList may be already stale even when the message is sent by
the browser. What's import for that interface is that (1) we get first
notification after NetworkMonitor instance is created, and (2) whenever network
configuration changes it gets notification about it. The current approach meets
these two requirements. In the scenario you described the worst that would
happen is that the plugin gets two notifications after NetworkMonitor is created
- but that's not a problem as long as the second notification correctly reflects
current network configuration.

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

http://codereview.chromium.org/9677060/diff/5021/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:102: for (j = 0; j < sizeof(ip[i]);
++j) {
On 2012/03/16 19:55:22, dmichael wrote:
> I think you mean sizeof(ip)?
Yes, thanks for catching it!

http://codereview.chromium.org/9677060/diff/5021/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:194: instance_, &TestCallback,
reinterpret_cast<void*>(&callback_data));
On 2012/03/16 19:55:22, dmichael wrote:
> is the reinterpret_cast necessary?

Done.

[Sergey Ulanov, 2012-03-16 23:39:31]

+brettw for content/browser OWNER approval

[dmichael (off chromium), 2012-03-19 16:25:08]

lgtm

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
File ppapi/proxy/ppb_network_monitor_private_proxy.cc (right):

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:30:
proxy_->OnNetworkMonitorDeleted(this, pp_instance());
On 2012/03/16 23:37:28, sergeyu wrote:
> On 2012/03/16 19:55:22, dmichael wrote:
> > On 2012/03/15 22:18:00, sergeyu wrote:
> > > On 2012/03/15 20:01:28, dmichael wrote:
> > > > By putting the bodies of these functions in the class definition, you're
> > > telling
> > > > the compiler to inline them. Is that what you want? I think the
destructor
> > at
> > > > least should not be inlined, since it's virtual.
> > > 
> > > The compiler will inline these methods only when it is possible, and it is
> > never
> > > possible for virtual methods. It just makes this code more readable. I'll
> move
> > > them if you disagree.
> > Yes, please move the destructor implementation out. It is possible in some
> cases
> > for the compiler to inline virtual functions. In any case, our Clang style
> > checker should flag it if you don't move it:
> >
http://www.chromium.org/developers/coding-style/chromium-style-checker-errors
> > It's a relatively minor thing, but it is agreed-on chromium style.
> > See also:
> >
>
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...
> 
> Clang style checker only checks that constructor/destructor are not inlined
when
> the class is declared in a header. That makes sense because header may be
> included it multiple files, and so the compiler would compile each
> constructor/destructor more than once. That slows down build. It's not a
problem
> when the class is declared in a .cc file because it's always compiled only
once.
Yeah, I thought you'd be stuck doing it anyway. Didn't realize the Clang checker
made that exception. If you want to inline it again, I'm okay with that. I agree
there's no harm in it here.
> Anyway I moved all methods outside the class declaration.
> 
Okay, thanks. Given your explanation, if you want to move them back inline,
that's fine with me too.

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

http://codereview.chromium.org/9677060/diff/5021/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:194: instance_, &TestCallback,
reinterpret_cast<void*>(&callback_data));
On 2012/03/16 23:37:28, sergeyu wrote:
> On 2012/03/16 19:55:22, dmichael wrote:
> > is the reinterpret_cast necessary?
> 
> Done.
nit: missed this one

[Sergey Ulanov, 2012-03-19 20:43:28]

brettw: ping

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
File ppapi/proxy/ppb_network_monitor_private_proxy.cc (right):

http://codereview.chromium.org/9677060/diff/1/ppapi/proxy/ppb_network_monitor...
ppapi/proxy/ppb_network_monitor_private_proxy.cc:30:
proxy_->OnNetworkMonitorDeleted(this, pp_instance());
On 2012/03/19 16:25:08, dmichael wrote:
> On 2012/03/16 23:37:28, sergeyu wrote:
> > On 2012/03/16 19:55:22, dmichael wrote:
> > > On 2012/03/15 22:18:00, sergeyu wrote:
> > > > On 2012/03/15 20:01:28, dmichael wrote:
> > > > > By putting the bodies of these functions in the class definition,
you're
> > > > telling
> > > > > the compiler to inline them. Is that what you want? I think the
> destructor
> > > at
> > > > > least should not be inlined, since it's virtual.
> > > > 
> > > > The compiler will inline these methods only when it is possible, and it
is
> > > never
> > > > possible for virtual methods. It just makes this code more readable.
I'll
> > move
> > > > them if you disagree.
> > > Yes, please move the destructor implementation out. It is possible in some
> > cases
> > > for the compiler to inline virtual functions. In any case, our Clang style
> > > checker should flag it if you don't move it:
> > >
> http://www.chromium.org/developers/coding-style/chromium-style-checker-errors
> > > It's a relatively minor thing, but it is agreed-on chromium style.
> > > See also:
> > >
> >
>
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...
> > 
> > Clang style checker only checks that constructor/destructor are not inlined
> when
> > the class is declared in a header. That makes sense because header may be
> > included it multiple files, and so the compiler would compile each
> > constructor/destructor more than once. That slows down build. It's not a
> problem
> > when the class is declared in a .cc file because it's always compiled only
> once.
> Yeah, I thought you'd be stuck doing it anyway. Didn't realize the Clang
checker
> made that exception. If you want to inline it again, I'm okay with that. I
agree
> there's no harm in it here.
> > Anyway I moved all methods outside the class declaration.
> > 
> Okay, thanks. Given your explanation, if you want to move them back inline,
> that's fine with me too.
> 

Done.

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

http://codereview.chromium.org/9677060/diff/5021/ppapi/tests/test_network_mon...
ppapi/tests/test_network_monitor_private.cc:194: instance_, &TestCallback,
reinterpret_cast<void*>(&callback_data));
On 2012/03/19 16:25:08, dmichael wrote:
> On 2012/03/16 23:37:28, sergeyu wrote:
> > On 2012/03/16 19:55:22, dmichael wrote:
> > > is the reinterpret_cast necessary?
> > 
> > Done.
> nit: missed this one

Done.

[brettw, 2012-03-19 20:52:16]

Sorry, LGTM rubberstamp

[commit-bot: I haz the power, 2012-03-19 22:27:48]

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

[commit-bot: I haz the power, 2012-03-19 22:27:54]

Can't process patch for file chrome/test/ui/ppapi_uitest.cc.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2012-03-19 22:30:52]

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

[commit-bot: I haz the power, 2012-03-19 23:28:10]

Try job failure for 9677060-24001 (retry) on linux_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-03-20 17:43:16]

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

[commit-bot: I haz the power, 2012-03-20 17:43:22]

Can't apply patch for file ppapi/proxy/resource_creation_proxy.cc.
While running patch -p1 --forward --force;
patching file ppapi/proxy/resource_creation_proxy.cc
Hunk #1 FAILED at 25.
Hunk #2 succeeded at 249 (offset 1 line).
1 out of 2 hunks FAILED -- saving rejects to file
ppapi/proxy/resource_creation_proxy.cc.rej

[commit-bot: I haz the power, 2012-03-20 17:47:53]

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

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

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

[commit-bot: I haz the power, 2012-03-20 18:59:36]

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

[commit-bot: I haz the power, 2012-03-20 20:19:44]

Change committed as 127762