[Webkit-unassigned] [Bug 76181] [GTK] FullScreen signals

[bugzilla-daemon at webkit.org]

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


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #123118|review-                     |review?
               Flag|                            |




--- Comment #8 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-02-02 02:15:47 PST ---
(From update of attachment 123118)
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.

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