Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2417983002:
Form submissions opening in a new window don't need special treatment anymore. (Closed)
|
Created:
4 years, 2 months ago by Łukasz Anforowicz Modified:
4 years, 2 months ago CC:
chromium-reviews, blink-reviews, loading-reviews_chromium.org, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org, site-isolation-reviews_chromium.org, alexmos Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionForm submissions opening in a new window don't need special treatment anymore.
This CL removes one of workaround used by FrameLoader::load method when
the http request is associated with a form. The workaround is no longer
needed after r400442 fixed how HTTP method and request body are
correctly plumbed through the OpenURL path.
To make sure that the layout test exercising this functionality
(shift-clicking on the form's submit button) still passes, some
follow-up changes had to be made:
- The CL needed to add support for NEW_WINDOW disposition to
content::Shell::OpenURLFromTab
- The CL needed to tweak the test to account for the fact that
window.opener is null after opening web contents via shift-click (I've
verified that this behavior is present for non-form-related, GET
requests in 55.0.2883.18 so it seems to be WAI).
BUG=344348
Committed: https://crrev.com/0413015fe88470d3e884a0aeb679eebb13a7e766
Cr-Commit-Position: refs/heads/master@{#426876}
Patch Set 1 #Patch Set 2 : Workaround for POST-becoming-GET-in-OpenURL no longer needed after r400442. #Patch Set 3 : Rebasing on top of https://crrev.com/2410303006 #Patch Set 4 : Add support for NEW_WINDOW disposition in content::Shell::OpenURLFromTab. #
Total comments: 14
Patch Set 5 : git cl format #Patch Set 6 : Addressed CR feedback from nasko@. #Patch Set 7 : Using params.source_site_instance for consistency with //chrome layer. #
Total comments: 3
Depends on Patchset: Messages
Total messages: 36 (26 generated)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Trying to remove frame-related workarounds from FrameLoader::load. BUG= ========== to ========== Form submissions opening in a new window don't need special treatment anymore. This CL removes one of workaround used by FrameLoader::load method when the http request is associated with a form. The workaround is no longer needed after r400442 fixed how HTTP method and request body are correctly plumbed through the OpenURL path. To make sure that the layout test exercising this functionality (shift-clicking on the form's submit button) still passes, some follow-up changes had to be made: - The CL needed to add support for NEW_WINDOW disposition to content::Shell::OpenURLFromTab - The CL needed to tweak the test to account for the fact that window.opener is null after opening web contents via shift-click (I've verified that this behavior is present for non-form-related, GET requests in 55.0.2883.18 so it seems to be WAI). BUG=344348 ==========
Description was changed from ========== Form submissions opening in a new window don't need special treatment anymore. This CL removes one of workaround used by FrameLoader::load method when the http request is associated with a form. The workaround is no longer needed after r400442 fixed how HTTP method and request body are correctly plumbed through the OpenURL path. To make sure that the layout test exercising this functionality (shift-clicking on the form's submit button) still passes, some follow-up changes had to be made: - The CL needed to add support for NEW_WINDOW disposition to content::Shell::OpenURLFromTab - The CL needed to tweak the test to account for the fact that window.opener is null after opening web contents via shift-click (I've verified that this behavior is present for non-form-related, GET requests in 55.0.2883.18 so it seems to be WAI). BUG=344348 ========== to ========== Form submissions opening in a new window don't need special treatment anymore. This CL removes one of workaround used by FrameLoader::load method when the http request is associated with a form. The workaround is no longer needed after r400442 fixed how HTTP method and request body are correctly plumbed through the OpenURL path. To make sure that the layout test exercising this functionality (shift-clicking on the form's submit button) still passes, some follow-up changes had to be made: - The CL needed to add support for NEW_WINDOW disposition to content::Shell::OpenURLFromTab - The CL needed to tweak the test to account for the fact that window.opener is null after opening web contents via shift-click (I've verified that this behavior is present for non-form-related, GET requests in 55.0.2883.18 so it seems to be WAI). BUG=344348 ==========
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lukasza@chromium.org changed reviewers: + japhet@chromium.org
japhet@, can you please take a look? I'll later ask //content OWNERS to review my changes to shell.cc where I had some comments/doubts about proper way to create new WebContents - feel free to ignore my Rietveld comments in shell.cc for now. I haven't yet pushed this CL through a CQ dry run, but patchset #3 was almost green and locally patchset #4 is passing the only failing test from the previous patchset (+ all layout tests under http/tests with "form" or "post" in their name; both with and without --site-per-process). And I am not sure which bug to associate this CL with. I guess the old POST handling bug (already marked as fixed in June) is an okay choice. https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... File content/shell/browser/shell.cc (right): https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... content/shell/browser/shell.cc:324: case WindowOpenDisposition::NEW_WINDOW: { 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). https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... content/shell/browser/shell.cc:326: source->GetBrowserContext(), 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")). https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... content/shell/browser/shell.cc:328: source->GetSiteInstance(), 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). https://codereview.chromium.org/2417983002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/navigation/post-with-modifier.html (right): https://codereview.chromium.org/2417983002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/navigation/post-with-modifier.html:37: window.name = "mainTestWindow"; The change in window.opener behavior is a bit worrying, but seems desirable overall (for consistent handling of shift-clicks and ctrl-clicks wrt non-form-related GET requests - for example I've tested in 55.0.2883.18 that window.opener is null after shift-clicking a regular [i.e. GET] anchor/link). https://codereview.chromium.org/2417983002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2417983002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1094: // schedule rather than at fire. The comment text has been copy&pasted from CL description from https://crrev.com//81593002.
blink lgtm if the bots are happy
lukasza@chromium.org changed reviewers: + nasko@chromium.org
nasko@, can you PTAL? https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... File content/shell/browser/shell.cc (right): https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... content/shell/browser/shell.cc:328: source->GetSiteInstance(), On 2016/10/19 20:48:50, Łukasz Anforowicz wrote: > 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). 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... File content/shell/browser/shell.cc (right): https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... content/shell/browser/shell.cc:317: case WindowOpenDisposition::CURRENT_TAB: Shouldn't case be indented from switch? https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... content/shell/browser/shell.cc:324: case WindowOpenDisposition::NEW_WINDOW: { On 2016/10/19 20:48:50, Łukasz Anforowicz wrote: > 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). 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. https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... content/shell/browser/shell.cc:328: source->GetSiteInstance(), On 2016/10/19 20:48:50, Łukasz Anforowicz wrote: > 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). 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.
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Nasko, can you take another look? https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... File content/shell/browser/shell.cc (right): https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... content/shell/browser/shell.cc:317: case WindowOpenDisposition::CURRENT_TAB: On 2016/10/20 18:28:07, nasko (slow) wrote: > Shouldn't case be indented from switch? Ooops. Done. I've incorrectly assumed that git cl upload will complain if I need to run git cl format. https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... content/shell/browser/shell.cc:324: case WindowOpenDisposition::NEW_WINDOW: { On 2016/10/20 18:28:07, nasko (slow) wrote: > On 2016/10/19 20:48:50, Łukasz Anforowicz wrote: > > 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). > > 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. Done. https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... content/shell/browser/shell.cc:328: source->GetSiteInstance(), On 2016/10/20 18:28:07, nasko (slow) wrote: > On 2016/10/19 20:48:50, Łukasz Anforowicz wrote: > > 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). > > 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. 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). https://codereview.chromium.org/2417983002/diff/120001/content/shell/browser/... File content/shell/browser/shell.cc (right): https://codereview.chromium.org/2417983002/diff/120001/content/shell/browser/... content/shell/browser/shell.cc:331: params.source_site_instance, Using |params.source_site_instance| is consistent with //chrome layer and therefore seems better to me than using |source->GetSiteInstance()| (even if they happen to be the same here). https://codereview.chromium.org/2417983002/diff/120001/content/shell/browser/... File content/shell/browser/shell.h (right): https://codereview.chromium.org/2417983002/diff/120001/content/shell/browser/... content/shell/browser/shell.h:92: const scoped_refptr<SiteInstance>& site_instance, scoped_refptr is better than a raw pointer, right? :-) (otherwise I would have to pass |params.source_site_instance.get()| - yucky "get"...)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... File content/shell/browser/shell.cc (right): https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... content/shell/browser/shell.cc:328: source->GetSiteInstance(), On 2016/10/20 22:56:42, Łukasz Anforowicz wrote: > On 2016/10/20 18:28:07, nasko (slow) wrote: > > On 2016/10/19 20:48:50, Łukasz Anforowicz wrote: > > > 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). > > > > 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. > > 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). Staying consistent with the //chrome layer sgtm and better overall. The less inconsistencies we have, the harder to make mistakes. https://codereview.chromium.org/2417983002/diff/120001/content/shell/browser/... File content/shell/browser/shell.cc (right): https://codereview.chromium.org/2417983002/diff/120001/content/shell/browser/... content/shell/browser/shell.cc:331: params.source_site_instance, On 2016/10/20 22:56:42, Łukasz Anforowicz wrote: > Using |params.source_site_instance| is consistent with //chrome layer and > therefore seems better to me than using |source->GetSiteInstance()| (even if > they happen to be the same here). Acknowledged.
Thanks. https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... File content/shell/browser/shell.cc (right): https://codereview.chromium.org/2417983002/diff/60001/content/shell/browser/s... content/shell/browser/shell.cc:328: source->GetSiteInstance(), On 2016/10/21 16:32:47, nasko (slow) wrote: > On 2016/10/20 22:56:42, Łukasz Anforowicz wrote: > > On 2016/10/20 18:28:07, nasko (slow) wrote: > > > On 2016/10/19 20:48:50, Łukasz Anforowicz wrote: > > > > 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). > > > > > > 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. > > > > 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). > > Staying consistent with the //chrome layer sgtm and better overall. The less > inconsistencies we have, the harder to make mistakes. I've opened https://crbug.com/658386 to try to answer the question whether shift-clicked windows should stay in the same browsing instance.
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/2417983002/#ps120001 (title: "Using params.source_site_instance for consistency with //chrome layer.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Form submissions opening in a new window don't need special treatment anymore. This CL removes one of workaround used by FrameLoader::load method when the http request is associated with a form. The workaround is no longer needed after r400442 fixed how HTTP method and request body are correctly plumbed through the OpenURL path. To make sure that the layout test exercising this functionality (shift-clicking on the form's submit button) still passes, some follow-up changes had to be made: - The CL needed to add support for NEW_WINDOW disposition to content::Shell::OpenURLFromTab - The CL needed to tweak the test to account for the fact that window.opener is null after opening web contents via shift-click (I've verified that this behavior is present for non-form-related, GET requests in 55.0.2883.18 so it seems to be WAI). BUG=344348 ========== to ========== Form submissions opening in a new window don't need special treatment anymore. This CL removes one of workaround used by FrameLoader::load method when the http request is associated with a form. The workaround is no longer needed after r400442 fixed how HTTP method and request body are correctly plumbed through the OpenURL path. To make sure that the layout test exercising this functionality (shift-clicking on the form's submit button) still passes, some follow-up changes had to be made: - The CL needed to add support for NEW_WINDOW disposition to content::Shell::OpenURLFromTab - The CL needed to tweak the test to account for the fact that window.opener is null after opening web contents via shift-click (I've verified that this behavior is present for non-form-related, GET requests in 55.0.2883.18 so it seems to be WAI). BUG=344348 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Form submissions opening in a new window don't need special treatment anymore. This CL removes one of workaround used by FrameLoader::load method when the http request is associated with a form. The workaround is no longer needed after r400442 fixed how HTTP method and request body are correctly plumbed through the OpenURL path. To make sure that the layout test exercising this functionality (shift-clicking on the form's submit button) still passes, some follow-up changes had to be made: - The CL needed to add support for NEW_WINDOW disposition to content::Shell::OpenURLFromTab - The CL needed to tweak the test to account for the fact that window.opener is null after opening web contents via shift-click (I've verified that this behavior is present for non-form-related, GET requests in 55.0.2883.18 so it seems to be WAI). BUG=344348 ========== to ========== Form submissions opening in a new window don't need special treatment anymore. This CL removes one of workaround used by FrameLoader::load method when the http request is associated with a form. The workaround is no longer needed after r400442 fixed how HTTP method and request body are correctly plumbed through the OpenURL path. To make sure that the layout test exercising this functionality (shift-clicking on the form's submit button) still passes, some follow-up changes had to be made: - The CL needed to add support for NEW_WINDOW disposition to content::Shell::OpenURLFromTab - The CL needed to tweak the test to account for the fact that window.opener is null after opening web contents via shift-click (I've verified that this behavior is present for non-form-related, GET requests in 55.0.2883.18 so it seems to be WAI). BUG=344348 Committed: https://crrev.com/0413015fe88470d3e884a0aeb679eebb13a7e766 Cr-Commit-Position: refs/heads/master@{#426876} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0413015fe88470d3e884a0aeb679eebb13a7e766 Cr-Commit-Position: refs/heads/master@{#426876} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
