Browser Plugin: Allocate Instance IDs in BrowserPluginEmbedder instead of BrowserPluginManager

content/renderer/browser_plugin/browser_plugin.cc

Index: content/renderer/browser_plugin/browser_plugin.cc
diff --git a/content/renderer/browser_plugin/browser_plugin.cc b/content/renderer/browser_plugin/browser_plugin.cc
index 0e95003d07cc5c1f1954a070275bd698fecff34c..acbbb669c0ba47e754d295abb7e0c1fce67bcb2c 100644
--- a/content/renderer/browser_plugin/browser_plugin.cc
+++ b/content/renderer/browser_plugin/browser_plugin.cc
@@ -48,6 +48,8 @@ namespace content {
 
 namespace {
 
+const int INSTANCE_ID_NONE = 0;
+
 // Events.
 const char kEventExit[] = "exit";
 const char kEventLoadAbort[] = "loadabort";
@@ -105,11 +107,10 @@ static std::string GetInternalEventName(const char* event_name) {
 }
 
 BrowserPlugin::BrowserPlugin(
-    int instance_id,
     RenderViewImpl* render_view,
     WebKit::WebFrame* frame,
     const WebPluginParams& params)
-    : instance_id_(instance_id),
+    : instance_id_(INSTANCE_ID_NONE),
       render_view_(render_view->AsWeakPtr()),
       render_view_routing_id_(render_view->GetRoutingID()),
       container_(NULL),
@@ -130,17 +131,22 @@ BrowserPlugin::BrowserPlugin(
       plugin_focused_(false),
       visible_(true),
       size_changed_in_flight_(false),
+      allocate_instance_id_in_flight_(false),
       browser_plugin_manager_(render_view->browser_plugin_manager()),
       current_nav_entry_index_(0),
       nav_entry_count_(0),
       compositing_enabled_(false) {
-  browser_plugin_manager()->AddBrowserPlugin(instance_id, this);
   bindings_.reset(new BrowserPluginBindings(this));
 
   ParseAttributes(params);
 }
 
 BrowserPlugin::~BrowserPlugin() {
+  // If the BrowserPlugin has never navigated then the browser process, and

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

Do we have a test that destroys BrowserPlugin before navigating it?

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

Done. Added a test.

+  // BrowserPluginManager don't know about it and so there is nothing to do
+  // here.
+  if (!navigate_src_sent_)
+    return;
   browser_plugin_manager()->RemoveBrowserPlugin(instance_id_);
   browser_plugin_manager()->Send(
       new BrowserPluginHostMsg_PluginDestroyed(
@@ -152,6 +158,8 @@ bool BrowserPlugin::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(BrowserPlugin, message)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_AdvanceFocus, OnAdvanceFocus)
+    IPC_MESSAGE_HANDLER(BrowserPluginMsg_AllocateInstanceIDResponse,
+                        OnAllocateInstanceIDResponse)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_BuffersSwapped, OnBuffersSwapped)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_GuestContentWindowReady,
                         OnGuestContentWindowReady)
@@ -219,23 +227,17 @@ bool BrowserPlugin::SetSrcAttribute(const std::string& src,
   if (src.empty() || (src == src_ && !guest_crashed_))
     return true;
 
+  src_ = src;
+
   // If we haven't created the guest yet, do so now. We will navigate it right
   // after creation. If |src| is empty, we can delay the creation until we
   // actually need it.
   if (!navigate_src_sent_) {
-    BrowserPluginHostMsg_CreateGuest_Params create_guest_params;
-    create_guest_params.storage_partition_id = storage_partition_id_;
-    create_guest_params.persist_storage = persist_storage_;
-    create_guest_params.focused = ShouldGuestBeFocused();
-    create_guest_params.visible = visible_;
-    create_guest_params.name = name_;
-    GetDamageBufferWithSizeParams(&create_guest_params.auto_size_params,
-                                  &create_guest_params.resize_guest_params);
-    browser_plugin_manager()->Send(
-        new BrowserPluginHostMsg_CreateGuest(
-            render_view_routing_id_,
-            instance_id_,
-            create_guest_params));
+    if (!allocate_instance_id_in_flight_) {

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

I'd put a short version of your CL description as code comment here, describing
how the flow is supposed to work.

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

Done.

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

Done.

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

Done.

+      browser_plugin_manager()->AllocateInstanceID(this);
+      allocate_instance_id_in_flight_ = true;

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

more state booleans. possible cleanup opportunities later:
from looking at navigate_src_sent_ and allocate_instance_id_in_flight_, seems
the browser plugin can be in the following 3 states regarding these booleans
(not 4),
1. not_navigated_and_not_allocated
2. allocating (in flight..)
3. navigated
and once it is in a step x, it cannot go back to state < x.

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

Agreed, there's room for cleanup here. mthiesse@ is doing a cleanup that might
improve things a bit here. Really, instance_id_ == INSTANCE_ID_NONE and
navigate_src_sent_ more or less mean the same thing now but that's possibly less
obvious.

+    }
+    return true;
   }
 
   browser_plugin_manager()->Send(
@@ -243,11 +245,6 @@ bool BrowserPlugin::SetSrcAttribute(const std::string& src,
           render_view_routing_id_,
           instance_id_,
           src));
-  // Record that we sent a NavigateGuest message to embedder.

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

Keep this comment in OnAllocateInstanceIDResponse()?

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

Done.

-  // Once this instance has navigated, the storage partition cannot be changed,
-  // so this value is used for enforcing this.
-  navigate_src_sent_ = true;
-  src_ = src;
   return true;
 }
 
@@ -314,6 +311,29 @@ bool BrowserPlugin::UsesPendingDamageBuffer(
   return damage_buffer_sequence_id_ == params.damage_buffer_sequence_id;
 }
 
+void BrowserPlugin::OnAllocateInstanceIDResponse(int request_id,
+                                                 int instance_id) {
+  instance_id_ = instance_id;
+  browser_plugin_manager()->AddBrowserPlugin(instance_id, this);
+
+  BrowserPluginHostMsg_CreateGuest_Params create_guest_params;
+  create_guest_params.storage_partition_id = storage_partition_id_;
+  create_guest_params.persist_storage = persist_storage_;
+  create_guest_params.focused = ShouldGuestBeFocused();
+  create_guest_params.visible = visible_;
+  create_guest_params.name = name_;
+  create_guest_params.src = src_;
+  GetDamageBufferWithSizeParams(&create_guest_params.auto_size_params,
+                                &create_guest_params.resize_guest_params);
+  browser_plugin_manager()->Send(
+      new BrowserPluginHostMsg_CreateGuest(
+          render_view_routing_id_,
+          instance_id_,
+          create_guest_params));
+
+  navigate_src_sent_ = true;
+}
+
 void BrowserPlugin::OnAdvanceFocus(int instance_id, bool reverse) {
   DCHECK(render_view_);
   render_view_->GetWebView()->advanceFocus(reverse);