With the following combination of flags: WebSettings.setJavaScriptCanOpenWindowsAutomatically(true) WebSettings.setSupportMultipleWindows(false) Android WebView "opens" new windows in the same WebView, replacing the current content and updating the history accordingly. This applies to "window.open" calls, <a href target="_blank"> and <form method="post" target="_blank"> elements. This behavior is tested in CTS tests, and at least GMail application relies on it. The patch is coming.
Created attachment 169408 [details] Implementation
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Attachment 169408 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit/chromium/tests/SupportMultipleWindowsSettingTest.cpp:35: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit/chromium/tests/SupportMultipleWindowsSettingTest.cpp:42: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit/chromium/src/WebViewImpl.cpp:4161: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 3 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 169409 [details] Fixed style errors
Chromium change that makes use of the setting: https://codereview.chromium.org/11192057/
Comment on attachment 169409 [details] Fixed style errors View in context: https://bugs.webkit.org/attachment.cgi?id=169409&action=review There's no reason for this feature to be Android-specific. Please consider that other operating systems might want to use this setting in the future. It's not clear to me whether we should implement this setting in the WebKit layer or in the WebCore layer. My instinct is to do it in WebCore, but I'm not sure what a patch that took that approach would look like. > Source/WebCore/loader/FrameLoader.cpp:3281 > +#if OS(ANDROID) FrameLoader.cpp should not have any OS(ANDROID) ifdefs. > Source/WebKit/chromium/WebKit.gypi:154 > + ['OS=="android"', { > + 'webkit_unittest_files': [ > + 'tests/SupportMultipleWindowsSettingTest.cpp', > + ], > + }], We should be able to run this unit test on all platforms, not just Android. > Source/WebKit/chromium/public/WebSettings.h:168 > + // This setting is only used on Android. Please remove this comment. We might use this setting on other platforms. > Source/WebKit/chromium/src/ChromeClientImpl.cpp:258 > +#if OS(ANDROID) > + if (!m_webView->supportMultipleWindows()) > + return frame->page(); > +#endif This should not be #if OS(ANDROID) > Source/WebKit/chromium/src/WebViewImpl.cpp:4164 > +bool WebViewImpl::supportMultipleWindows() > +{ > + return settingsImpl()->supportMultipleWindows(); > +} There's no need for this function. > Source/WebKit/chromium/src/WebViewImpl.h:345 > + bool supportMultipleWindows(); There's no need for this function. > Source/WebKit/chromium/tests/SupportMultipleWindowsSettingTest.cpp:98 > +TEST_F(SupportMultipleWindowsSettingTest, WindowOpenSupportMultipleWindowsDisabled) There should be an existing test file that you can add this test to. I think there's already a test file for unit testing WebView or WebFrame.
Created attachment 169628 [details] A WebCore version of the patch, as Adam has suggested
(In reply to comment #6) > (From update of attachment 169409 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169409&action=review > > There's no reason for this feature to be Android-specific. Please consider that other operating systems might want to use this setting in the future. It's not clear to me whether we should implement this setting in the WebKit layer or in the WebCore layer. My instinct is to do it in WebCore, but I'm not sure what a patch that took that approach would look like. > Thanks for the review! OK, moved the setting and checks into WebCore. Port-specific changes are still required, if the port calls chrome's createWindow method (as in chromium port). All comments seem to be addressed. > > Source/WebCore/loader/FrameLoader.cpp:3281 > > +#if OS(ANDROID) > > FrameLoader.cpp should not have any OS(ANDROID) ifdefs. > > > Source/WebKit/chromium/WebKit.gypi:154 > > + ['OS=="android"', { > > + 'webkit_unittest_files': [ > > + 'tests/SupportMultipleWindowsSettingTest.cpp', > > + ], > > + }], > > We should be able to run this unit test on all platforms, not just Android. > > > Source/WebKit/chromium/public/WebSettings.h:168 > > + // This setting is only used on Android. > > Please remove this comment. We might use this setting on other platforms. > > > Source/WebKit/chromium/src/ChromeClientImpl.cpp:258 > > +#if OS(ANDROID) > > + if (!m_webView->supportMultipleWindows()) > > + return frame->page(); > > +#endif > > This should not be #if OS(ANDROID) > > > Source/WebKit/chromium/src/WebViewImpl.cpp:4164 > > +bool WebViewImpl::supportMultipleWindows() > > +{ > > + return settingsImpl()->supportMultipleWindows(); > > +} > > There's no need for this function. > > > Source/WebKit/chromium/src/WebViewImpl.h:345 > > + bool supportMultipleWindows(); > > There's no need for this function. > > > Source/WebKit/chromium/tests/SupportMultipleWindowsSettingTest.cpp:98 > > +TEST_F(SupportMultipleWindowsSettingTest, WindowOpenSupportMultipleWindowsDisabled) > > There should be an existing test file that you can add this test to. I think there's already a test file for unit testing WebView or WebFrame.
Comment on attachment 169628 [details] A WebCore version of the patch, as Adam has suggested View in context: https://bugs.webkit.org/attachment.cgi?id=169628&action=review This is much better. My only real complaint is with the test. > Source/WebCore/page/ContextMenuController.cpp:186 > - if (Page* newPage = oldPage->chrome()->createWindow(frame, request, WindowFeatures(), NavigationAction(request.resourceRequest()))) { > + if (frame->settings() && !frame->settings()->supportMultipleWindows()) > + oldPage->mainFrame()->loader()->loadFrameRequest(request, false, false, 0, 0, MaybeSendReferrer); > + else if (Page* newPage = oldPage->chrome()->createWindow(frame, request, WindowFeatures(), NavigationAction(request.resourceRequest()))) { > newPage->mainFrame()->loader()->loadFrameRequest(request, false, false, 0, 0, MaybeSendReferrer); > newPage->chrome()->show(); This can be done slightly more elegantly to avoid repeating "mainFrame()->loader()->loadFrameRequest(request, false, false, 0, 0, MaybeSendReferrer)" > Source/WebCore/page/Settings.h:708 > + bool m_supportMultipleWindows : 1; We're very inconsistent about naming these settings, but technically this should be "supports" rather than "support". I realize that's very hard to tell at this point since many, many of these settings have wacky names. > Source/WebKit/chromium/tests/WebViewTest.cpp:665 > + webView->settings()->setSupportMultipleWindows(false); > + WebCore::Frame* oldFrame = static_cast<WebFrameImpl*>(webView->mainFrame())->frame(); > + WebCore::FrameLoadRequest request(oldFrame->document()->securityOrigin()); > + WebCore::WindowFeatures features; > + bool created = true; > + WebCore::Frame* newFrame = WebCore::createWindow(oldFrame, 0, request, features, created); You're using way more WebCore types that needed in this test. Is there some reason you don't want to write a LayoutTest for this change? You can add an API to SettingsInternal.idl to twiddle the setting and then use window.open to create a new window (after telling the testRunner to allow popups). We generally prefer LayoutTests to unit tests because we can run them on every port. However, if you really want to keep this as a unit test, please use the WebKit API rather than manipulating WebCore objects directly.
(In reply to comment #9) Thanks for the review! > (From update of attachment 169628 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169628&action=review > > This is much better. My only real complaint is with the test. > > > Source/WebCore/page/ContextMenuController.cpp:186 > > - if (Page* newPage = oldPage->chrome()->createWindow(frame, request, WindowFeatures(), NavigationAction(request.resourceRequest()))) { > > + if (frame->settings() && !frame->settings()->supportMultipleWindows()) > > + oldPage->mainFrame()->loader()->loadFrameRequest(request, false, false, 0, 0, MaybeSendReferrer); > > + else if (Page* newPage = oldPage->chrome()->createWindow(frame, request, WindowFeatures(), NavigationAction(request.resourceRequest()))) { > > newPage->mainFrame()->loader()->loadFrameRequest(request, false, false, 0, 0, MaybeSendReferrer); > > newPage->chrome()->show(); > > This can be done slightly more elegantly to avoid repeating "mainFrame()->loader()->loadFrameRequest(request, false, false, 0, 0, MaybeSendReferrer)" > Done. > > Source/WebCore/page/Settings.h:708 > > + bool m_supportMultipleWindows : 1; > > We're very inconsistent about naming these settings, but technically this should be "supports" rather than "support". I realize that's very hard to tell at this point since many, many of these settings have wacky names. > Renamed. > > Source/WebKit/chromium/tests/WebViewTest.cpp:665 > > + webView->settings()->setSupportMultipleWindows(false); > > + WebCore::Frame* oldFrame = static_cast<WebFrameImpl*>(webView->mainFrame())->frame(); > > + WebCore::FrameLoadRequest request(oldFrame->document()->securityOrigin()); > > + WebCore::WindowFeatures features; > > + bool created = true; > > + WebCore::Frame* newFrame = WebCore::createWindow(oldFrame, 0, request, features, created); > > You're using way more WebCore types that needed in this test. > > Is there some reason you don't want to write a LayoutTest for this change? You can add an API to SettingsInternal.idl to twiddle the setting and then use window.open to create a new window (after telling the testRunner to allow popups). > > We generally prefer LayoutTests to unit tests because we can run them on every port. However, if you really want to keep this as a unit test, please use the WebKit API rather than manipulating WebCore objects directly. Added layout tests that pass on the Chromium platform. As we are adding this setting only for Chromium now, DRT of other ports doesn't have the setting exposed. As for the unit test--I really want to test the call to WebCore::createWindow, to test that 'created' argument is set to 'false'. I don't see any clear way how to do this using only the public API.
Created attachment 169905 [details] Comments addressed, added layout tests
Comment on attachment 169905 [details] Comments addressed, added layout tests Attachment 169905 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14483578 New failing tests: fast/dom/HTMLAnchorElement/anchor-no-multiple-windows.html fast/dom/Window/window-open-no-multiple-windows.html fast/forms/post-popup-no-multiple-windows.html
Created attachment 170495 [details] Should now pass on mac bot as well as on chromium Adam, can you please take a look?
Comment on attachment 170495 [details] Should now pass on mac bot as well as on chromium View in context: https://bugs.webkit.org/attachment.cgi?id=170495&action=review > LayoutTests/fast/dom/HTMLAnchorElement/anchor-no-multiple-windows.html:8 > + testRunner.setCanOpenWindows(true); > + testRunner.setPopupBlockingEnabled(false); Are these redundant? > Source/WebKit/chromium/tests/WebViewTest.cpp:668 > +TEST_F(WebViewTest, SupportsMultipleWindowsSetting) > +{ > + WebView* webView = FrameTestHelpers::createWebViewAndLoad("about:blank"); > + webView->settings()->setSupportsMultipleWindows(false); > + WebCore::Frame* oldFrame = static_cast<WebFrameImpl*>(webView->mainFrame())->frame(); > + WebCore::FrameLoadRequest request(oldFrame->document()->securityOrigin()); > + WebCore::WindowFeatures features; > + bool created = true; > + WebCore::Frame* newFrame = WebCore::createWindow(oldFrame, 0, request, features, created); > + EXPECT_EQ(oldFrame, newFrame); > + EXPECT_FALSE(created); > +} I would skip this test. You're reaching too much into WebCore. The LayoutTest is much better.
Thank you very much! (In reply to comment #14) > (From update of attachment 170495 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170495&action=review > > > LayoutTests/fast/dom/HTMLAnchorElement/anchor-no-multiple-windows.html:8 > > + testRunner.setCanOpenWindows(true); > > + testRunner.setPopupBlockingEnabled(false); > > Are these redundant? > Correct. setPopupBlockingEnabled(false) is only needed for the test that uses 'window.popup'. I will remove it. > > Source/WebKit/chromium/tests/WebViewTest.cpp:668 > > +TEST_F(WebViewTest, SupportsMultipleWindowsSetting) > > +{ > > + WebView* webView = FrameTestHelpers::createWebViewAndLoad("about:blank"); > > + webView->settings()->setSupportsMultipleWindows(false); > > + WebCore::Frame* oldFrame = static_cast<WebFrameImpl*>(webView->mainFrame())->frame(); > > + WebCore::FrameLoadRequest request(oldFrame->document()->securityOrigin()); > > + WebCore::WindowFeatures features; > > + bool created = true; > > + WebCore::Frame* newFrame = WebCore::createWindow(oldFrame, 0, request, features, created); > > + EXPECT_EQ(oldFrame, newFrame); > > + EXPECT_FALSE(created); > > +} > > I would skip this test. You're reaching too much into WebCore. The LayoutTest is much better. OK, I will remove it.
(In reply to comment #15) > Thank you very much! > > (In reply to comment #14) > > (From update of attachment 170495 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=170495&action=review > > > > > LayoutTests/fast/dom/HTMLAnchorElement/anchor-no-multiple-windows.html:8 > > > + testRunner.setCanOpenWindows(true); > > > + testRunner.setPopupBlockingEnabled(false); > > > > Are these redundant? > > > > Correct. setPopupBlockingEnabled(false) is only needed for the test that uses 'window.popup'. I will remove it. > *window.open > > > Source/WebKit/chromium/tests/WebViewTest.cpp:668 > > > +TEST_F(WebViewTest, SupportsMultipleWindowsSetting) > > > +{ > > > + WebView* webView = FrameTestHelpers::createWebViewAndLoad("about:blank"); > > > + webView->settings()->setSupportsMultipleWindows(false); > > > + WebCore::Frame* oldFrame = static_cast<WebFrameImpl*>(webView->mainFrame())->frame(); > > > + WebCore::FrameLoadRequest request(oldFrame->document()->securityOrigin()); > > > + WebCore::WindowFeatures features; > > > + bool created = true; > > > + WebCore::Frame* newFrame = WebCore::createWindow(oldFrame, 0, request, features, created); > > > + EXPECT_EQ(oldFrame, newFrame); > > > + EXPECT_FALSE(created); > > > +} > > > > I would skip this test. You're reaching too much into WebCore. The LayoutTest is much better. > > OK, I will remove it.
Committed r132478: <http://trac.webkit.org/changeset/132478>