[Webkit-unassigned] [Bug 53600] [GTK] Add DRT support for modal dialogs

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=53600





--- Comment #13 from Zan Dobersek <zandobersek at gmail.com>  2012-02-22 01:47:56 PST ---
(From update of attachment 126324)
View in context: https://bugs.webkit.org/attachment.cgi?id=126324&action=review

About the name in the next comment.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2746
>>      /*
> 
> Double * missing here

Noted.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2747
>> +     * WebKitWebView::run-modal-dialog
> 
> Trailing : missing here. also the name is a bit confusing, it seems this signal is to run a modal dialog, while it actually runs the toplevel window of the web view in modal mode.

Noted about the ':', I'll write about the name below.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2748
>> +     * @webView: the object which received the signal
> 
> This should be @web_view

Noted.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2751
>> +     * avoided if FALSE is returned in the signal handler. Otherwise, the
> 
> %FALSE

Noted.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2752
>> +     * @webView (or its toplevel window) should be made transient for its parent,
> 
> @web_view

Noted.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2753
>> +     * set as a modal window, and TRUE should be returned in the signal handler.
> 
> %TRUE

Noted.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2754
>> +     * After that, a loop will be created and run until the @webView is closed
> 
> @web_view

Noted.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2756
>> +     *
> 
> Signal is true_handled, it should include information about the Return value:
> 
> * Return value: %TRUE to stop handlers from being invoked for the event or                                                                                                              
>      * %FALSE to propagate the event furter

I'd like to be more verbose - %TRUE to stop handlers being invoked and run the modal dialog loop, %FALSE to state that the modal dialog should not be run and propagate the event further.

(Not this exact wording, but something similar.)

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2757
>> +     * Since: 1.7.6
> 
> This version doesn't exist, it's 1.7.90

Noted.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2759
>> +    webkit_web_view_signals[RESOURCE_LOAD_FAILED] = g_signal_new("run-modal-dialog",
> 
> RESOURCE_LOAD_FAILED -> that should be RUN_MODAL_DIALOG

Oops :)

>> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:962
>> +    GtkWindow* viewTopLevel = GTK_WINDOW(gtk_widget_get_toplevel(GTK_WIDGET(view)));
> 
> You should check that returned widget is actually a toplevel and a GdkWindow. We have a method in GtkUtilities to check this properly.

I'll put a similar check in both DRT and GtkLauncher.

>> Tools/GtkLauncher/main.c:134
>> +    gtk_window_set_modal(GTK_WINDOW(window), TRUE);
>> +    return TRUE;
> 
> Instead of duplicating this in GtkLauncher and DRT, this should be default implementation when the signal is not handled in WebView

When signal is not handled (or FALSE is returned), the current implementation interprets this as a request not to run the modal dialog loop, cancelling any further operations in that direction.

I think it wouldn't be appropriate to force a modal dialog to be shown if the signal is not handled.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.