Bug 26718 – [Gtk] Add support for javascript windows for DRT

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Jan Alonzo]

The Gtk port's DRT need to support javascript windows so we can run, at least, some of the http/tests/security tests, and possibly tests that use LayoutTestController::canOpenWindows.

[Jan Alonzo]

Created attachment 31854 [details]
Gtk DRT JS windows support

This patch adds a new property: javascript-can-open-windows and a new signal close-web-view to tell the clients that the window will be closing soon. This signal is also the other half of create-web-view which is already an API in WebKitGtk.

These two features are needed to get JS windows support in Gtk DRT. I've also unskip the tests that now run because of this (there's probably more but this set will do for now).

[Xan Lopez]

Comment on attachment 31854 [details]
Gtk DRT JS windows support

>          Reviewed by Holger Freyther.
> diff --git a/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp b/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp
> index 5023d5c..a08e4f9 100644
> --- a/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp
> +++ b/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp
> @@ -215,7 +215,10 @@ void ChromeClient::setResizable(bool)
>  
>  void ChromeClient::closeWindowSoon()
>  {
> -    notImplemented();
> +    webkit_web_view_set_group_name(m_webView, "");

What's this for?

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

In some cases it's good to have a way to ask the user if he wants to cancel the closing action (ie, return TRUE to prevent closing). For example, the case were forms are modified and not submitted when closing a window can be handled in two ways I guess: 'close-web-view', call view_has_modified_forms, if it has show a dialog asking, return what the user wants to close-request. Another option, I guess, is to just have higher level APIs for this ('close-with-modified-forms' signal).

Not sure what makes most sense here, but just commenting :)


