[webkit-reviews] review denied: [Bug 99716] [Chromium] Add supportMultipleWindows setting for Android : [Attachment 169628] A WebCore version of the patch, as Adam has suggested

[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 169628: A WebCore version of the patch, as Adam has suggested
https://bugs.webkit.org/attachment.cgi?id=169628&action=review

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