Out-of-process implementation of the PPB_NetworkMonitor_Private interface.

ppapi/proxy/ppb_network_monitor_private_proxy.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ppapi/proxy/ppb_network_monitor_private_proxy.h"
+
+#include "ppapi/proxy/enter_proxy.h"
+#include "ppapi/proxy/plugin_proxy_delegate.h"
+#include "ppapi/proxy/ppapi_messages.h"
+#include "ppapi/thunk/ppb_network_monitor_private_api.h"
+
+namespace ppapi {
+namespace proxy {
+
+class PPB_NetworkMonitor_Private_Proxy::NetworkMonitor
+    : public Resource,
+      public thunk::PPB_NetworkMonitor_Private_API {
+ public:
+  NetworkMonitor(PP_Instance instance,
+                 PPB_NetworkMonitor_Private_Proxy* proxy,
+                 PPB_NetworkMonitor_Callback callback,
+                 void* user_data)
+      : Resource(OBJECT_IS_PROXY, instance),
+        proxy_(proxy),
+        callback_(callback),
+        user_data_(user_data) {
+  }
+
+  virtual ~NetworkMonitor() {
+    proxy_->OnNetworkMonitorDeleted(this, pp_instance());

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

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.

[Sergey Ulanov, 2012/03/15 22:18:00]

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.

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

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...

[Sergey Ulanov, 2012/03/16 23:37:28]

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.

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

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.
Okay, thanks. Given your explanation, if you want to move them back inline,
that's fine with me too.

[Sergey Ulanov, 2012/03/19 20:43:29]

Done.

+  }
+
+  // Resource overrides.
+  ppapi::thunk::PPB_NetworkMonitor_Private_API*
+  AsPPB_NetworkMonitor_Private_API() {
+    return this;
+  }
+
+  void OnNetworkList(const scoped_refptr<NetworkListStorage>& list) {

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

Can you think of a better name , like...  OnReceiveNetworkList or something, to
make it clear what's happening with the NetworkList?

[Sergey Ulanov, 2012/03/15 22:18:00]

Renamed to OnNetworkListReceived()

+    PP_Resource list_resource =
+        PPB_NetworkList_Private_Shared::Create(
+            OBJECT_IS_PROXY, pp_instance(), list);
+    callback_(user_data_, list_resource);

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

Here, you need to use "CallWhileUnlocked" from ppapi/shared_impl/proxy_lock.h

[Sergey Ulanov, 2012/03/15 22:18:00]

Ah, that makes sense now!

+  }
+
+ private:
+  PPB_NetworkMonitor_Private_Proxy* proxy_;
+  PPB_NetworkMonitor_Callback callback_;
+  void* user_data_;
+
+  DISALLOW_COPY_AND_ASSIGN(NetworkMonitor);
+};
+
+PPB_NetworkMonitor_Private_Proxy::PPB_NetworkMonitor_Private_Proxy(
+    Dispatcher* dispatcher)
+    : InterfaceProxy(dispatcher),
+      monitors_(new ObserverListThreadSafe<NetworkMonitor>()),
+      monitors_count_(0) {
+}
+
+PPB_NetworkMonitor_Private_Proxy::~PPB_NetworkMonitor_Private_Proxy() {
+  monitors_->AssertEmpty();
+}
+
+// static
+PP_Resource PPB_NetworkMonitor_Private_Proxy::CreateProxyResource(
+    PP_Instance instance,
+    PPB_NetworkMonitor_Callback callback,
+    void* user_data) {
+  if (!callback)
+    return 0;
+
+  PluginDispatcher* dispatcher = PluginDispatcher::GetForInstance(instance);
+  if (!dispatcher)
+    return 0;
+  PPB_NetworkMonitor_Private_Proxy* proxy =
+      static_cast<PPB_NetworkMonitor_Private_Proxy*>(
+          dispatcher->GetInterfaceProxy(kApiID));
+  if (!proxy)
+    return 0;
+
+  scoped_ptr<NetworkMonitor> result(
+      new NetworkMonitor(instance, proxy, callback, user_data));
+  proxy->monitors_->AddObserver(result.get());
+
+  scoped_refptr<NetworkListStorage> current_list;
+
+  {
+    base::AutoLock lock(proxy->lock_);
+
+    proxy->monitors_count_++;
+    if (proxy->monitors_count_ == 1) {

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

ObserverList has size(), so I don't think you need your own count (after you
stop using the ThreadSafe version).

[Sergey Ulanov, 2012/03/15 22:18:00]

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.

+      // If that is the first network monitor then release the
+      // previous list, if any, and then send Start message.
+      proxy->current_list_ = NULL;

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

It seems cleaner to me to clear the list in OnNetworkMonitorDeleted if the
number of observers goes to zero.

[Sergey Ulanov, 2012/03/15 22:18:00]

Done.

+      PluginGlobals::Get()->plugin_proxy_delegate()->SendToBrowser(
+          new PpapiHostMsg_PPBNetworkMonitor_Start(
+              dispatcher->plugin_dispatcher_id()));
+    } else if (proxy->current_list_.get()) {
+      current_list = proxy->current_list_;
+    }
+  }
+
+  if (current_list.get())
+    result->OnNetworkList(current_list);

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

Shouldn't this be posted to happen asynchronously using PostTask?

[Sergey Ulanov, 2012/03/15 22:18:00]

Done.

+
+  return result.release()->GetReference();
+}
+
+bool PPB_NetworkMonitor_Private_Proxy::OnMessageReceived(
+    const IPC::Message& msg) {
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(PPB_NetworkMonitor_Private_Proxy, msg)
+    IPC_MESSAGE_HANDLER(PpapiMsg_PPBNetworkMonitor_NetworkList,
+                        OnPluginMsgNetworkList)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+  return handled;
+}
+
+void PPB_NetworkMonitor_Private_Proxy::OnPluginMsgNetworkList(
+    uint32 plugin_dispatcher_id,
+    const ppapi::NetworkList& list) {
+  scoped_refptr<NetworkListStorage> list_storage(new NetworkListStorage(list));
+
+  {
+    base::AutoLock lock(lock_);
+    current_list_ = list_storage;
+  }
+
+  monitors_->Notify(&NetworkMonitor::OnNetworkList, list_storage);
+}
+
+void PPB_NetworkMonitor_Private_Proxy::OnNetworkMonitorDeleted(
+    NetworkMonitor* monitor,
+    PP_Instance instance) {
+  monitors_->RemoveObserver(monitor);
+  {
+    base::AutoLock lock(lock_);
+    monitors_count_--;
+    if (monitors_count_ == 0) {
+      // Send Stop message if that was the last NetworkMonitor.
+      PluginDispatcher* dispatcher = PluginDispatcher::GetForInstance(instance);
+      if (dispatcher) {
+        PluginGlobals::Get()->plugin_proxy_delegate()->SendToBrowser(
+            new PpapiHostMsg_PPBNetworkMonitor_Stop(
+                dispatcher->plugin_dispatcher_id()));
+      }
+    }
+  }
+}
+
+}  // namespace proxy
+}  // namespace ppapi