[webkit-reviews] review requested: [Bug 76181] [GTK] FullScreen signals : [Attachment 123118] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has asked  for review:
Bug 76181: [GTK] FullScreen signals
https://bugs.webkit.org/show_bug.cgi?id=76181

Attachment 123118: Patch
https://bugs.webkit.org/attachment.cgi?id=123118&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=123118&action=review


> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:943
> +    GtkWidget* topLevelWindow =
gtk_widget_get_toplevel(GTK_WIDGET(m_webView));
> +    if (gtk_widget_is_toplevel(topLevelWindow))
> +	   g_signal_connect(topLevelWindow, "key-press-event",
G_CALLBACK(onFullscreenGtkKeyPressEvent), this);
> +
> +    gtk_window_fullscreen(GTK_WINDOW(topLevelWindow));

You should check topLevelWindow is actually a GTK_WINDOW (and not a
GtkOffscreenWindow, see bug #76911)

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:952
> +    gboolean returnValue;
> +    g_signal_emit_by_name(m_webView, "leaving-fullscreen", &returnValue);
> +

You should return here if returnValue is TRUE.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:955
> +    GtkWidget* topLevelWindow =
gtk_widget_get_toplevel(GTK_WIDGET(m_webView));
> +    if (gtk_widget_is_toplevel(topLevelWindow))
> +	   g_signal_handlers_disconnect_by_func(topLevelWindow,
reinterpret_cast<void*>(onFullscreenGtkKeyPressEvent), this);

You should check topLevelWindow is actually a GTK_WINDOW (and not a
GtkOffscreenWindow, see bug #76911)

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:959
> +    if (gtk_widget_is_toplevel(topLevelWindow))
> +	   gtk_window_unfullscreen(GTK_WINDOW(topLevelWindow));

if topLevelWindow is not a topLevel you shouldn't get here, I guess.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1266
> +static gboolean webkit_web_view_real_entering_fullscreen(WebKitWebView*
webView)
> +{
> +    return TRUE;
> +}
> +
> +static gboolean webkit_web_view_real_leaving_fullscreen(WebKitWebView*
webView)
> +{
> +    return TRUE;
> +}

Default implementation of these happen after emitting the signal, so they
should return FALSE or just don't add them, since FALSE is returned by default.


> Source/WebKit/gtk/webkit/webkitwebview.cpp:2634
> +	* its top level window. This signal can be used by client code to
> +	* request permission to the user prior doing the full screen
> +	* transition and eventually prepare the top-level window
> +	* (e.g. hide some widgets that would otherwise be part of the
> +	* full screen window).

It can be used to handle the fullscreen yourself, I guess. You know what your
toplevel window is, so you can do gtk_window_fullscreen by yourself, or even
for preventing that window is fullscreened. I would simply say that the default
implementation of the signals is not handled is to full screen the current
toplevel window of web_view.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2644
> +			    (GSignalFlags) G_SIGNAL_RUN_LAST,

I don't think you need the cast

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2646
> +			    g_signal_accumulator_first_wins, 0,

Why first_wins instead of true_handled?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2658
> +	* Emitted when the WebView is about to restore its top level
> +	* window out of its full screen state. This signal can be used by
> +	* client code to restore widgets hidden during the
> +	* entering-fullscreen stage for instance.

or to handling the unfullscreen by yourself, I guess again.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2660
> +
> +	* Returns: %TRUE to stop other handlers from being invoked for the
event.

There's an * missing there

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2668
> +			    (GSignalFlags) G_SIGNAL_RUN_LAST,

I don't think you need the cast

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2670
> +			    g_signal_accumulator_first_wins, 0,

Why first_wins instead of true_handled?

> Tools/GtkLauncher/main.c:130
> +    if (GTK_IS_WIDGET(dialog))
> +	   g_signal_emit_by_name(GTK_DIALOG(dialog), "close");
> +    return FALSE;

why not just destroy the dialog?

> Tools/GtkLauncher/main.c:138
> +	   GtkWidget *topLevelWindow =
gtk_widget_get_toplevel(GTK_WIDGET(webView));

You should check topLevelWindow is actually a GTK_WINDOW (and not a
GtkOffscreenWindow, see bug #76911)

> Tools/GtkLauncher/main.c:139
> +	   GtkWidget *dialog =
gtk_message_dialog_new(GTK_WINDOW(topLevelWindow),

topLevelWindow might be null, use topLevelWindow ? GTK_WINDOW(topLevelWindow) :
NULL

> Tools/GtkLauncher/main.c:146
> +	   g_signal_connect_swapped(dialog, "response",
G_CALLBACK(gtk_widget_destroy), dialog);
> +	   g_timeout_add(1500, (GSourceFunc)
webViewFullscreenMessageWindowClose, dialog);
> +	   gtk_dialog_run(GTK_DIALOG(dialog));

This is blocking and dialog is modal, so you don't need to connect to response
nor to add the timeout, just destroy the dialog after dialog_run()

> Tools/GtkLauncher/main.c:148
> +    return TRUE;

You are only handling the signal when GDK_WINDOW_STATE_FULLSCREEN is
fullscreen, so you should return FALSE here.

> Tools/GtkLauncher/main.c:160
> +	   gtk_widget_show_all(widget);

Why show_all? you are iterating all widgets, I think you should use show()
instead of show_all()

> Tools/GtkLauncher/main.c:176
> +	   g_signal_connect(topLevelWindow, "window-state-event",
G_CALLBACK(webViewWindowStateEvent), webView);

I wonder whether the default impl for entering/leaving fullscreen should happen
in the object method handler instead of after emitting the signal. That way,
you could connect to entering-fullscreen with g_signal_connect to show the
confirm dialog, and g_signal_connect_after to show the information dialog.

> Tools/GtkLauncher/main.c:178
> +	   gtk_widget_destroy(GTK_WIDGET(dialog));

No need to cast, dialog is a GtkWidget

> Tools/GtkLauncher/main.c:179
> +	   return TRUE;

I think you actually want to return FALSE to allow the default impl to
fullscreen the window

> Tools/GtkLauncher/main.c:181
> +    gtk_widget_destroy(GTK_WIDGET(dialog));

Ditto

> Tools/GtkLauncher/main.c:190
> +    return TRUE;

This should be FALSE too, I guess.