Browser Plugin: Simplified guest attachment

BUG=330264

Review URL: https://codereview.chromium.org/250063002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@266640 0039d316-1c4b-4281-b951-d872f2087c98

chrome/renderer/resources/extensions/web_view.js

@@ -165,6 +165,7 @@ WebViewInternal.maybeRegisterExperimentalAPIs = function(proto) {}
 function WebViewInternal(webviewNode) {
   privates(webviewNode).internal = this;
   this.webviewNode = webviewNode;
+  this.attached = false;
   this.browserPluginNode = this.createBrowserPluginNode();
   var shadowRoot = this.webviewNode.createShadowRoot();
   shadowRoot.appendChild(this.browserPluginNode);
@@ -569,20 +570,7 @@ WebViewInternal.prototype.setupWebviewNodeEvents = function() {
   this.viewInstanceId = IdGenerator.GetNextId();
   var onInstanceIdAllocated = function(e) {
     var detail = e.detail ? JSON.parse(e.detail) : {};
-    self.instanceId = detail.windowId;
-    var params = {
-      'api': 'webview',
-      'instanceId': self.viewInstanceId
-    };
-    if (self.userAgentOverride) {
-      params['userAgentOverride'] = self.userAgentOverride;
-    }
-    self.browserPluginNode['-internal-attach'](params);
-
-    var events = self.getEvents();
-    for (var eventName in events) {
-      self.setupEvent(eventName, events[eventName]);
-    }
+    self.attachWindowAndSetUpEvents(detail.windowId);
   };
   this.browserPluginNode.addEventListener('-internal-instanceid-allocated',
                                           onInstanceIdAllocated);
@@ -801,7 +789,7 @@ WebViewInternal.prototype.handleNewWindowEvent =
   var windowObj = {
     attach: function(webview) {
       validateCall();
-      if (!webview)
+      if (!webview || !webview.tagName || webview.tagName != 'WEBVIEW')
         throw new Error(ERROR_MSG_WEBVIEW_EXPECTED);
       // Attach happens asynchronously to give the tagWatcher an opportunity
       // to pick up the new webview before attach operates on it, if it hasn't
@@ -809,9 +797,10 @@ WebViewInternal.prototype.handleNewWindowEvent =
       // Note: Any subsequent errors cannot be exceptions because they happen
       // asynchronously.
       setTimeout(function() {
+        var webViewInternal = privates(webview).internal;
         var attached =
-            browserPluginNode['-internal-attachWindowTo'](webview,
-                                                          event.windowId);
+            webViewInternal.attachWindowAndSetUpEvents(event.windowId);
+
         if (!attached) {
           window.console.error(ERROR_MSG_NEWWINDOW_UNABLE_TO_ATTACH);
         }
@@ -1022,6 +1011,25 @@ WebViewInternal.prototype.setUserAgentOverride = function(userAgentOverride) {
   WebView.overrideUserAgent(this.instanceId, userAgentOverride);
 };
 
+/** @private */
+WebViewInternal.prototype.attachWindowAndSetUpEvents = function(instanceId) {
+  this.instanceId = instanceId;
+  var params = {
+    'api': 'webview',
+    'instanceId': this.viewInstanceId
+  };
+  if (this.userAgentOverride) {
+    params['userAgentOverride'] = this.userAgentOverride;
+  }
+  this.browserPluginNode['-internal-attach'](this.instanceId, params);
+
+  var events = this.getEvents();
+  for (var eventName in events) {
+    this.setupEvent(eventName, events[eventName]);
+  }
+  return true;
+};
+
 // Registers browser plugin <object> custom element.
 function registerBrowserPluginElement() {
   var proto = Object.create(HTMLObjectElement.prototype);

content/renderer/browser_plugin/browser_plugin.cc

@@ -61,11 +61,6 @@ static std::string GetInternalEventName(const char* event_name) {
   return base::StringPrintf("-internal-%s", event_name);
 }
 
-typedef std::map<blink::WebPluginContainer*,
-                 BrowserPlugin*> PluginContainerMap;
-static base::LazyInstance<PluginContainerMap> g_plugin_container_map =
-    LAZY_INSTANCE_INITIALIZER;
-
 }  // namespace
 
 BrowserPlugin::BrowserPlugin(RenderViewImpl* render_view,
@@ -108,14 +103,6 @@ BrowserPlugin::~BrowserPlugin() {
                                                guest_instance_id_));
 }
 
-/*static*/
-BrowserPlugin* BrowserPlugin::FromContainer(
-    blink::WebPluginContainer* container) {
-  PluginContainerMap* browser_plugins = g_plugin_container_map.Pointer();
-  PluginContainerMap::iterator it = browser_plugins->find(container);
-  return it == browser_plugins->end() ? NULL : it->second;
-}
-
 bool BrowserPlugin::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(BrowserPlugin, message)
@@ -380,13 +367,10 @@ bool BrowserPlugin::UsesPendingDamageBuffer(
 
 void BrowserPlugin::OnInstanceIDAllocated(int guest_instance_id) {
   CHECK(guest_instance_id != browser_plugin::kInstanceIDNone);
-  before_first_navigation_ = false;
-  guest_instance_id_ = guest_instance_id;
-  browser_plugin_manager()->AddBrowserPlugin(guest_instance_id, this);
 
   if (auto_navigate_) {
     scoped_ptr<base::DictionaryValue> params(new base::DictionaryValue());
-    Attach(params.Pass());
+    Attach(guest_instance_id, params.Pass());
     return;
   }
 
@@ -396,7 +380,20 @@ void BrowserPlugin::OnInstanceIDAllocated(int guest_instance_id) {
   TriggerEvent(browser_plugin::kEventInternalInstanceIDAllocated, &props);
 }
 
-void BrowserPlugin::Attach(scoped_ptr<base::DictionaryValue> extra_params) {
+void BrowserPlugin::Attach(int guest_instance_id,
+                           scoped_ptr<base::DictionaryValue> extra_params) {
+  CHECK(guest_instance_id != browser_plugin::kInstanceIDNone);
+
+  // If this BrowserPlugin is already attached to a guest, then do nothing.
+  if (HasGuestInstanceID())
+    return;
+
+  // This API may be called directly without setting the src attribute.
+  // In that case, we need to make sure we don't allocate another instance ID.
+  before_first_navigation_ = false;
+  guest_instance_id_ = guest_instance_id;
+  browser_plugin_manager()->AddBrowserPlugin(guest_instance_id, this);
+
   BrowserPluginHostMsg_Attach_Params attach_params;
   attach_params.focused = ShouldGuestBeFocused();
   attach_params.visible = visible_;
@@ -693,57 +690,6 @@ NPObject* BrowserPlugin::GetContentWindow() const {
   return guest_frame->windowObject();
 }
 
-// static
-bool BrowserPlugin::AttachWindowTo(const blink::WebNode& node, int window_id) {
-  if (node.isNull())
-    return false;
-
-  if (!node.isElementNode())
-    return false;
-
-  blink::WebElement shim_element = node.toConst<blink::WebElement>();
-  // The shim containing the BrowserPlugin must be attached to a document.
-  if (shim_element.document().isNull())
-    return false;
-
-  blink::WebNode shadow_root = shim_element.shadowRoot();
-  if (shadow_root.isNull() || !shadow_root.hasChildNodes())
-    return false;
-
-  blink::WebNode plugin_element = shadow_root.firstChild();
-  blink::WebPluginContainer* plugin_container =
-      plugin_element.pluginContainer();
-  if (!plugin_container)
-    return false;
-
-  BrowserPlugin* browser_plugin =
-      BrowserPlugin::FromContainer(plugin_container);
-  if (!browser_plugin)
-    return false;
-
-  // If the BrowserPlugin has already begun to navigate then we shouldn't allow
-  // attaching a different guest.
-  //
-  // Navigation happens in two stages.
-  // 1. BrowserPlugin requests an instance ID from the browser process.
-  // 2. The browser process returns an instance ID and BrowserPlugin is
-  //    "Attach"ed to that instance ID.
-  // If the instance ID is new then a new guest will be created.
-  // If the instance ID corresponds to an unattached guest then BrowserPlugin
-  // is attached to that guest.
-  //
-  // Between step 1, and step 2, BrowserPlugin::AttachWindowTo may be called.
-  // The check below ensures that BrowserPlugin:Attach does not get called with
-  // a different instance ID after step 1 has happened.
-  // TODO(fsamuel): We may wish to support reattaching guests in the future:
-  // http://crbug.com/156219.
-  if (browser_plugin->HasNavigated())
-    return false;
-
-  browser_plugin->OnInstanceIDAllocated(window_id);
-  return true;
-}
-
 bool BrowserPlugin::HasNavigated() const {
   return !before_first_navigation_;
 }
@@ -914,7 +860,6 @@ bool BrowserPlugin::initialize(WebPluginContainer* container) {
   container_ = container;
   container_->setWantsWheelEvents(true);
   ParseAttributes();
-  g_plugin_container_map.Get().insert(std::make_pair(container_, this));
   return true;
 }
 
@@ -961,9 +906,6 @@ void BrowserPlugin::destroy() {
   if (container_)
     container_->clearScriptObjects();
 
-  // The BrowserPlugin's WebPluginContainer is deleted immediately after this
-  // call returns, so let's not keep a reference to it around.
-  g_plugin_container_map.Get().erase(container_);
   if (compositing_helper_.get())
     compositing_helper_->OnContainerDestroy();
   container_ = NULL;

content/renderer/browser_plugin/browser_plugin.h

@@ -46,8 +46,6 @@ class CONTENT_EXPORT BrowserPlugin :
     return browser_plugin_manager_.get();
   }
 
-  static BrowserPlugin* FromContainer(blink::WebPluginContainer* container);
-
   bool OnMessageReceived(const IPC::Message& msg);
 
   // Update Browser Plugin's DOM Node attribute |attribute_name| with the value
@@ -109,11 +107,6 @@ class CONTENT_EXPORT BrowserPlugin :
   // Returns whether this BrowserPlugin has allocated an instance ID.
   bool HasGuestInstanceID() const;
 
-  // Attaches the window identified by |window_id| to the the given node
-  // encapsulating a BrowserPlugin.
-  static bool AttachWindowTo(const blink::WebNode& node,
-                             int window_id);
-
   // Informs the guest of an updated focus state.
   void UpdateGuestFocusState();
   // Indicates whether the guest should be focused.
@@ -131,7 +124,8 @@ class CONTENT_EXPORT BrowserPlugin :
   // Provided that a guest instance ID has been allocated, this method attaches
   // this BrowserPlugin instance to that guest. |extra_params| are parameters
   // passed in by the content embedder to the browser process.
-  void Attach(scoped_ptr<base::DictionaryValue> extra_params);
+  void Attach(int guest_instance_id,
+              scoped_ptr<base::DictionaryValue> extra_params);
 
   // Notify the plugin about a compositor commit so that frame ACKs could be
   // sent, if needed.

content/renderer/browser_plugin/browser_plugin_bindings.cc

@@ -231,9 +231,7 @@ class BrowserPluginMethodBinding {
 class BrowserPluginBindingAttach: public BrowserPluginMethodBinding {
  public:
   BrowserPluginBindingAttach()
-      : BrowserPluginMethodBinding(
-          browser_plugin::kMethodInternalAttach, 1) {
-  }
+      : BrowserPluginMethodBinding(browser_plugin::kMethodInternalAttach, 2) {}
 
   virtual bool Invoke(BrowserPluginBindings* bindings,
                       const NPVariant* args,
@@ -241,8 +239,12 @@ class BrowserPluginBindingAttach: public BrowserPluginMethodBinding {
     if (!bindings->instance()->render_view())
       return false;
 
+    int instance_id = IntFromNPVariant(args[0]);
+    if (!instance_id)
+      return false;
+
     scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create());
-    v8::Handle<v8::Value> obj(blink::WebBindings::toV8Value(&args[0]));
+    v8::Handle<v8::Value> obj(blink::WebBindings::toV8Value(&args[1]));
     scoped_ptr<base::Value> value(
         converter->FromV8Value(obj, bindings->instance()->render_view()->
             GetWebView()->mainFrame()->mainWorldScriptContext()));
@@ -254,7 +256,7 @@ class BrowserPluginBindingAttach: public BrowserPluginMethodBinding {
 
     scoped_ptr<base::DictionaryValue> extra_params(
         static_cast<base::DictionaryValue*>(value.release()));
-    bindings->instance()->Attach(extra_params.Pass());
+    bindings->instance()->Attach(instance_id, extra_params.Pass());
     return true;
   }
 
@@ -262,28 +264,6 @@ class BrowserPluginBindingAttach: public BrowserPluginMethodBinding {
   DISALLOW_COPY_AND_ASSIGN(BrowserPluginBindingAttach);
 };
 
-class BrowserPluginBindingAttachWindowTo : public BrowserPluginMethodBinding {
- public:
-  BrowserPluginBindingAttachWindowTo()
-      : BrowserPluginMethodBinding(
-          browser_plugin::kMethodInternalAttachWindowTo, 2) {
-  }
-
-  virtual bool Invoke(BrowserPluginBindings* bindings,
-                      const NPVariant* args,
-                      NPVariant* result) OVERRIDE {
-    blink::WebNode node;
-    WebBindings::getNode(NPVARIANT_TO_OBJECT(args[0]), &node);
-    int window_id = IntFromNPVariant(args[1]);
-    BOOLEAN_TO_NPVARIANT(BrowserPlugin::AttachWindowTo(node, window_id),
-                         *result);
-    return true;
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(BrowserPluginBindingAttachWindowTo);
-};
-
 // BrowserPluginPropertyBinding ------------------------------------------------
 
 class BrowserPluginPropertyBinding {
@@ -671,7 +651,6 @@ BrowserPluginBindings::BrowserPluginBindings(BrowserPlugin* instance)
   np_object_->message_channel = weak_ptr_factory_.GetWeakPtr();
 
   method_bindings_.push_back(new BrowserPluginBindingAttach);
-  method_bindings_.push_back(new BrowserPluginBindingAttachWindowTo);
 
   property_bindings_.push_back(
       new BrowserPluginPropertyBindingAllowTransparency);

content/renderer/browser_plugin/mock_browser_plugin_manager.cc

@@ -41,9 +41,8 @@ void MockBrowserPluginManager::AllocateInstanceID(
 void MockBrowserPluginManager::AllocateInstanceIDACK(
     BrowserPlugin* browser_plugin,
     int guest_instance_id) {
-  browser_plugin->OnInstanceIDAllocated(guest_instance_id);
   scoped_ptr<base::DictionaryValue> extra_params(new base::DictionaryValue());
-  browser_plugin->Attach(extra_params.Pass());
+  browser_plugin->Attach(guest_instance_id, extra_params.Pass());
 }
 
 bool MockBrowserPluginManager::Send(IPC::Message* msg) {

content/test/data/browser_plugin_dragging.html

@@ -32,7 +32,8 @@ function SetSrc(src) {
 
 var plugin = document.getElementById('plugin');
 plugin.addEventListener('-internal-instanceid-allocated', function(e) {
-  plugin['-internal-attach']({});
+  var detail = e.detail ? JSON.parse(e.detail) : {};
+  plugin['-internal-attach'](detail.windowId, {});
 });
 </script>
 

content/test/data/browser_plugin_embedder.html

@@ -53,6 +53,7 @@ function receiveMessage(event) {
 var plugin = document.getElementById('plugin');
 window.addEventListener('message', receiveMessage, false);
 plugin.addEventListener('-internal-instanceid-allocated', function(e) {
-  plugin['-internal-attach']({});
+  var detail = e.detail ? JSON.parse(e.detail) : {};
+  plugin['-internal-attach'](detail.windowId, {});
 });
 </script>

content/test/data/browser_plugin_focus.html

@@ -20,7 +20,8 @@
 <script type="text/javascript">
 var plugin = document.getElementById('plugin');
 plugin.addEventListener('-internal-instanceid-allocated', function(e) {
-  plugin['-internal-attach']({});
+  var detail = e.detail ? JSON.parse(e.detail) : {};
+  plugin['-internal-attach'](detail.windowId, {});
 });
 </script>
 </body>