Browser Plugin: Allocate Instance IDs in BrowserPluginEmbedder instead of BrowserPluginManager

content/renderer/browser_plugin/browser_plugin_manager_impl.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 "content/renderer/browser_plugin/browser_plugin_manager_impl.h"
 
 #include "content/common/browser_plugin_messages.h"
 #include "content/renderer/browser_plugin/browser_plugin.h"
 #include "content/renderer/render_thread_impl.h"
 #include "ui/gfx/point.h"
 #include "webkit/glue/webcursor.h"
 
 namespace content {
 
 BrowserPluginManagerImpl::BrowserPluginManagerImpl(
     RenderViewImpl* render_view)
-    : BrowserPluginManager(render_view) {
+    : BrowserPluginManager(render_view),
+      request_counter_(0) {

[lazyboy, 2013/01/17 17:39:25]

nit: request_id_counter_ so it's obvious that this is related to request_id-s

[Fady Samuel, 2013/01/17 18:40:38]

Done.

 }
 
 BrowserPluginManagerImpl::~BrowserPluginManagerImpl() {
 }
 
 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_counter_++;
+  requests_.AddWithID(browser_plugin, request_id);

[lazyboy, 2013/01/17 17:39:25]

requests_ probably too broad name, maybe name it to be more specific? e.g.
pending_allocate_instance_id_requests_

[Fady Samuel, 2013/01/17 18:40:38]

Done.

+  Send(new BrowserPluginHostMsg_AllocateInstanceIDRequest(
+      browser_plugin->render_view_routing_id(), request_id));
 }
 
 bool BrowserPluginManagerImpl::Send(IPC::Message* msg) {
   return RenderThread::Get()->Send(msg);
 }
 
 bool BrowserPluginManagerImpl::OnMessageReceived(
     const IPC::Message& message) {
   if (ShouldForwardToBrowserPlugin(message)) {
     int instance_id = 0;
     // All allowed messages must have instance_id as their first parameter.
     PickleIterator iter(message);
     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_GENERIC(BrowserPluginMsg_AllocateInstanceIDResponse,
+                                OnAllocateInstanceIDResponse(message))
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_PluginAtPositionRequest,
                         OnPluginAtPositionRequest);
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
+void BrowserPluginManagerImpl::OnAllocateInstanceIDResponse(
+    const IPC::Message& message) {
+  int request_id = 0;
+  PickleIterator iter(message);
+  bool success = iter.ReadInt(&request_id);
+  DCHECK(success);
+  BrowserPlugin* plugin = requests_.Lookup(request_id);
+  requests_.Remove(request_id);
+  if (plugin)
+    plugin->OnMessageReceived(message);

[lazyboy, 2013/01/17 17:39:25]

Can we just read both request_id and instance_id (by making the handler
IPC_MESSAGE_HANDLER instead of *HANDLER_GENERIC) from message and call helper
function directly?:
plugin->AllocateInstanceId(request_id, instance_id);
Otherwise seems a bit confusing to handle this message in both places where
BrowserPlugin never directly receives this msg as IPC.

[Fady Samuel, 2013/01/17 18:40:38]

I'd prefer to leave it this way because eventually, when I introduce
BrowserPluginObservers: https://codereview.chromium.org/11826005/

I'll like Observers to get an opportunity to handle as many message types as
possible, in order to allow as much opportunity for modularity as possible.

+}
+
 void BrowserPluginManagerImpl::OnPluginAtPositionRequest(
     const IPC::Message& message,
     int request_id,
     const gfx::Point& position) {
   int instance_id = -1;
   IDMap<BrowserPlugin>::iterator it(&instances_);
   gfx::Point local_position = position;
   int source_routing_id = message.routing_id();
   while (!it.IsAtEnd()) {
     const BrowserPlugin* plugin = it.GetCurrentValue();
@@ skipping 35 matching lines @@
     case BrowserPluginMsg_UpdateRect::ID:
     case BrowserPluginMsg_BuffersSwapped::ID:
       return true;
     default:
       break;
   }
   return false;
 }
 
 }  // namespace content