Bug 99716 – [Chromium] Add supportMultipleWindows setting for Android

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Mikhail Naganov]

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.

[Mikhail Naganov]

Created attachment 169408 [details]
Implementation

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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.

[Mikhail Naganov]

Created attachment 169409 [details]
Fixed style errors

[Mikhail Naganov]

Chromium change that makes use of the setting: https://codereview.chromium.org/11192057/

[Adam Barth]

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.

[Mikhail Naganov]

Created attachment 169628 [details]
A WebCore version of the patch, as Adam has suggested

[Mikhail Naganov]

(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.

[Adam Barth]

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.

[Mikhail Naganov]

(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.

[Mikhail Naganov]

Created attachment 169905 [details]
Comments addressed, added layout tests

[Build Bot]

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

[Mikhail Naganov]

Created attachment 170495 [details]
Should now pass on mac bot as well as on chromium

Adam, can you please take a look?

[Adam Barth]

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.

[Mikhail Naganov]

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.

[Mikhail Naganov]

(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.

[Mikhail Naganov]

Committed r132478: <http://trac.webkit.org/changeset/132478>