Form submissions opening in a new window don't need special treatment anymore.

content/shell/browser/shell.cc

Index: content/shell/browser/shell.cc
diff --git a/content/shell/browser/shell.cc b/content/shell/browser/shell.cc
index a214d2a61a115dee8bce6f6fef68deda6a2b5185..30f091435ef50c3f43999f0d5ef886b71625cf05 100644
--- a/content/shell/browser/shell.cc
+++ b/content/shell/browser/shell.cc
@@ -312,9 +312,43 @@ gfx::NativeView Shell::GetContentView() {
 
 WebContents* Shell::OpenURLFromTab(WebContents* source,
                                    const OpenURLParams& params) {
-  // This implementation only handles CURRENT_TAB.
-  if (params.disposition != WindowOpenDisposition::CURRENT_TAB)
+  WebContents* target = nullptr;
+  switch (params.disposition) {
+  case WindowOpenDisposition::CURRENT_TAB:

[nasko, 2016/10/20 18:28:07]

Shouldn't case be indented from switch?

[Łukasz Anforowicz, 2016/10/20 22:56:42]

Ooops.  Done.  I've incorrectly assumed that git cl upload will complain if I
need to run git cl format.

+    target = source;
+    break;
+
+  // NEW_POPUP and NEW_WINDOW can be treated in the same way in content shell
+  // and layout tests.
+  case WindowOpenDisposition::NEW_POPUP:
+  case WindowOpenDisposition::NEW_WINDOW: {

[Łukasz Anforowicz, 2016/10/19 20:48:50]

It seems to be that the only difference between NEW_POPUP and NEW_WINDOW is that
a popup is supposed to have no toolbar, no status bar, no menu bar, no
scrollbars and be not resizable.  I think that having a toolbar (and other
things) is okay for popups shown by content_shell (and I think that layout tests
shouldn't care about this for now).

FWIW, this CL only really requires support for NEW_WINDOW disposition, so if
handling of NEW_POPUP in the same way as NEW_WINDOW doesn't seem desirable, then
we can always make NEW_POPUP unhandled (just as it was before this CL).

[nasko, 2016/10/20 18:28:07]

I'm ok with leaving it in, but please put part of your first paragraph as a
comment to explain why we are combining the two.

[Łukasz Anforowicz, 2016/10/20 22:56:42]

Done.

+    Shell* new_window = Shell::CreateNewWindow(
+        source->GetBrowserContext(),

[Łukasz Anforowicz, 2016/10/19 20:48:50]

I've verified that shift-clicked windows stay in the same browsing
instance/context as the old window (in 55.0.2883.18 I've 1) set
window.name="blah", 2) shift-clicked, 3) in the new console was able to find the
other window via window.open("", "blah")).

+        GURL(),  // Don't load anything just yet.
+        source->GetSiteInstance(),

[Łukasz Anforowicz, 2016/10/19 20:48:50]

I am not quite sure what I should use as the site instance of the new web
contents (since we are immediately loading different content in the remainder of
this method).

[Łukasz Anforowicz, 2016/10/19 22:23:00]

Some notes from my little research:

- I see that other users of WebContents::CreateParams also populate it with
browser context and site instance of the source contents - this is for example
what happens in CreateTargetContents in chrome/browser/ui/browser_navigator.cc.

- OTOH, there is also OpenURLParams::source_site_instance - we could use this
one here.

- And also - sometimes a brand new site instance is created (but this is
probably wrong for things like about:blank), but this only seems to happen for
things that I think don't apply here (e.g. WebContentsImpl::CreateNewWindow
checks params.opener_suppressed && !is_guest).


Still, I think the arguments of Shell::CreateNewWindow above are fine.

[nasko, 2016/10/20 18:28:07]

Setting a null SiteInstance is probably better. The goal of this parameter is to
supply the SiteInstance ahead of time if we know it. Here we don't.

[Łukasz Anforowicz, 2016/10/20 22:56:42]

Interesting.  FWIW, I see that WebContentsImpl::Init properly handles nullptr
here (by initializing the site instance via
SiteInstance::Create(params.browser_context)).

I've verified that:

(1.) Even without my CL OpenURLFromTab gets invoked after shift-clicking a
regular (i.e. HTTP GET) anchor/link.

(2.) I see that OpenURLFromTab in //chrome layer uses a non-null site instance
when creating the new WebContents:

#2 0x7f109253b6c9 content::WebContentsImpl::CreateWithOpener()
#3 0x7f109253b4e9 content::WebContents::Create()
#4 0x7f109e793ae7 chrome::Navigate() <- around here
#5 0x7f109e76ad09 Browser::OpenURLFromTab()
#6 0x7f1092579221 content::WebContentsImpl::RequestOpenURL()
#7 0x7f1091c0de8c content::NavigatorImpl::RequestOpenURL()
#8 0x7f1091c3b160 content::RenderFrameHostImpl::OpenURL()
#9 0x7f1091c21eef content::RenderFrameHostImpl::OnOpenURL()

chrome/browser/ui/browser_navigator.cc:

# this is inlined between frames #3 and #4
content::WebContents* CreateTargetContents(const chrome::NavigateParams& params,
                                           const GURL& url) {
  WebContents::CreateParams create_params(
      params.browser->profile(),
      params.source_site_instance
          ? params.source_site_instance
          : tab_util::GetSiteInstanceForNewTab(params.browser->profile(), url));

I see that |params.source_site_instance| originates from
|this->GetSiteInstance()| call made in RenderFrameHostImpl::OnOpenURL.


Now (2.) above means that the WebContents temporarily has a
RenderView/RenderFrame in the old renderer and consequently can find windows by
name in the old browsing instance.  OTOH, where exactly RV/RF is created is
somewhat accidental (i.e. a specific renderer process is not guaranteed, even if
passing the same site instance;  even if it is guaranteed today, then it might
not be in the future, right?).


So - I think the right thing to do is to 1) proceed with the CL under review
using source->GetSiteInstance() [i.e. same behavior as the implementation of
OpenURLFromTab in //chrome layer] and 2) open a separate bug to investigate
whether shift-clicking should open the new window in the same or old browsing
instance (possibly comparing with behavior in other browsers).

WDYT?


PS. If putting the new window in a separate browsing instance is desirable, then
today it is difficult to continue testing this scenario via a layout test - the
new window has currently no way to communicate back verification results (i.e.
if method and form value are correct) back to the layout test harness.  OTOH, in
the future we might want to enable this by either 1) replicating custom text
output and/or 2) making sure that console messages are forwarded to the browser
from *all* test windows (there is already a bug for this, but this is tricky to
do).

[nasko, 2016/10/21 16:32:47]

Staying consistent with the //chrome layer sgtm and better overall. The less
inconsistencies we have, the harder to make mistakes.

[Łukasz Anforowicz, 2016/10/21 20:02:50]

I've opened https://crbug.com/658386 to try to answer the question whether
shift-clicked windows should stay in the same browsing instance.

+        gfx::Size());  // Use default size.
+    target = new_window->web_contents();
+    if (switches::IsRunLayoutTestSwitchPresent())
+      SecondaryTestWindowObserver::CreateForWebContents(target);
+    break;
+  }
+
+  // No tabs in content_shell:
+  case WindowOpenDisposition::SINGLETON_TAB:
+  case WindowOpenDisposition::NEW_FOREGROUND_TAB:
+  case WindowOpenDisposition::NEW_BACKGROUND_TAB:
+  // No incognito mode in content_shell:
+  case WindowOpenDisposition::OFF_THE_RECORD:
+  // TODO(lukasza): Investigate if some layout tests might need support for
+  // SAVE_TO_DISK disposition.  This would probably require that
+  // BlinkTestController always sets up and cleans up a temporary directory
+  // as the default downloads destinations for the duration of a test.
+  case WindowOpenDisposition::SAVE_TO_DISK:
+  // Ignoring requests with disposition == IGNORE_ACTION...
+  case WindowOpenDisposition::IGNORE_ACTION:
+  default:
     return nullptr;
+  }
 
   NavigationController::LoadURLParams load_url_params(params.url);
   load_url_params.source_site_instance = params.source_site_instance;
@@ -332,8 +366,8 @@ WebContents* Shell::OpenURLFromTab(WebContents* source,
     load_url_params.post_data = params.post_data;
   }
 
-  source->GetController().LoadURLWithParams(load_url_params);
-  return source;
+  target->GetController().LoadURLWithParams(load_url_params);
+  return target;
 }
 
 void Shell::LoadingStateChanged(WebContents* source,