Browser Plugin: Allocate Instance IDs in BrowserPluginEmbedder instead of 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 7eb0b713cddad1428d3132d8b55219293efedfb9..8c4938bd82c8894210023aeaeb3fae7f21642408 100644
--- a/content/renderer/browser_plugin/browser_plugin_manager_impl.cc
+++ b/content/renderer/browser_plugin/browser_plugin_manager_impl.cc
@@ -14,7 +14,8 @@ namespace content {
 
 BrowserPluginManagerImpl::BrowserPluginManagerImpl(
     RenderViewImpl* render_view)
-    : BrowserPluginManager(render_view) {
+    : BrowserPluginManager(render_view),
+      request_id_counter_(0) {
 }
 
 BrowserPluginManagerImpl::~BrowserPluginManagerImpl() {
@@ -24,10 +25,15 @@ BrowserPlugin* BrowserPluginManagerImpl::CreateBrowserPlugin(
     RenderViewImpl* render_view,
     WebKit::WebFrame* frame,
     const WebKit::WebPluginParams& params) {
-  return new BrowserPlugin(++browser_plugin_counter_,
-                           render_view,
-                           frame,
-                           params);
+  return new BrowserPlugin(render_view, frame, params);
+}
+
+void BrowserPluginManagerImpl::AllocateInstanceID(
+    BrowserPlugin* browser_plugin) {
+  int request_id = request_id_counter_++;
+  pending_allocate_instance_id_requests_.AddWithID(browser_plugin, request_id);
+  Send(new BrowserPluginHostMsg_AllocateInstanceIDRequest(
+      browser_plugin->render_view_routing_id(), request_id));
 }
 
 bool BrowserPluginManagerImpl::Send(IPC::Message* msg) {
@@ -49,6 +55,8 @@ bool BrowserPluginManagerImpl::OnMessageReceived(
 
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(BrowserPluginManagerImpl, message)
+    IPC_MESSAGE_HANDLER_GENERIC(BrowserPluginMsg_AllocateInstanceIDResponse,
+                                OnAllocateInstanceIDResponse(message))
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_PluginAtPositionRequest,
                         OnPluginAtPositionRequest);
     IPC_MESSAGE_UNHANDLED(handled = false)
@@ -56,6 +64,19 @@ bool BrowserPluginManagerImpl::OnMessageReceived(
   return handled;
 }
 
+void BrowserPluginManagerImpl::OnAllocateInstanceIDResponse(
+    const IPC::Message& message) {
+  int request_id = 0;
+  PickleIterator iter(message);

[Charlie Reis, 2013/01/18 01:54:31]

This is usually a red flag.  Seems like we should be able to get by with the
normal message handling, without having to parse it ourselves.

Why does BrowserPlugin need a message handler for this?  I can't think of a case
it would be receiving the message directly.  Couldn't it just have a normal
method that takes in the instance_id and request_id from this handler?

[Fady Samuel, 2013/01/18 02:23:10]

Fixed. Using the same technique Istiaque used below.

[Charlie Reis, 2013/01/18 05:36:45]

Sure, that's better.  But why is it a message handler in BrowserPlugin? 
BrowserPlugin doesn't even need the request ID.  If the message always goes
through here, shouldn't we just have BrowserPlugin::SetInstanceID(instance_id)
or BrowserPlugin::Init(instance_id) instead?

+  bool success = iter.ReadInt(&request_id);
+  DCHECK(success);
+  BrowserPlugin* plugin =
+      pending_allocate_instance_id_requests_.Lookup(request_id);
+  pending_allocate_instance_id_requests_.Remove(request_id);
+  if (plugin)
+    plugin->OnMessageReceived(message);
+}
+
 void BrowserPluginManagerImpl::OnPluginAtPositionRequest(
     const IPC::Message& message,
     int request_id,