Browser Plugin: Reduce code repetition in BrowserPluginManager.

content/renderer/browser_plugin/browser_plugin_manager_impl.cc

Index: content/renderer/browser_plugin/browser_plugin_manager_impl.cc
diff --git a/content/renderer/browser_plugin/browser_plugin_manager_impl.cc b/content/renderer/browser_plugin/browser_plugin_manager_impl.cc
index a9d9e4598d27fa40b7d865ffb5c0d50f6aef75ee..aa09efc970848d43b3811266f9b6c6b5fe22e71e 100644
--- a/content/renderer/browser_plugin/browser_plugin_manager_impl.cc
+++ b/content/renderer/browser_plugin/browser_plugin_manager_impl.cc
@@ -15,6 +15,7 @@ namespace content {
 BrowserPluginManagerImpl::BrowserPluginManagerImpl(
     RenderViewImpl* render_view)
     : BrowserPluginManager(render_view) {
+  RegisterBrowserPluginMessages();
 }
 
 BrowserPluginManagerImpl::~BrowserPluginManagerImpl() {
@@ -24,7 +25,7 @@ BrowserPlugin* BrowserPluginManagerImpl::CreateBrowserPlugin(
     RenderViewImpl* render_view,
     WebKit::WebFrame* frame,
     const WebKit::WebPluginParams& params) {
-  return new BrowserPlugin(browser_plugin_counter_++,
+  return new BrowserPlugin(++browser_plugin_counter_,
                            render_view,
                            frame,
                            params);
@@ -36,25 +37,20 @@ bool BrowserPluginManagerImpl::Send(IPC::Message* msg) {
 
 bool BrowserPluginManagerImpl::OnMessageReceived(
     const IPC::Message& message) {
+  if (browser_plugin_messages_.count(message.type()) > 0) {

[Charlie Reis, 2012/12/14 18:24:02]

This feels like a mis-use of the IPC logic.  All the macros are designed to
route appropriate messages to the right instances, so it would be better if we
didn't go outside the system.

Is it possible to use "routed" IPC messages with routing_ids to accomplish this
same thing?

And do we even need BrowserPluginManager anymore if we go that route?

[Fady Samuel, 2012/12/14 18:47:48]

Regardless of the direction we go, BrowserPluginManager is still useful for
handling "control" messages that aren't directed at a particular BrowserPlugin:
see  OnPluginAtPositionRequest.

window.open CL also does some work in BrowserPluginManager that doesn't fit into
a particular BrowserPlugin: OnNewWindow.

We need to allocate route IDs from somewhere. My concern is what you're
suggesting will require some non-trivial changes to RenderThreadImpl and browser
process side changes (RenderProcessHostImpl::OnMessageReceived assumes a route
ID is a handle to a particular RenderWidgetHost)to ensure that a RenderView
doesn't end up with the same route ID as a BrowserPlugin.

Also, with the introduction of window.open, some instance_ids are allocated by
the browser process (negative IDs) and some by the BrowserPluginManager
(positive IDs). All route IDs are currently allocated by the browser process
IIRC (RenderWidgetHelper::GetNextRoutingID).

We have a BrowserPluginManager per RenderViewImpl, so BrowserPluginMsg_* are
routed to a particular BrowserPluginManager, which, from there, routes on
instance_id to a particular BrowserPlugin.

What your suggesting is certainly possible, but definitely not an easy task to
get right. Are you OK with my landing this as is?

[Charlie Reis, 2012/12/14 19:16:36]

Perhaps the class is still needed.
 
No, I was picturing a different ID space than RenderWidgets, and thus not
routing these through RenderThreadImpl.  Perhaps that's something that can be
discussed separately, including someone like John.
Negative IDs do not sound OK to me, but that's a topic for a different CL.
We can skip the larger change here, but it's worth a discussion for later.

+    int instance_id = 0;
+    PickleIterator iter(message);

[Charlie Reis, 2012/12/14 19:16:36]

Please document that all messages that we allow here must have instance_id as
their first parameter.

Also, we should get a security team reviewer on this code, as if it were in a
_messages.h file.

[Fady Samuel, 2012/12/14 19:34:08]

Done.

+    bool success = iter.ReadInt(&instance_id);
+    DCHECK(success);
+    BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
+    if (plugin && plugin->OnMessageReceived(message))
+      return true;
+  }
+
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(BrowserPluginManagerImpl, message)
-    IPC_MESSAGE_HANDLER(BrowserPluginMsg_UpdateRect, OnUpdateRect)
-    IPC_MESSAGE_HANDLER(BrowserPluginMsg_GuestGone, OnGuestGone)
-    IPC_MESSAGE_HANDLER(BrowserPluginMsg_AdvanceFocus, OnAdvanceFocus)
-    IPC_MESSAGE_HANDLER(BrowserPluginMsg_GuestContentWindowReady,
-                        OnGuestContentWindowReady)
-    IPC_MESSAGE_HANDLER(BrowserPluginMsg_ShouldAcceptTouchEvents,
-                        OnShouldAcceptTouchEvents)
-    IPC_MESSAGE_HANDLER(BrowserPluginMsg_LoadStart, OnLoadStart)
-    IPC_MESSAGE_HANDLER(BrowserPluginMsg_LoadAbort, OnLoadAbort)
-    IPC_MESSAGE_HANDLER(BrowserPluginMsg_LoadRedirect, OnLoadRedirect)
-    IPC_MESSAGE_HANDLER(BrowserPluginMsg_LoadCommit, OnLoadCommit)
-    IPC_MESSAGE_HANDLER(BrowserPluginMsg_LoadStop, OnLoadStop)
-    IPC_MESSAGE_HANDLER(BrowserPluginMsg_SetCursor, OnSetCursor)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_PluginAtPositionRequest,
                         OnPluginAtPositionRequest);
-    IPC_MESSAGE_HANDLER(BrowserPluginMsg_GuestUnresponsive, OnGuestUnresponsive)
-    IPC_MESSAGE_HANDLER(BrowserPluginMsg_GuestResponsive, OnGuestResponsive)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
@@ -88,101 +84,20 @@ void BrowserPluginManagerImpl::OnPluginAtPositionRequest(
        local_position));
 }
 
-void BrowserPluginManagerImpl::OnUpdateRect(
-    int instance_id,
-    int message_id,
-    const BrowserPluginMsg_UpdateRect_Params& params) {
-  BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
-  if (plugin)
-    plugin->UpdateRect(message_id, params);
-}
-
-void BrowserPluginManagerImpl::OnGuestGone(int instance_id,
-                                           int process_id,
-                                           int status) {
-  BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
-  if (plugin)
-    plugin->GuestGone(process_id, static_cast<base::TerminationStatus>(status));
+void BrowserPluginManagerImpl::RegisterBrowserPluginMessages() {

[Charlie Reis, 2012/12/14 19:16:36]

Instead of holding a set, just use a switch statement, like:
https://code.google.com/searchframe#OAMlx_jo-ck/src/content/common/swapped_ou...

[Fady Samuel, 2012/12/14 19:34:08]

I made this a set rather than a static switch statement because the set of
messages we're interested in might grow if a BrowserPluginObserver (coming in a
subsequent CL) requests more message types.

You can imagine a ChromeBrowserPlugin/ChromeBrowserPluginObserver that requests
the receipt of

ChromeBrowserPluginMsg_* messages. 

[Charlie Reis, 2012/12/14 19:40:32]

That doesn't imply that you need a set at runtime, though. 
SwappedOutMessages::CanHandleWhileSwappedOut calls
ContentClient::CanHandleWhileSwappedOut to let the embedder provide more
supported message types.

+  browser_plugin_messages_.insert(BrowserPluginMsg_UpdateRect::ID);
+  browser_plugin_messages_.insert(BrowserPluginMsg_GuestGone::ID);
+  browser_plugin_messages_.insert(BrowserPluginMsg_AdvanceFocus::ID);
+  browser_plugin_messages_.insert(BrowserPluginMsg_GuestContentWindowReady::ID);
+  browser_plugin_messages_.insert(BrowserPluginMsg_ShouldAcceptTouchEvents::ID);
+  browser_plugin_messages_.insert(BrowserPluginMsg_LoadStart::ID);
+  browser_plugin_messages_.insert(BrowserPluginMsg_LoadAbort::ID);
+  browser_plugin_messages_.insert(BrowserPluginMsg_LoadRedirect::ID);
+  browser_plugin_messages_.insert(BrowserPluginMsg_LoadCommit::ID);
+  browser_plugin_messages_.insert(BrowserPluginMsg_LoadStop::ID);
+  browser_plugin_messages_.insert(BrowserPluginMsg_SetCursor::ID);
+  browser_plugin_messages_.insert(BrowserPluginMsg_GuestUnresponsive::ID);
+  browser_plugin_messages_.insert(BrowserPluginMsg_GuestResponsive::ID);
 }
 
-void BrowserPluginManagerImpl::OnAdvanceFocus(int instance_id, bool reverse) {
-  BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
-  if (plugin)
-    plugin->AdvanceFocus(reverse);
-}
-
-void BrowserPluginManagerImpl::OnGuestContentWindowReady(int instance_id,
-                                                         int guest_routing_id) {
-  BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
-  if (plugin)
-    plugin->GuestContentWindowReady(guest_routing_id);
-}
-
-void BrowserPluginManagerImpl::OnShouldAcceptTouchEvents(int instance_id,
-                                                         bool accept) {
-  BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
-  if (plugin)
-    plugin->SetAcceptTouchEvents(accept);
-}
-
-void BrowserPluginManagerImpl::OnLoadStart(int instance_id,
-                                           const GURL& url,
-                                           bool is_top_level) {
-  BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
-  if (plugin)
-    plugin->LoadStart(url, is_top_level);
-}
-
-void BrowserPluginManagerImpl::OnLoadCommit(
-    int instance_id,
-    const BrowserPluginMsg_LoadCommit_Params& params) {
-  BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
-  if (plugin)
-    plugin->LoadCommit(params);
-}
-
-void BrowserPluginManagerImpl::OnLoadStop(int instance_id) {
-  BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
-  if (plugin)
-    plugin->LoadStop();
-}
-
-void BrowserPluginManagerImpl::OnLoadAbort(int instance_id,
-                                           const GURL& url,
-                                           bool is_top_level,
-                                           const std::string& type) {
-  BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
-  if (plugin)
-    plugin->LoadAbort(url, is_top_level, type);
-}
-
-void BrowserPluginManagerImpl::OnLoadRedirect(int instance_id,
-                                              const GURL& old_url,
-                                              const GURL& new_url,
-                                              bool is_top_level) {
-  BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
-  if (plugin)
-    plugin->LoadRedirect(old_url, new_url, is_top_level);
-}
-
-void BrowserPluginManagerImpl::OnSetCursor(int instance_id,
-                                           const WebCursor& cursor) {
-  BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
-  if (plugin)
-    plugin->SetCursor(cursor);
-}
-
-void BrowserPluginManagerImpl::OnGuestUnresponsive(int instance_id,
-                                                   int process_id) {
-  BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
-  if (plugin)
-    plugin->GuestUnresponsive(process_id);
-}
-
-void BrowserPluginManagerImpl::OnGuestResponsive(int instance_id,
-                                                 int process_id) {
-  BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
-  if (plugin)
-    plugin->GuestResponsive(process_id);
-}
 }  // namespace content