Try to restore window.opener when opening blocked popups

content/browser/web_contents/web_contents_impl.cc

Index: content/browser/web_contents/web_contents_impl.cc
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index fe34bc0ef8a11d9eafcc62fa09f5e9829945b869..8d2a92f831dd0da8499e139df606c195f4f2746c 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -287,6 +287,46 @@ WebContents* WebContents::CreateWithSessionStorage(
   return new_contents;
 }
 
+// static
+WebContents* WebContents::CreateWithOpener(
+    const WebContents::CreateParams& params,
+    WebContents* opener,
+    bool propagate_opener) {

[Charlie Reis, 2013/07/30 01:26:30]

Why switch from suppress_opener, which is used commonly elsewhere?

+  WebContentsImpl* new_contents = new WebContentsImpl(
+      params.browser_context,
+      propagate_opener ? static_cast<WebContentsImpl*>(opener) : NULL);
+
+  if (opener) {
+    // To clone the session storage namespace, we need a site instance.
+    CHECK(params.site_instance);

[Bernhard Bauer, 2013/07/30 08:34:50]

I assume you have good reasons for using a CHECK over a DCHECK here? If it's
just "this shouldn't happen", I'd prefer a DCHECK for that.

+
+    // It's not possible to use a guest as opener.

[Charlie Reis, 2013/07/30 01:26:30]

That's not true.  Fady changed WebContentsImpl::CreateNewWindow to support
window.open from guests.

+    CHECK(!opener->GetRenderProcessHost()->IsGuest());
+
+    // Clone the session storage namespace of the opener.

[Charlie Reis, 2013/07/30 01:26:30]

This code only copies the session storage namespace for the current
SiteInstance, which will be a problem if the popup navigates to a different
SiteInstance that had previously been open in the opener window.

The logic in CreateWithSessionStorage already handles this by copying over the
whole map.  Is there some way that we can make these various functions share
code?  It's getting a little unwieldy with Create, CreateWithSessionStorage, and
two CreateWithOpeners, though I'm not sure if they're easy to combine.  (They
all take in CreateParams, so it seems like it might be possible.)

[jochen (gone - plz use gerrit), 2013/07/30 14:50:39]

No. it clones it (see line 320).
John suggested to move the cloning logic to browser navigator and use
CreateWithSessionStorage.

[Charlie Reis, 2013/07/31 17:16:16]

You're only cloning a single SessionStorageNamespace below.  Each WebContents
has an entire map, and the whole map needs to be cloned, as in
CreateWithSessionStorage.

+    const std::string& partition_id =
+      GetContentClient()->browser()->

[Bernhard Bauer, 2013/07/30 08:34:50]

Nit: This needs to be indented four spaces more.

+          GetStoragePartitionIdForSite(opener->GetBrowserContext(),
+                                       params.site_instance->GetSiteURL());
+    StoragePartition* partition = BrowserContext::GetStoragePartition(
+        opener->GetBrowserContext(), params.site_instance);
+    DOMStorageContextImpl* dom_storage_context =
+        static_cast<DOMStorageContextImpl*>(partition->GetDOMStorageContext());
+    SessionStorageNamespaceMap::const_iterator old_namespace = opener
+        ->GetController().GetSessionStorageNamespaceMap().find(partition_id);

[Charlie Reis, 2013/07/30 01:26:30]

nit: Please be consistent about -> being at the end or beginning of a line (vs
line 308).  Personally, I prefer -> at the end (like line 308) and . at the
beginning (like line 323).

+    CHECK(old_namespace !=
+          opener->GetController().GetSessionStorageNamespaceMap().end());

[Bernhard Bauer, 2013/07/30 08:34:50]

If the partition ID isn't found, you'll crash later anyway.

+    scoped_refptr<SessionStorageNamespaceImpl> cloned_namespace =
+        new SessionStorageNamespaceImpl(dom_storage_context,
+                                        old_namespace->second->id());
+    new_contents->GetController()
+        .SetSessionStorageNamespace(partition_id, cloned_namespace.get());
+  }
+
+  new_contents->Init(params);
+  return new_contents;
+}
+
 void WebContents::AddCreatedCallback(const CreatedCallback& callback) {
   g_created_callbacks.Get().push_back(callback);
 }