> +    /**
> +    * WebKitWebSettings:javascript-can-open-windows
> +    *
> +    * Whether JavaScript can open windows automatically.
> +    *
> +    * Since 1.1.11
> +    */
> +    g_object_class_install_property(gobject_class,
> +                                    PROP_JAVASCRIPT_CAN_OPEN_WINDOWS,
> +                                    g_param_spec_boolean("javascript-can-open-windows",
> +                                                         _("JavaScript can open windows automatically"),
> +                                                         _("Whether JavaScript can open windows automatically"),
> +                                                         FALSE,

Mmm, are we sure FALSE is the right default?

>      /**
> +     * WebKitWebView::close-web-view:
> +     * @web_view: the object on which the signal is emitted
> +     *
> +     * Emitted when closing a window is requested.

So, I guess you mean view here and not window? Is this signal emitted when the widget is destroyed?


> @@ -132,8 +136,10 @@ static gchar* dumpFramesAsText(WebKitWebFrame* frame)
>  
>      if (gLayoutTestController->dumpChildFramesAsText()) {
>          GSList* children = webkit_web_frame_get_children(frame);
> -        for (GSList* child = children; child; child = g_slist_next(child))
> -           appendString(result, dumpFramesAsText((WebKitWebFrame*)children->data));
> +        for (unsigned i = 0; i < g_slist_length(children); ++i) {
> +            WebKitWebFrame* childFrame = static_cast<WebKitWebFrame*>(g_slist_nth_data(children, i));
> +            appendString(result, dumpFramesAsText(childFrame));
> +        }

Mmm, why this?

> +
> +    // Also check if we still have opened webViews and free them.
> +    if (gLayoutTestController->closeRemainingWindowsWhenComplete() || webViewList) {

Shouldn't that be an && instead of ||? Or am I missunderstaning something.

> +        while (webViewList) {
> +            g_object_unref(WEBKIT_WEB_VIEW(webViewList->data));
> +            webViewList = g_slist_next(webViewList);
> +        }
> +        g_slist_free(webViewList);
> +        webViewList = 0;
> +    }
> +

> +static WebKitWebView* webViewCreate(WebKitWebView*, WebKitWebFrame*);
> +
> +static WebKitWebView* createWebView()
> +{
> +    WebKitWebView* view = WEBKIT_WEB_VIEW(webkit_web_view_new());
> +    webkit_web_view_set_group_name(view, "org.webkit.gtk.DumpRenderTree");
> +
> +    g_signal_connect(G_OBJECT(view), "load-started", G_CALLBACK(webViewLoadStarted), 0);
> +    g_signal_connect(G_OBJECT(view), "load-finished", G_CALLBACK(webViewLoadFinished), 0);
> +    g_signal_connect(G_OBJECT(view), "window-object-cleared", G_CALLBACK(webViewWindowObjectCleared), 0);
> +    g_signal_connect(G_OBJECT(view), "console-message", G_CALLBACK(webViewConsoleMessage), 0);
> +    g_signal_connect(G_OBJECT(view), "script-alert", G_CALLBACK(webViewScriptAlert), 0);
> +    g_signal_connect(G_OBJECT(view), "script-prompt", G_CALLBACK(webViewScriptPrompt), 0);
> +    g_signal_connect(G_OBJECT(view), "script-confirm", G_CALLBACK(webViewScriptConfirm), 0);
> +    g_signal_connect(G_OBJECT(view), "title-changed", G_CALLBACK(webViewTitleChanged), 0);
> +    g_signal_connect(G_OBJECT(view), "navigation-policy-decision-requested", G_CALLBACK(webViewNavigationPolicyDecisionRequested), 0);
> +    g_signal_connect(G_OBJECT(view), "status-bar-text-changed", G_CALLBACK(webViewStatusBarTextChanged), 0);
> +    g_signal_connect(G_OBJECT(view), "create-web-view", G_CALLBACK(webViewCreate), 0);
> +    g_signal_connect(G_OBJECT(view), "close-web-view", G_CALLBACK(webViewClose), 0);

While you are at it, g_signal_connect does not take a GObject as first parameter, so no need to cast. And you could use g_object_connect.

[Jan Alonzo]

> >  
> >  void ChromeClient::closeWindowSoon()
> >  {
> > -    notImplemented();
> > +    webkit_web_view_set_group_name(m_webView, "");
> 
> What's this for?



> 
> > +    webkit_web_view_stop_loading(m_webView);
> > +
> > +    g_signal_emit_by_name(m_webView, "close-web-view");
> >  }
> 
> In some cases it's good to have a way to ask the user if he wants to cancel the
> closing action (ie, return TRUE to prevent closing). For example, the case were
> forms are modified and not submitted when closing a window can be handled in
> two ways I guess: 'close-web-view', call view_has_modified_forms, if it has
> show a dialog asking, return what the user wants to close-request. Another
> option, I guess, is to just have higher level APIs for this
> ('close-with-modified-forms' signal).
>
> Not sure what makes most sense here, but just commenting :)
>

I'll add a check if it's handled to give clients a way to do whatever before closing. Thanks :)

> 
> > +    /**
> > +    * WebKitWebSettings:javascript-can-open-windows
> > +    *
> > +    * Whether JavaScript can open windows automatically.
> > +    *
> > +    * Since 1.1.11
> > +    */
> > +    g_object_class_install_property(gobject_class,
> > +                                    PROP_JAVASCRIPT_CAN_OPEN_WINDOWS,
> > +                                    g_param_spec_boolean("javascript-can-open-windows",
> > +                                                         _("JavaScript can open windows automatically"),
> > +                                                         _("Whether JavaScript can open windows automatically"),
> > +                                                         FALSE,
> 
> Mmm, are we sure FALSE is the right default?

It's defaulted to FALSE in WebCore::Settings that's why it's also FALSE here. Maybe we should set it to TRUE and let applications decide if they don't really want it.

> 
> >      /**
> > +     * WebKitWebView::close-web-view:
> > +     * @web_view: the object on which the signal is emitted
> > +     *
> > +     * Emitted when closing a window is requested.
> 
> So, I guess you mean view here and not window? Is this signal emitted when the
> widget is destroyed?

Should be view. This gets emitted when window.close() is called.

> 
> > @@ -132,8 +136,10 @@ static gchar* dumpFramesAsText(WebKitWebFrame* frame)
> >  
> >      if (gLayoutTestController->dumpChildFramesAsText()) {
> >          GSList* children = webkit_web_frame_get_children(frame);
> > -        for (GSList* child = children; child; child = g_slist_next(child))
> > -           appendString(result, dumpFramesAsText((WebKitWebFrame*)children->data));
> > +        for (unsigned i = 0; i < g_slist_length(children); ++i) {
> > +            WebKitWebFrame* childFrame = static_cast<WebKitWebFrame*>(g_slist_nth_data(children, i));
> > +            appendString(result, dumpFramesAsText(childFrame));
> > +        }
> 
> Mmm, why this?

This is a fix to get all the unskipped tests in this patch work. This seemed to fix it, but I think I know the reason why. I'll update.

> > +
> > +    // Also check if we still have opened webViews and free them.
> > +    if (gLayoutTestController->closeRemainingWindowsWhenComplete() || webViewList) {
> 
> Shouldn't that be an && instead of ||? Or am I missunderstaning something.

No. The webViewList check is just so we cleanup properly. Some tests may have forgotten to call closeRemainingWindowsWhenComplete so this just makes sure we free those WebViews.

> 
> > +        while (webViewList) {
> > +            g_object_unref(WEBKIT_WEB_VIEW(webViewList->data));
> > +            webViewList = g_slist_next(webViewList);
> > +        }
> > +        g_slist_free(webViewList);
> > +        webViewList = 0;
> > +    }
> > +
> 
> > +static WebKitWebView* webViewCreate(WebKitWebView*, WebKitWebFrame*);
> > +
> > +static WebKitWebView* createWebView()
> > +{
> > +    WebKitWebView* view = WEBKIT_WEB_VIEW(webkit_web_view_new());
> > +    webkit_web_view_set_group_name(view, "org.webkit.gtk.DumpRenderTree");
> > +
> > +    g_signal_connect(G_OBJECT(view), "load-started", G_CALLBACK(webViewLoadStarted), 0);
> > +    g_signal_connect(G_OBJECT(view), "load-finished", G_CALLBACK(webViewLoadFinished), 0);
> > +    g_signal_connect(G_OBJECT(view), "window-object-cleared", G_CALLBACK(webViewWindowObjectCleared), 0);
> > +    g_signal_connect(G_OBJECT(view), "console-message", G_CALLBACK(webViewConsoleMessage), 0);
> > +    g_signal_connect(G_OBJECT(view), "script-alert", G_CALLBACK(webViewScriptAlert), 0);
> > +    g_signal_connect(G_OBJECT(view), "script-prompt", G_CALLBACK(webViewScriptPrompt), 0);
> > +    g_signal_connect(G_OBJECT(view), "script-confirm", G_CALLBACK(webViewScriptConfirm), 0);
> > +    g_signal_connect(G_OBJECT(view), "title-changed", G_CALLBACK(webViewTitleChanged), 0);
> > +    g_signal_connect(G_OBJECT(view), "navigation-policy-decision-requested", G_CALLBACK(webViewNavigationPolicyDecisionRequested), 0);
> > +    g_signal_connect(G_OBJECT(view), "status-bar-text-changed", G_CALLBACK(webViewStatusBarTextChanged), 0);
> > +    g_signal_connect(G_OBJECT(view), "create-web-view", G_CALLBACK(webViewCreate), 0);
> > +    g_signal_connect(G_OBJECT(view), "close-web-view", G_CALLBACK(webViewClose), 0);
> 
> While you are at it, g_signal_connect does not take a GObject as first
> parameter, so no need to cast. And you could use g_object_connect.

Ok, thanks for that. I'll post an update.
> 

[Jan Alonzo]

Created attachment 31923 [details]
updated based on xan's feedback

[Jan Alonzo]

(In reply to comment #3)
> > >  
> > >  void ChromeClient::closeWindowSoon()
> > >  {
> > > -    notImplemented();
> > > +    webkit_web_view_set_group_name(m_webView, "");
> > 
> > What's this for?

We're closing the page (and maybe finalizing the WebView) so might be a good idea to set the group name to empty. Mac does it too, that's why I added it there as well :)

> > > +    webkit_web_view_stop_loading(m_webView);
> > > +
> > > +    g_signal_emit_by_name(m_webView, "close-web-view");
> > >  }
> > 
> > In some cases it's good to have a way to ask the user if he wants to cancel the
> > closing action (ie, return TRUE to prevent closing). For example, the case were
> > forms are modified and not submitted when closing a window can be handled in
> > two ways I guess: 'close-web-view', call view_has_modified_forms, if it has
> > show a dialog asking, return what the user wants to close-request. Another
> > option, I guess, is to just have higher level APIs for this
> > ('close-with-modified-forms' signal).
> >
> > Not sure what makes most sense here, but just commenting :)
> >
> 
> I'll add a check if it's handled to give clients a way to do whatever before
> closing. Thanks :)

I've thought about it, but I don't think it makes sense. This gets emitted when JavaScript is used to close the window, not when a user closes a window/webview. If a form was filled out and unsubmitted, and the user suddenly wants to move away from the page, I think a better mechanism should handle that. Maybe popup a dialog on delete-event, but we don't have access to form data atm so I'm not sure how to better handle this.

> > > @@ -132,8 +136,10 @@ static gchar* dumpFramesAsText(WebKitWebFrame* frame)
> > >  
> > >      if (gLayoutTestController->dumpChildFramesAsText()) {
> > >          GSList* children = webkit_web_frame_get_children(frame);
> > > -        for (GSList* child = children; child; child = g_slist_next(child))
> > > -           appendString(result, dumpFramesAsText((WebKitWebFrame*)children->data));
> > > +        for (unsigned i = 0; i < g_slist_length(children); ++i) {
> > > +            WebKitWebFrame* childFrame = static_cast<WebKitWebFrame*>(g_slist_nth_data(children, i));
> > > +            appendString(result, dumpFramesAsText(childFrame));
> > > +        }
> > 
> > Mmm, why this?
> 
> This is a fix to get all the unskipped tests in this patch work. This seemed to
> fix it, but I think I know the reason why. I'll update.

We were using children instead of child here. Fix was included in the patch.

> > > +static WebKitWebView* createWebView()
> > > +{
> > > +    WebKitWebView* view = WEBKIT_WEB_VIEW(webkit_web_view_new());
> > > +    webkit_web_view_set_group_name(view, "org.webkit.gtk.DumpRenderTree");
> > > +
> > > +    g_signal_connect(G_OBJECT(view), "load-started", G_CALLBACK(webViewLoadStarted), 0);
> > > +    g_signal_connect(G_OBJECT(view), "load-finished", G_CALLBACK(webViewLoadFinished), 0);
> > > +    g_signal_connect(G_OBJECT(view), "window-object-cleared", G_CALLBACK(webViewWindowObjectCleared), 0);
> > > +    g_signal_connect(G_OBJECT(view), "console-message", G_CALLBACK(webViewConsoleMessage), 0);
> > > +    g_signal_connect(G_OBJECT(view), "script-alert", G_CALLBACK(webViewScriptAlert), 0);
> > > +    g_signal_connect(G_OBJECT(view), "script-prompt", G_CALLBACK(webViewScriptPrompt), 0);
> > > +    g_signal_connect(G_OBJECT(view), "script-confirm", G_CALLBACK(webViewScriptConfirm), 0);
> > > +    g_signal_connect(G_OBJECT(view), "title-changed", G_CALLBACK(webViewTitleChanged), 0);
> > > +    g_signal_connect(G_OBJECT(view), "navigation-policy-decision-requested", G_CALLBACK(webViewNavigationPolicyDecisionRequested), 0);
> > > +    g_signal_connect(G_OBJECT(view), "status-bar-text-changed", G_CALLBACK(webViewStatusBarTextChanged), 0);
> > > +    g_signal_connect(G_OBJECT(view), "create-web-view", G_CALLBACK(webViewCreate), 0);
> > > +    g_signal_connect(G_OBJECT(view), "close-web-view", G_CALLBACK(webViewClose), 0);
> > 
> > While you are at it, g_signal_connect does not take a GObject as first
> > parameter, so no need to cast. And you could use g_object_connect.
> 
> Ok, thanks for that. I'll post an update.

Done. :)

[Gustavo Noronha (kov)]

Comment on attachment 31923 [details]
updated based on xan's feedback

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

[Jan Alonzo]

Created attachment 32101 [details]
patch addressing issues raised by Gustavo

[Jan Alonzo]

Created attachment 32102 [details]
updated to fix the popup blocking flag

[Jan Alonzo]

(In reply to comment #6)
> (From update of attachment 31923 [details] [review])
> >  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.

I've added a FIXME here. I've added a comment where I've used this function in DRT and why it's used there.

> 
> > +    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?

I've added my own reasoning in comment #5. But with the updated patch, I've updated this to let the application handle the signal.

> 
> > +    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?

As discussed in IRC, this is for opening windows that aren't initiated by a user action.

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

flag should've been !flag. Fixed.

Thanks.

[Gustavo Noronha (kov)]

Comment on attachment 32102 [details]
updated to fix the popup blocking flag

> +    /**
> +    * WebKitWebSettings:javascript-can-open-windows
> +    *
> +    * Whether JavaScript can open popup windows automatically without user
> +    * intervention.

I think I would like to have the name of that setting contain -automatically, to make it more obvious. So, javascript-can-open-windows-automatically. I know it gets a bit too long, but there's no way someone would confuse what it does. I'm good if there's a consensus on keeping it this way, though.

> +    gboolean                   (* close_web_view)         (WebKitWebView* web_view);
> +

Probably need to remove one of the paddings because of this new class method. Or should we take the opportunity, break ABI, and also rename the library? =P

Take one out if you land before a decision is reached on breaking ABI, though.

Except for those two comments, r=me, with or without the rename, depending on what you and xan think. I'm leaving the official r+ for Xan.

[Xan Lopez]

+++ b/WebKit/gtk/webkit/webkitwebview.cpp
@@ -117,6 +117,7 @@ enum {
     MIME_TYPE_POLICY_DECISION_REQUESTED,
     CREATE_WEB_VIEW,
     WEB_VIEW_READY,
+    CLOSE_WEB_VIEW,
     WINDOW_OBJECT_CLEARED,
     LOAD_STARTED,
     LOAD_COMMITTED,

Mmm, I believe this is an ABI break too.

I agree about adding the 'automatically' bit to the name. Not sure about the renaming the library bit, although I guess that if we want to do it we should do it ASAP.

[Xan Lopez]

(In reply to comment #11)
> +++ b/WebKit/gtk/webkit/webkitwebview.cpp
> @@ -117,6 +117,7 @@ enum {
>      MIME_TYPE_POLICY_DECISION_REQUESTED,
>      CREATE_WEB_VIEW,
>      WEB_VIEW_READY,
> +    CLOSE_WEB_VIEW,
>      WINDOW_OBJECT_CLEARED,
>      LOAD_STARTED,
>      LOAD_COMMITTED,
> 
> Mmm, I believe this is an ABI break too.

Nevermind, kov pointed on IRC that it's not, since it's only used internally.

[Gustavo Noronha (kov)]

Comment on attachment 32102 [details]
updated to fix the popup blocking flag

So, since Xan commented. r=me with the name change, the removal of one of the class method placeholders, and please reposition the signal in the enum.

[Jan Alonzo]

(In reply to comment #13)
> (From update of attachment 32102 [details])
> So, since Xan commented. r=me with the name change, the removal of one of the
> class method placeholders, and please reposition the signal in the enum.

Thanks. Landed in http://trac.webkit.org/changeset/45801