Browser Plugin: Allocate Instance IDs in BrowserPluginEmbedder instead of BrowserPluginManager

content/renderer/browser_plugin/browser_plugin.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.h"
 
 #include "base/json/json_string_value_serializer.h"
 #include "base/message_loop.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
@@ skipping 30 matching lines @@
 using WebKit::WebPluginParams;
 using WebKit::WebPoint;
 using WebKit::WebRect;
 using WebKit::WebURL;
 using WebKit::WebVector;
 
 namespace content {
 
 namespace {
 
+const int INSTANCE_ID_NONE = 0;
+
 // Events.
 const char kEventExit[] = "exit";
 const char kEventLoadAbort[] = "loadabort";
 const char kEventLoadCommit[] = "loadcommit";
 const char kEventLoadRedirect[] = "loadredirect";
 const char kEventLoadStart[] = "loadstart";
 const char kEventLoadStop[] = "loadstop";
 const char kEventResponsive[] = "responsive";
 const char kEventSizeChanged[] = "sizechanged";
 const char kEventUnresponsive[] = "unresponsive";
@@ skipping 37 matching lines @@
   NOTREACHED() << "Unknown Termination Status.";
   return "unknown";
 }
 
 static std::string GetInternalEventName(const char* event_name) {
   return base::StringPrintf("-internal-%s", 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),
       damage_buffer_sequence_id_(0),
       resize_ack_received_(true),
       sad_guest_(NULL),
       guest_crashed_(false),
       navigate_src_sent_(false),
       auto_size_(false),
       max_height_(0),
       max_width_(0),
       min_height_(0),
       min_width_(0),
       process_id_(-1),
       persist_storage_(false),
       valid_partition_id_(true),
       content_window_routing_id_(MSG_ROUTING_NONE),
       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(
           render_view_routing_id_,
           instance_id_));
 }
 
 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)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_GuestGone, OnGuestGone)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_GuestResponsive, OnGuestResponsive)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_GuestUnresponsive, OnGuestUnresponsive)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_LoadAbort, OnLoadAbort)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_LoadCommit, OnLoadCommit)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_LoadRedirect, OnLoadRedirect)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_LoadStart, OnLoadStart)
@@ skipping 47 matching lines @@
 bool BrowserPlugin::SetSrcAttribute(const std::string& src,
                                     std::string* error_message) {
   if (!valid_partition_id_) {
     *error_message = kErrorInvalidPartition;
     return false;
   }
 
   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(
       new BrowserPluginHostMsg_NavigateGuest(
           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;
 }
 
 void BrowserPlugin::SetAutoSizeAttribute(bool auto_size) {
   if (auto_size_ == auto_size)
     return;
   auto_size_ = auto_size;
   last_view_size_ = plugin_rect_.size();
   UpdateGuestAutoSizeState();
 }
@@ skipping 46 matching lines @@
   return params.damage_buffer_sequence_id != 0;
 }
 
 bool BrowserPlugin::UsesPendingDamageBuffer(
     const BrowserPluginMsg_UpdateRect_Params& params) {
   if (!pending_damage_buffer_.get())
     return false;
   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);
 }
 
 void BrowserPlugin::OnBuffersSwapped(int instance_id,
                                      const gfx::Size& size,
                                      std::string mailbox_name,
                                      int gpu_route_id,
                                      int gpu_host_id) {
@@ skipping 759 matching lines @@
                                                  void* notify_data) {
 }
 
 void BrowserPlugin::didFailLoadingFrameRequest(
     const WebKit::WebURL& url,
     void* notify_data,
     const WebKit::WebURLError& error) {
 }
 
 }  // namespace content