[webkit-reviews] review denied: [Bug 99716] [Chromium] Add supportMultipleWindows setting for Android : [Attachment 169409] Fixed style errors

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Mikhail Naganov
<mnaganov at chromium.org>'s request for review:
Bug 99716: [Chromium] Add supportMultipleWindows setting for Android
https://bugs.webkit.org/show_bug.cgi?id=99716

Attachment 169409: Fixed style errors
https://bugs.webkit.org/attachment.cgi?id=169409&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.