Refactor RenderProcessHost to use IPC::Listener instead of RenderWidgetHost

content/browser/renderer_host/render_widget_host_impl.cc

Index: content/browser/renderer_host/render_widget_host_impl.cc
diff --git a/content/browser/renderer_host/render_widget_host_impl.cc b/content/browser/renderer_host/render_widget_host_impl.cc
index 6063e9e47a5a86b1919262fa5a32c7d4afbc5da2..4e42047b7b4791c3a9aef91e7c3c7e2c652c620b 100644
--- a/content/browser/renderer_host/render_widget_host_impl.cc
+++ b/content/browser/renderer_host/render_widget_host_impl.cc
@@ -11,6 +11,7 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
+#include "base/hash_tables.h"
 #include "base/i18n/rtl.h"
 #include "base/lazy_instance.h"
 #include "base/message_loop.h"
@@ -82,6 +83,43 @@ using WebKit::WebMouseEvent;
 using WebKit::WebMouseWheelEvent;
 using WebKit::WebTextDirection;
 
+namespace {
+
+struct RenderWidgetID {
+  RenderWidgetID(int32 process_id, int32 routing_id)
+    : process_id_(process_id),
+      routing_id_(routing_id) {}
+  size_t hash() const {
+    return (process_id_ << sizeof(int32)) | routing_id_;

[dcheng, 2013/06/11 00:34:58]

Is it intentional that you're only left shifting by 4?

+  }
+  bool operator==(const RenderWidgetID& rwh) const {
+    return process_id_ ==  rwh.process_id_ && routing_id_ == rwh.routing_id_;
+  }
+  bool operator<(const RenderWidgetID& rwh) const {
+    return hash() < rwh.hash();
+  }
+  int32 process_id_;

[dcheng, 2013/06/11 00:34:58]

Nit: No _ when naming structs.
Also, I might personally just use a union here but that's just preference (I
don't know what convention is).

+  int32 routing_id_;
+};
+
+} //  namespace
+
+namespace BASE_HASH_NAMESPACE {
+#if defined(COMPILER_GCC)
+template <>
+struct hash<RenderWidgetID> {
+  size_t operator()(const RenderWidgetID& rwh) const {
+    return rwh.hash();
+  }
+};
+#elif defined(COMPILER_MSVC)
+inline size_t hash_value(const RenderWidgetID& rwh) {
+  return rwh.hash();
+}
+#endif
+}  // namespace BASE_HASH_NAMESPACE
+
+
 namespace content {
 namespace {
 
@@ -117,9 +155,15 @@ float GetAccelerationRatio(float accelerated_delta, float unaccelerated_delta) {
 base::LazyInstance<std::vector<RenderWidgetHost::CreatedCallback> >
 g_created_callbacks = LAZY_INSTANCE_INITIALIZER;
 
+
 }  // namespace
 
 
+typedef base::hash_map<RenderWidgetID, RenderWidgetHostImpl*>
+    RoutingIDWidgetMap;
+static base::LazyInstance<RoutingIDWidgetMap> g_routing_id_widget_map =
+    LAZY_INSTANCE_INITIALIZER;
+
 // static
 void RenderWidgetHost::RemoveAllBackingStores() {
   BackingStoreManager::RemoveAllBackingStores();
@@ -198,7 +242,9 @@ RenderWidgetHostImpl::RenderWidgetHostImpl(RenderWidgetHostDelegate* delegate,
 
   is_threaded_compositing_enabled_ = IsThreadedCompositingEnabled();
 
-  process_->Attach(this, routing_id_);
+  g_routing_id_widget_map.Get().insert(std::make_pair(
+      RenderWidgetID(process->GetID(), routing_id_), this));
+  process_->AddRoute(routing_id_, this);
   // Because the widget initializes as is_hidden_ == false,
   // tell the process host that we're alive.
   process_->WidgetRestored();
@@ -225,13 +271,46 @@ RenderWidgetHostImpl::~RenderWidgetHostImpl() {
   GpuSurfaceTracker::Get()->RemoveSurface(surface_id_);
   surface_id_ = 0;
 
-  process_->Release(routing_id_);
+  process_->RemoveRoute(routing_id_);
+  g_routing_id_widget_map.Get().erase(
+      RenderWidgetID(process_->GetID(), routing_id_));
 
   if (delegate_)
     delegate_->RenderWidgetDeleted(this);
 }
 
 // static
+RenderWidgetHost* RenderWidgetHost::FromID(
+    int32 process_id,
+    int32 routing_id) {
+  return RenderWidgetHostImpl::FromID(process_id, routing_id);
+}
+
+// static
+RenderWidgetHostImpl* RenderWidgetHostImpl::FromID(
+    int32 process_id,
+    int32 routing_id) {
+  RoutingIDWidgetMap* widgets = g_routing_id_widget_map.Pointer();
+  RoutingIDWidgetMap::iterator it = widgets->find(
+      RenderWidgetID(process_id, routing_id));
+  return it == widgets->end() ? NULL : it->second;
+}
+
+// static
+scoped_ptr<std::vector<RenderWidgetHost*> >
+RenderWidgetHost::GetRenderWidgetHosts() {

[dcheng, 2013/06/11 00:34:58]

From what I can see, it's fine to just use return by value here (or pass a
pointer to the output hash_map). Note that a lot of code in Chrome already
depends on RVO though; for example,
render_view_devtools_agent_host.cc:DevToolsAgentHost::GetValidRenderViewHosts,
which your patch touches, depends on RVO.

Note another advantage to using RVO is you could make the returned value const
if you want to insure that it is not accidentally modified, which you can't do
if you pass it in by pointer.

+  scoped_ptr<std::vector<RenderWidgetHost*> > hosts(
+      new std::vector<RenderWidgetHost*>());
+  RoutingIDWidgetMap* widgets = g_routing_id_widget_map.Pointer();
+  for (RoutingIDWidgetMap::const_iterator it = widgets->begin();
+       it != widgets->end();
+       ++it) {
+    hosts->push_back(it->second);
+  }
+  return hosts.Pass();
+}
+
+// static
 RenderWidgetHostImpl* RenderWidgetHostImpl::From(RenderWidgetHost* rwh) {
   return rwh->AsRenderWidgetHostImpl();
 }
@@ -2543,10 +2622,8 @@ void RenderWidgetHostImpl::CompositorFrameDrawn(
     // Matches with GetLatencyComponentId
     int routing_id = b->first.second & 0xffffffff;
     int process_id = (b->first.second >> 32) & 0xffffffff;
-    RenderProcessHost* host = RenderProcessHost::FromID(process_id);
-    if (!host)
-      continue;
-    RenderWidgetHost* rwh = host->GetRenderWidgetHostByID(routing_id);
+    RenderWidgetHost* rwh =
+        RenderWidgetHost::FromID(process_id, routing_id);
     if (!rwh)
       continue;
     RenderWidgetHostImpl::From(rwh)->FrameSwapped(latency_info);