[webkit-reviews] review denied: [Bug 26718] [Gtk] Add support for javascript windows for DRT : [Attachment 31923] updated based on xan's feedback

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied Jan Alonzo
<jmalonzo at gmail.com>'s request for review:
Bug 26718: [Gtk] Add support for javascript windows for DRT
https://bugs.webkit.org/show_bug.cgi?id=26718

Attachment 31923: updated based on xan's feedback
https://bugs.webkit.org/attachment.cgi?id=31923&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
>  void ChromeClient::closeWindowSoon()
>  {
> -    notImplemented();
> +    // Clear the page group name (because mac does!)
> +    webkit_web_view_set_group_name(m_webView, "");

Make this a FIXME. We still need to figure out how PageGroups will be useful,
or not, for us.

> +    webkit_web_view_stop_loading(m_webView);
> +
> +    g_signal_emit_by_name(m_webView, "close-web-view");
>  }

I wonder if the signal should be emitted first, and give the application the
oportunity of avoiding the closing of the window. I think this was discussed
already?

> +   
settings->setJavaScriptCanOpenWindowsAutomatically(javascriptCanOpenWindows);

The meaning of this setting is a bit nebulous to me. Without turning it on we
were able to let javascript create new windows, so why do we need it now?

> -void LayoutTestController::setPopupBlockingEnabled(bool
popupBlockingEnabled)
> +void LayoutTestController::setPopupBlockingEnabled(bool flag)
>  {
> -    // FIXME: implement
> +    WebKitWebView* view = webkit_web_frame_get_web_view(mainFrame);
> +    ASSERT(view);
> +
> +    WebKitWebSettings* settings = webkit_web_view_get_settings(view);
> +    g_object_set(G_OBJECT(settings), "javascript-can-open-windows", flag,
NULL);
> +
>  }

hrm; this looks very wrong. Enabling popup blocking by setting
javascript-can-open-windows to TRUE? I'm under the impression that there is a
misunderstanding about the meaning of this setting. I'm going to say r-.