Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2680353005:
WebContents created via ctrl-click should be in a new process (target=_blank). (Closed)
|
Created:
3 years, 10 months ago by Łukasz Anforowicz Modified:
3 years, 8 months ago CC:
chromium-reviews, blink-reviews, alexmos, Nate Chapin, site-isolation-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebContents created via ctrl-click should be in a new process (target=_blank).
Previous CL (https://crrev.com/2686943002) started putting WebContents
created via ctrl-click into a new process. One case that was still
broken is target="_blank" (or target="non-existant-name").
At first glance, it might seem arbitrary to decide which directive
(ctrl-click VS target=_blank) wins and influences 1) process allocation
behavior and 2) window.opener / browsing instance behavior. In reality,
ctrl-click should win because:
1. This is the behavior desired by users (e.g. see
https://crbug.com/23815).
2. This is the behavior of other browsers (e.g. see
https://crbug.com/658386#c9).
Before this CL, FrameLoader::load would ignore ctrl-click /
NavigationPolicyNewForegroundTab and go through createWindowForRequest
path if the named target frame didn't exist when the navigation is
initiated. After this CL, ctrl-click "wins".
This CL has to tweak 2 layout tests affected by the change. In both
cases, the test expectation that a background-tab-navigation is handled
by createView is no longer correct - now the navigation goes through
OpenURL path rather than through CreateWindowForRequest. Accomodating
the 2 tests also required adding support for handling
WindowOpenDisposition::NEW_BACKGROUND_TAB in
content::Shell::OpenURLFromTab.
NOPRESUBMIT usage has been discussed in https://crrev.com/2680353005/#msg50
BUG=23815, 658386
NOPRESUBMIT=true
Review-Url: https://codereview.chromium.org/2680353005
Cr-Commit-Position: refs/heads/master@{#466843}
Committed: https://chromium.googlesource.com/chromium/src/+/be2f0da0b064edc7e59d08129538a09d3b2f30c1
Patch Set 1 #Patch Set 2 : Rebasing... #Patch Set 3 : Rebasing... #Patch Set 4 : Change layout test expectations to match product code changes. #
Total comments: 6
Patch Set 5 : Explicitly reusing code instead of using parametrized tests (in response to CR feedback). #Patch Set 6 : Rebasing... #Patch Set 7 : Rebasing on top of changes in the CL we depend on. #Patch Set 8 : The new field needs to be initialized in all constructor overrides. #Patch Set 9 : Fixing test expectations for middle-click-target-blank.html layout test. #Patch Set 10 : Rebasing... #
Total comments: 2
Patch Set 11 : Rebasing... #Patch Set 12 : Rebasing updating LayoutTest/FlagExpectations to account for test name change. #Messages
Total messages: 65 (57 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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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...
Description was changed from ========== WebContents created via ctrl-click should be in a new process (target=_blank). Previous CL (https://crrev.com/2686943002) started putting WebContents created via ctrl-click into a new process. One case that was still broken is target="_blank" (or target="non-existant-name"). At first glance, it might seem arbitrary to decide which directive (ctrl-click VS target=_blank) wins and influences 1) process allocation behavior and 2) window.opener / browsing instance behavior. In reality, ctrl-click should win because: 1. This is the behavior desired by users (e.g. see https://crbug.com/23815). 2. This is the behavior of other browsers (e.g. see https://crbug.com/658386#c9). Before this CL, FrameLoader::load would ignore ctrl-click / NavigationPolicyNewForegroundTab and go through createWindowForRequest path if the named target frame didn't exist when the navigation is initiated. After this CL, ctrl-click "wins". BUG=23815, 658386 ========== to ========== WebContents created via ctrl-click should be in a new process (target=_blank). Previous CL (https://crrev.com/2686943002) started putting WebContents created via ctrl-click into a new process. One case that was still broken is target="_blank" (or target="non-existant-name"). At first glance, it might seem arbitrary to decide which directive (ctrl-click VS target=_blank) wins and influences 1) process allocation behavior and 2) window.opener / browsing instance behavior. In reality, ctrl-click should win because: 1. This is the behavior desired by users (e.g. see https://crbug.com/23815). 2. This is the behavior of other browsers (e.g. see https://crbug.com/658386#c9). Before this CL, FrameLoader::load would ignore ctrl-click / NavigationPolicyNewForegroundTab and go through createWindowForRequest path if the named target frame didn't exist when the navigation is initiated. After this CL, ctrl-click "wins". This CL has to tweak 2 layout tests affected by the change. In both cases, the test expectation that a background-tab-navigation is handled by createView is no longer correct - now the navigation goes through OpenURL path rather than through CreateWindowForRequest. Accomodating the 2 tests also required adding support for handling WindowOpenDisposition::NEW_BACKGROUND_TAB in content::Shell::OpenURLFromTab. BUG=23815, 658386 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lukasza@chromium.org changed reviewers: + creis@chromium.org
Charlie, can you PTAL? https://codereview.chromium.org/2680353005/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/isolated-worlds-override-keystate-expected.txt (right): https://codereview.chromium.org/2680353005/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/isolated-worlds-override-keystate-expected.txt:2: Default policy for navigation to 'blank2' is 'new background tab' Since this test is no longer going through CreateWindowsForRequest (after this CL ctrl-clicks will go through RequestOpenURL path instead), the test expectations have to be adjusted accordingly. https://codereview.chromium.org/2680353005/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/loader/middle-click-target-blank-expected.txt (right): https://codereview.chromium.org/2680353005/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/loader/middle-click-target-blank-expected.txt:1: Default policy for navigation to 'done.html' is 'new background tab' Since this test is no longer going through CreateWindowsForRequest (after this CL middle-clicks will go through RequestOpenURL path instead), the test expectations have to be adjusted accordingly. https://codereview.chromium.org/2680353005/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/loader/middle-click-target-blank.html (right): https://codereview.chromium.org/2680353005/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/loader/middle-click-target-blank.html:7: testRunner.dumpNavigationPolicy(); This artificially preserves test coverage to get the new message in test results (see the changes in test expectations). TBH, I am not sure if this test is really needed / valuable - it seems to test implementation details, not things that an end-user would care about. Maybe we should remove this test and say that it is being replaced by the browser tests being added in this CL? https://codereview.chromium.org/2680353005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2680353005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:916: } else if (ShouldNavigateTargetFrame(policy)) { This is the main fix - we should avoid going via CreateWindowForRequest if the |policy| says that we shouldn't navigate the target frame (e.g. because we are opening the link in a new tab). The ShouldNavigateTargetFrame was introduced previously to fix similar bugs (e.g. see https://crrev.com/2410303006 - avoiding going through RenderFrameProxy when targeting a *new* frame).
creis@chromium.org changed reviewers: + japhet@chromium.org
LGTM, but let's get Nate's thoughts on the Blink parts as well. https://codereview.chromium.org/2680353005/diff/60001/chrome/browser/chrome_n... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2680353005/diff/60001/chrome/browser/chrome_n... chrome/browser/chrome_navigation_browsertest.cc:194: public ::testing::WithParamInterface<const char*> {}; Personally I find parameterized tests really confusing to read/run/debug. Would it be roughly equivalent to use a lambda in the test itself and call it with each anchor name?
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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: This issue passed the CQ dry run.
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...
Thanks Charlie. Nate - can you PTAL? https://codereview.chromium.org/2680353005/diff/60001/chrome/browser/chrome_n... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2680353005/diff/60001/chrome/browser/chrome_n... chrome/browser/chrome_navigation_browsertest.cc:194: public ::testing::WithParamInterface<const char*> {}; On 2017/04/14 22:39:28, Charlie Reis wrote: > Personally I find parameterized tests really confusing to read/run/debug. Would > it be roughly equivalent to use a lambda in the test itself and call it with > each anchor name? I've switched to separate tests that call to a shared/common method in patchset #5. https://codereview.chromium.org/2680353005/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/loader/middle-click-target-blank-expected.txt (right): https://codereview.chromium.org/2680353005/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/loader/middle-click-target-blank-expected.txt:1: Default policy for navigation to 'done.html' is 'new background tab' This comes from when we actually handle the middle click and end up calling RenderFrameImpl::OpenURL. This is the part that we care about (i.e. we want the test to continue verifying that the link got opened in a new background tab). https://codereview.chromium.org/2680353005/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/loader/middle-click-target-blank-expected.txt:2: Default policy for navigation to 'done.html' is 'current tab' This comes later, after the new tab got already created and when we are navigating the new tab to the expected URL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
lukasza@chromium.org changed reviewers: + lanwei@chromium.org
lanwei@, could you please help me figure out what I should do about the presubmit failures hit by this CL (*)? I am not adding a new usage (or even touching an existing usage) of eventSender layout tests API - I am only renaming create-view-target-blank.html layout test to a more appropriate name: middle-click-target-blank.html. Given the above, would it be okay if I skip the failing presubmit when landing my CL (i.e. add NOPRESUBMIT=true after verifying manually that all the other presubmits are passing)? If not, could you please provide some guidance and/or examples on how to replace eventSender simulation of middle-clicking an anchor/link with an equivalent chrome.gpuBenchmarking.pointerActionSequence code. Regardless of what we should do with this CL, I think it would be useful to add a link to the migration guidance/examples to 1) https://crbug.com/711340 and 2) to the PRESUBMIT message. And in addition to giving guidance/examples for migration, it would be great if we could describe the eventSender deprecation more broadly: - Is eventSender effectively deprecated altogether or only in some specific scenarios? Why are we deprecating eventSender / what is the problem with using eventSender? - What is the plan for migrating all the legacy uses of eventSender? Do you plan to tackle the migration yourself, or do you want everyone to chime in as they touch tests using eventSender? WDYT? (*) Copy&pasting the failures reported by |git cl presubmit|: ** Presubmit ERRORS ** Files that still use eventSender: third_party/WebKit/LayoutTests/fast/loader/middle-click-target-blank.html:16 eventSender.mouseMoveTo(link.offsetLeft + 2, link.offsetTop + 2);, please use chrome.gpuBenchmarking.pointerActionSequence instead. Files that still use eventSender: third_party/WebKit/LayoutTests/fast/loader/middle-click-target-blank.html:17 eventSender.mouseDown(middleMouseButton);, please use chrome.gpuBenchmarking.pointerActionSequence instead. Files that still use eventSender: third_party/WebKit/LayoutTests/fast/loader/middle-click-target-blank.html:18 eventSender.mouseUp(middleMouseButton);, please use chrome.gpuBenchmarking.pointerActionSequence instead.
On 2017/04/24 19:09:36, Łukasz A. wrote: > lanwei@, could you please help me figure out what I should do about the > presubmit failures hit by this CL (*)? > > I am not adding a new usage (or even touching an existing usage) of eventSender > layout tests API - I am only renaming create-view-target-blank.html layout test > to a more appropriate name: middle-click-target-blank.html. Given the above, > would it be okay if I skip the failing presubmit when landing my CL (i.e. add > NOPRESUBMIT=true after verifying manually that all the other presubmits are > passing)? If not, could you please provide some guidance and/or examples on how > to replace eventSender simulation of middle-clicking an anchor/link with an > equivalent chrome.gpuBenchmarking.pointerActionSequence code. > > Regardless of what we should do with this CL, I think it would be useful to add > a link to the migration guidance/examples to 1) https://crbug.com/711340 and 2) > to the PRESUBMIT message. And in addition to giving guidance/examples for > migration, it would be great if we could describe the eventSender deprecation > more broadly: > - Is eventSender effectively deprecated altogether or only in some specific > scenarios? Why are we deprecating eventSender / what is the problem with using > eventSender? > - What is the plan for migrating all the legacy uses of eventSender? Do you > plan to tackle the migration yourself, or do you want everyone to chime in as > they touch tests using eventSender? > > WDYT? > > (*) Copy&pasting the failures reported by |git cl presubmit|: > > ** Presubmit ERRORS ** > Files that still use eventSender: > third_party/WebKit/LayoutTests/fast/loader/middle-click-target-blank.html:16 > eventSender.mouseMoveTo(link.offsetLeft + 2, link.offsetTop + 2);, please use > chrome.gpuBenchmarking.pointerActionSequence instead. > > Files that still use eventSender: > third_party/WebKit/LayoutTests/fast/loader/middle-click-target-blank.html:17 > eventSender.mouseDown(middleMouseButton);, please use > chrome.gpuBenchmarking.pointerActionSequence instead. > > Files that still use eventSender: > third_party/WebKit/LayoutTests/fast/loader/middle-click-target-blank.html:18 > eventSender.mouseUp(middleMouseButton);, please use > chrome.gpuBenchmarking.pointerActionSequence instead. We are targeting to the new added layout tests that uses eventSender, but I guess renaming is treated as deleting and adding new files. If you want, you can skip this presubmit check. I also gave an example of how to replacing the eventsender, if you want to use it, I am very happy to help you. Thank you for your suggestions, I will add more explanations of why we want to use chrome.gpuBenchmarking.pointerActionSequence to replace eventSendeer in the bug, and add the bug number to the error message.
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== WebContents created via ctrl-click should be in a new process (target=_blank). Previous CL (https://crrev.com/2686943002) started putting WebContents created via ctrl-click into a new process. One case that was still broken is target="_blank" (or target="non-existant-name"). At first glance, it might seem arbitrary to decide which directive (ctrl-click VS target=_blank) wins and influences 1) process allocation behavior and 2) window.opener / browsing instance behavior. In reality, ctrl-click should win because: 1. This is the behavior desired by users (e.g. see https://crbug.com/23815). 2. This is the behavior of other browsers (e.g. see https://crbug.com/658386#c9). Before this CL, FrameLoader::load would ignore ctrl-click / NavigationPolicyNewForegroundTab and go through createWindowForRequest path if the named target frame didn't exist when the navigation is initiated. After this CL, ctrl-click "wins". This CL has to tweak 2 layout tests affected by the change. In both cases, the test expectation that a background-tab-navigation is handled by createView is no longer correct - now the navigation goes through OpenURL path rather than through CreateWindowForRequest. Accomodating the 2 tests also required adding support for handling WindowOpenDisposition::NEW_BACKGROUND_TAB in content::Shell::OpenURLFromTab. BUG=23815, 658386 ========== to ========== WebContents created via ctrl-click should be in a new process (target=_blank). Previous CL (https://crrev.com/2686943002) started putting WebContents created via ctrl-click into a new process. One case that was still broken is target="_blank" (or target="non-existant-name"). At first glance, it might seem arbitrary to decide which directive (ctrl-click VS target=_blank) wins and influences 1) process allocation behavior and 2) window.opener / browsing instance behavior. In reality, ctrl-click should win because: 1. This is the behavior desired by users (e.g. see https://crbug.com/23815). 2. This is the behavior of other browsers (e.g. see https://crbug.com/658386#c9). Before this CL, FrameLoader::load would ignore ctrl-click / NavigationPolicyNewForegroundTab and go through createWindowForRequest path if the named target frame didn't exist when the navigation is initiated. After this CL, ctrl-click "wins". This CL has to tweak 2 layout tests affected by the change. In both cases, the test expectation that a background-tab-navigation is handled by createView is no longer correct - now the navigation goes through OpenURL path rather than through CreateWindowForRequest. Accomodating the 2 tests also required adding support for handling WindowOpenDisposition::NEW_BACKGROUND_TAB in content::Shell::OpenURLFromTab. NOPRESUBMIT usage has been discussed in https://crrev.com/2680353005/#msg50 BUG=23815, 658386 NOPRESUBMIT=true ==========
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, japhet@chromium.org Link to the patchset: https://codereview.chromium.org/2680353005/#ps220001 (title: "Rebasing updating LayoutTest/FlagExpectations to account for test name change.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1493078509457690,
"parent_rev": "6aee4d67dcc67850c8ec7b9fc181cb7d91b55734", "commit_rev":
"be2f0da0b064edc7e59d08129538a09d3b2f30c1"}
Message was sent while issue was closed.
Description was changed from ========== WebContents created via ctrl-click should be in a new process (target=_blank). Previous CL (https://crrev.com/2686943002) started putting WebContents created via ctrl-click into a new process. One case that was still broken is target="_blank" (or target="non-existant-name"). At first glance, it might seem arbitrary to decide which directive (ctrl-click VS target=_blank) wins and influences 1) process allocation behavior and 2) window.opener / browsing instance behavior. In reality, ctrl-click should win because: 1. This is the behavior desired by users (e.g. see https://crbug.com/23815). 2. This is the behavior of other browsers (e.g. see https://crbug.com/658386#c9). Before this CL, FrameLoader::load would ignore ctrl-click / NavigationPolicyNewForegroundTab and go through createWindowForRequest path if the named target frame didn't exist when the navigation is initiated. After this CL, ctrl-click "wins". This CL has to tweak 2 layout tests affected by the change. In both cases, the test expectation that a background-tab-navigation is handled by createView is no longer correct - now the navigation goes through OpenURL path rather than through CreateWindowForRequest. Accomodating the 2 tests also required adding support for handling WindowOpenDisposition::NEW_BACKGROUND_TAB in content::Shell::OpenURLFromTab. NOPRESUBMIT usage has been discussed in https://crrev.com/2680353005/#msg50 BUG=23815, 658386 NOPRESUBMIT=true ========== to ========== WebContents created via ctrl-click should be in a new process (target=_blank). Previous CL (https://crrev.com/2686943002) started putting WebContents created via ctrl-click into a new process. One case that was still broken is target="_blank" (or target="non-existant-name"). At first glance, it might seem arbitrary to decide which directive (ctrl-click VS target=_blank) wins and influences 1) process allocation behavior and 2) window.opener / browsing instance behavior. In reality, ctrl-click should win because: 1. This is the behavior desired by users (e.g. see https://crbug.com/23815). 2. This is the behavior of other browsers (e.g. see https://crbug.com/658386#c9). Before this CL, FrameLoader::load would ignore ctrl-click / NavigationPolicyNewForegroundTab and go through createWindowForRequest path if the named target frame didn't exist when the navigation is initiated. After this CL, ctrl-click "wins". This CL has to tweak 2 layout tests affected by the change. In both cases, the test expectation that a background-tab-navigation is handled by createView is no longer correct - now the navigation goes through OpenURL path rather than through CreateWindowForRequest. Accomodating the 2 tests also required adding support for handling WindowOpenDisposition::NEW_BACKGROUND_TAB in content::Shell::OpenURLFromTab. NOPRESUBMIT usage has been discussed in https://crrev.com/2680353005/#msg50 BUG=23815, 658386 NOPRESUBMIT=true Review-Url: https://codereview.chromium.org/2680353005 Cr-Commit-Position: refs/heads/master@{#466843} Committed: https://chromium.googlesource.com/chromium/src/+/be2f0da0b064edc7e59d08129538... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/be2f0da0b064edc7e59d08129538... |
