Browser Plugin: Refactor BrowserPluginEmbedder to allow creating guests with openers

content/browser/browser_plugin/browser_plugin_embedder.cc

Index: content/browser/browser_plugin/browser_plugin_embedder.cc
diff --git a/content/browser/browser_plugin/browser_plugin_embedder.cc b/content/browser/browser_plugin/browser_plugin_embedder.cc
index 3d60f39569aa1859287a617f6bc8d60085a50f77..68273df07d3b84a52bf12f11a3fc72b5257620e6 100644
--- a/content/browser/browser_plugin/browser_plugin_embedder.cc
+++ b/content/browser/browser_plugin/browser_plugin_embedder.cc
@@ -54,6 +54,127 @@ BrowserPluginEmbedder* BrowserPluginEmbedder::Create(
   return new BrowserPluginEmbedder(web_contents, render_view_host);
 }
 
+void BrowserPluginEmbedder::CreateGuest(
+    int instance_id,
+    int routing_id,
+    BrowserPluginGuest* guest_opener,
+    const BrowserPluginHostMsg_CreateGuest_Params& params) {
+  WebContentsImpl* guest_web_contents = NULL;
+  BrowserPluginGuest* guest = GetGuestByInstanceID(instance_id);
+  CHECK(!guest);
+
+  // Validate that the partition id coming from the renderer is valid UTF-8,
+  // since we depend on this in other parts of the code, such as FilePath
+  // creation. If the validation fails, treat it as a bad message and kill the
+  // renderer process.
+  if (!IsStringUTF8(params.storage_partition_id)) {
+    content::RecordAction(UserMetricsAction("BadMessageTerminate_BPE"));
+    base::KillProcess(render_view_host_->GetProcess()->GetHandle(),
+                      content::RESULT_CODE_KILLED_BAD_MESSAGE, false);
+    return;
+  }
+
+  const std::string& host =
+      render_view_host_->GetSiteInstance()->GetSiteURL().host();
+  std::string url_encoded_partition = net::EscapeQueryParamValue(
+      params.storage_partition_id, false);
+
+  SiteInstance* guest_site_instance = NULL;
+  if (guest_opener) {
+    guest_site_instance = guest_opener->GetWebContents()->GetSiteInstance();

[Charlie Reis, 2013/01/04 18:53:19]

Yep, this looks right.  This will ensure the new guest and its opener share a
process, which is what we want.

[Fady Samuel, 2013/01/08 17:00:49]

OK

+  } else {
+    // The SiteInstance of a given webview tag is based on the fact that it's a
+    // guest process in addition to which platform application the tag belongs
+    // to and what storage partition is in use, rather than the URL that the tag
+    // is being navigated to.
+    GURL guest_site(
+        base::StringPrintf("%s://%s/%s?%s", chrome::kGuestScheme,
+                          host.c_str(), params.persist_storage ? "persist" : "",
+                          url_encoded_partition.c_str()));
+
+    // If we already have a webview tag in the same app using the same storage
+    // partition, we should use the same SiteInstance so the existing tag and
+    // the new tag can script each other.
+    for (ContainerInstanceMap::const_iterator it =
+            guest_web_contents_by_instance_id_.begin();
+        it != guest_web_contents_by_instance_id_.end(); ++it) {
+      if (it->second->GetSiteInstance()->GetSiteURL() == guest_site) {
+        guest_site_instance = it->second->GetSiteInstance();
+        break;
+      }
+    }
+    if (!guest_site_instance) {
+      // Create the SiteInstance in a new BrowsingInstance, which will ensure
+      // that webview tags are also not allowed to send messages across
+      // different partitions.
+      guest_site_instance = SiteInstance::CreateForURL(
+          web_contents()->GetBrowserContext(), guest_site);
+    }
+  }
+  WebContentsImpl* base_web_contents = static_cast<WebContentsImpl*>(

[Charlie Reis, 2013/01/04 18:53:19]

What does base mean?  Might be clearer to call this opener_web_contents.

[Fady Samuel, 2013/01/08 17:00:49]

Done.

+      guest_opener ? guest_opener->GetWebContents() : NULL);
+  guest_web_contents = WebContentsImpl::CreateGuest(
+      web_contents()->GetBrowserContext(),

[sadrul, 2013/01/07 15:09:54]

Should you be using the browser-context of opener_web_contents (when not null)?
Does it matter? (I notice that this is part of the code moved from
OnCreateGuest, as opposed to new code)

[Fady Samuel, 2013/01/08 17:00:49]

This is the embedder's BrowserContext. All guests use the embedder's browser
context. Charlie, could you please comment on whether this needs to change? I
guess I just don't know the scope of what is "browser context".

[Charlie Reis, 2013/01/08 21:13:52]

The BrowserContext is the part of Profile that lives in the content module. 
You're right that the embedder and both old and new guests necessarily have the
same BrowserContext.  I don't think it needs to change.

+      guest_site_instance,
+      routing_id,
+      base_web_contents,
+      instance_id,
+      params);
+
+  guest = guest_web_contents->GetBrowserPluginGuest();
+  guest->set_embedder_web_contents(
+      static_cast<WebContentsImpl*>(web_contents()));
+
+  RendererPreferences* guest_renderer_prefs =
+      guest_web_contents->GetMutableRendererPrefs();
+  // Copy renderer preferences (and nothing else) from the embedder's
+  // TabContents to the guest.
+  //
+  // For GTK and Aura this is necessary to get proper renderer configuration
+  // values for caret blinking interval, colors related to selection and
+  // focus.
+  *guest_renderer_prefs = *web_contents()->GetMutableRendererPrefs();
+
+  guest_renderer_prefs->throttle_input_events = false;
+  // Navigation is disabled in Chrome Apps. We want to make sure guest-initiated
+  // navigations still continue to function inside the app.
+  guest_renderer_prefs->browser_handles_all_top_level_requests = false;
+  AddGuest(instance_id, guest_web_contents);
+  guest_web_contents->SetDelegate(guest);
+
+  // Create a swapped out RenderView for the guest in the embedder render
+  // process, so that the embedder can access the guest's window object.
+  int guest_routing_id =
+      static_cast<WebContentsImpl*>(guest->GetWebContents())->
+            CreateSwappedOutRenderView(web_contents()->GetSiteInstance());
+  render_view_host_->Send(new BrowserPluginMsg_GuestContentWindowReady(
+      render_view_host_->GetRoutingID(), instance_id, guest_routing_id));
+
+  // If this guest was created by another guest, then don't initialize it yet as
+  // it has not yet necessarily been attached to a BrowserPlugin.

[Charlie Reis, 2013/01/04 18:53:19]

The comment makes it sound like we should be checking guest_opener here.  Should
we?

[Fady Samuel, 2013/01/08 17:00:49]

This is a bad comment. Thanks for catching this! At some point I had an if
statement guarding Initialize in my code locally but then I realized it's not
necessary.

+  guest->Initialize(params, guest_web_contents->GetRenderViewHost());
+}
+
+BrowserPluginGuest* BrowserPluginEmbedder::GetGuestByInstanceID(
+    int instance_id) const {
+  ContainerInstanceMap::const_iterator it =
+      guest_web_contents_by_instance_id_.find(instance_id);
+  if (it != guest_web_contents_by_instance_id_.end())
+    return static_cast<WebContentsImpl*>(it->second)->GetBrowserPluginGuest();
+  return NULL;
+}
+
+void BrowserPluginEmbedder::DestroyGuestByInstanceID(int instance_id) {
+  BrowserPluginGuest* guest = GetGuestByInstanceID(instance_id);
+  if (guest) {
+    WebContents* guest_web_contents = guest->GetWebContents();
+
+    // Destroy the guest's web_contents.
+    delete guest_web_contents;
+    guest_web_contents_by_instance_id_.erase(instance_id);
+  }
+}
+
 void BrowserPluginEmbedder::GetRenderViewHostAtPosition(
     int x, int y, const WebContents::GetRenderViewHostCallback& callback) {
   // Store the callback so we can call it later when we have the response.
@@ -117,15 +238,6 @@ void BrowserPluginEmbedder::Observe(int type,
   }
 }
 
-BrowserPluginGuest* BrowserPluginEmbedder::GetGuestByInstanceID(
-    int instance_id) const {
-  ContainerInstanceMap::const_iterator it =
-      guest_web_contents_by_instance_id_.find(instance_id);
-  if (it != guest_web_contents_by_instance_id_.end())
-    return static_cast<WebContentsImpl*>(it->second)->GetBrowserPluginGuest();
-  return NULL;
-}
-
 void BrowserPluginEmbedder::AddGuest(int instance_id,
                                      WebContents* guest_web_contents) {
   DCHECK(guest_web_contents_by_instance_id_.find(instance_id) ==
@@ -133,17 +245,6 @@ void BrowserPluginEmbedder::AddGuest(int instance_id,
   guest_web_contents_by_instance_id_[instance_id] = guest_web_contents;
 }
 
-void BrowserPluginEmbedder::DestroyGuestByInstanceID(int instance_id) {
-  BrowserPluginGuest* guest = GetGuestByInstanceID(instance_id);
-  if (guest) {
-    WebContents* guest_web_contents = guest->GetWebContents();
-
-    // Destroy the guest's web_contents.
-    delete guest_web_contents;
-    guest_web_contents_by_instance_id_.erase(instance_id);
-  }
-}
-
 void BrowserPluginEmbedder::CleanUp() {
   // Destroy guests that are managed by the current embedder.
   STLDeleteContainerPairSecondPointers(
@@ -194,91 +295,7 @@ bool BrowserPluginEmbedder::ShouldForwardToBrowserPluginGuest(
 void BrowserPluginEmbedder::OnCreateGuest(
     int instance_id,
     const BrowserPluginHostMsg_CreateGuest_Params& params) {
-  WebContentsImpl* guest_web_contents = NULL;
-  BrowserPluginGuest* guest = GetGuestByInstanceID(instance_id);
-  CHECK(!guest);
-
-  // Validate that the partition id coming from the renderer is valid UTF-8,
-  // since we depend on this in other parts of the code, such as FilePath
-  // creation. If the validation fails, treat it as a bad message and kill the
-  // renderer process.
-  if (!IsStringUTF8(params.storage_partition_id)) {
-    content::RecordAction(UserMetricsAction("BadMessageTerminate_BPE"));
-    base::KillProcess(render_view_host_->GetProcess()->GetHandle(),
-                      content::RESULT_CODE_KILLED_BAD_MESSAGE, false);
-    return;
-  }
-
-  const std::string& host =
-      render_view_host_->GetSiteInstance()->GetSiteURL().host();
-  std::string url_encoded_partition = net::EscapeQueryParamValue(
-      params.storage_partition_id, false);
-
-  // The SiteInstance of a given webview tag is based on the fact that it's a
-  // guest process in addition to which platform application the tag belongs to
-  // and what storage partition is in use, rather than the URL that the tag is
-  // being navigated to.
-  GURL guest_site(
-      base::StringPrintf("%s://%s/%s?%s", chrome::kGuestScheme,
-                         host.c_str(), params.persist_storage ? "persist" : "",
-                         url_encoded_partition.c_str()));
-
-  // If we already have a webview tag in the same app using the same storage
-  // partition, we should use the same SiteInstance so the existing tag and
-  // the new tag can script each other.
-  SiteInstance* guest_site_instance = NULL;
-  for (ContainerInstanceMap::const_iterator it =
-           guest_web_contents_by_instance_id_.begin();
-       it != guest_web_contents_by_instance_id_.end(); ++it) {
-    if (it->second->GetSiteInstance()->GetSiteURL() == guest_site) {
-      guest_site_instance = it->second->GetSiteInstance();
-      break;
-    }
-  }
-  if (!guest_site_instance) {
-    // Create the SiteInstance in a new BrowsingInstance, which will ensure that
-    // webview tags are also not allowed to send messages across different
-    // partitions.
-    guest_site_instance = SiteInstance::CreateForURL(
-        web_contents()->GetBrowserContext(), guest_site);
-  }
-
-  guest_web_contents = WebContentsImpl::CreateGuest(
-      web_contents()->GetBrowserContext(),
-      guest_site_instance,
-      instance_id,
-      params);
-
-  guest = guest_web_contents->GetBrowserPluginGuest();
-  guest->set_embedder_web_contents(
-      static_cast<WebContentsImpl*>(web_contents()));
-
-  RendererPreferences* guest_renderer_prefs =
-      guest_web_contents->GetMutableRendererPrefs();
-  // Copy renderer preferences (and nothing else) from the embedder's
-  // TabContents to the guest.
-  //
-  // For GTK and Aura this is necessary to get proper renderer configuration
-  // values for caret blinking interval, colors related to selection and
-  // focus.
-  *guest_renderer_prefs = *web_contents()->GetMutableRendererPrefs();
-
-  guest_renderer_prefs->throttle_input_events = false;
-  // Navigation is disabled in Chrome Apps. We want to make sure guest-initiated
-  // navigations still continue to function inside the app.
-  guest_renderer_prefs->browser_handles_all_top_level_requests = false;
-  AddGuest(instance_id, guest_web_contents);
-  guest_web_contents->SetDelegate(guest);
-
-  // Create a swapped out RenderView for the guest in the embedder render
-  // process, so that the embedder can access the guest's window object.
-  int guest_routing_id =
-      static_cast<WebContentsImpl*>(guest->GetWebContents())->
-            CreateSwappedOutRenderView(web_contents()->GetSiteInstance());
-  render_view_host_->Send(new BrowserPluginMsg_GuestContentWindowReady(
-      render_view_host_->GetRoutingID(), instance_id, guest_routing_id));
-
-  guest->Initialize(params, guest_web_contents->GetRenderViewHost());
+  CreateGuest(instance_id, MSG_ROUTING_NONE, NULL, params);
 }
 
 void BrowserPluginEmbedder::OnNavigateGuest(