Bug 76181 – [GTK] FullScreen signals

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

WebKit1 pendant of bug 76166

[Philippe Normand]

Created attachment 122246 [details]
WebKit1 Fullscreen signals

[Philippe Normand]

Created attachment 122247 [details]
WebKit1 Fullscreen signals

[WebKit Review Bot]

Attachment 122246 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

Tools/GtkLauncher/main.c:129:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/GtkLauncher/main.c:139:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/GtkLauncher/main.c:144:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/GtkLauncher/main.c:145:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/GtkLauncher/main.c:146:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/GtkLauncher/main.c:151:  hide_widget is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Tools/GtkLauncher/main.c:154:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/GtkLauncher/main.c:157:  show_widget is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Tools/GtkLauncher/main.c:160:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/GtkLauncher/main.c:167:  Declaration has space between * and variable name in GtkWidget* topLevelWindow  [whitespace/declaration] [3]
Tools/GtkLauncher/main.c:169:  Declaration has space between * and variable name in GtkWidget* dialog  [whitespace/declaration] [3]
Tools/GtkLauncher/main.c:169:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/GtkLauncher/main.c:174:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/GtkLauncher/main.c:177:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/GtkLauncher/main.c:178:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/GtkLauncher/main.c:181:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/GtkLauncher/main.c:187:  Declaration has space between * and variable name in GtkWidget* topLevelWindow  [whitespace/declaration] [3]
Tools/GtkLauncher/main.c:189:  Extra space before ( in function call  [whitespace/parens] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebview.c"
Total errors found: 18 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xan Lopez]

Comment on attachment 122247 [details]
WebKit1 Fullscreen signals

View in context: https://bugs.webkit.org/attachment.cgi?id=122247&action=review

An overall comment, I think it would fit using the policy action mechanism here.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:932
> +        return;

Shouldn't this be if (returnValue) ? Otherwise you are doing nothing if the signal is not handled? Maybe I'm on crack.

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

Shouldn't this also allow to cancel the action?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2622
> +    /**

Missing Since: in the signals?

> Source/WebKit/gtk/webkit/webkitwebview.h:180
> +    gboolean                   (* leaving_fullscreen) (WebKitWebView   *web_view);

You need to remove padding from the class, otherwise you break the ABI.

[Philippe Normand]

(In reply to comment #4)
> (From update of attachment 122247 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122247&action=review
> 
> An overall comment, I think it would fit using the policy action mechanism here.
> 
> > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:932
> > +        return;
> 
> Shouldn't this be if (returnValue) ? Otherwise you are doing nothing if the signal is not handled? Maybe I'm on crack.
> 

I don't think so. returnValue would be FALSE here if the signal wasn't handled properly.

> > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:951
> > +    g_signal_emit_by_name(m_webView, "leaving-fullscreen", &returnValue);
> 
> Shouldn't this also allow to cancel the action?
> 

I haven't found a use-case for this but yeah, I'd be Ok with allowing to cancel the action.

> > Source/WebKit/gtk/webkit/webkitwebview.cpp:2622
> > +    /**
> 
> Missing Since: in the signals?
> 
> > Source/WebKit/gtk/webkit/webkitwebview.h:180
> > +    gboolean                   (* leaving_fullscreen) (WebKitWebView   *web_view);
> 
> You need to remove padding from the class, otherwise you break the ABI.

Ok!

[Philippe Normand]

Created attachment 123118 [details]
Patch

[Xan Lopez]

Comment on attachment 123118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123118&action=review

r- at least for the accumulators used.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:932
> +        return;

If the handler handled the signal it will return TRUE, so I still think if you need to flip the boolean here. Perhaps this is not working correctly because you are using the wrong accumulator.

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

You need to check the return value and do nothing if it's TRUE, per what the documentation says.

> Source/WebKit/gtk/tests/testwebview.c:478
> +    g_test_add_data_func("/webkit/webview/fullscreen", (gconstpointer) TRUE, test_webkit_web_view_fullscreen);

No spaces before the cast?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2638
> +     *

FALSE to block the event? Usually TRUE = hanlded ("block the event"), FALSE = continue emission. So I think you are saying the same thing twice basically.

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

I think you want true_handled, not first_wins.

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

Same.

[Carlos Garcia Campos]

Comment on attachment 123118 [details]
Patch

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.

[Philippe Normand]

Comment on attachment 123118 [details]
Patch

Needs work.

[Philippe Normand]

(In reply to comment #8)
> > 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()
> 

I need that code to close the popup even if the user doesn't explicit close it.

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

Hum, I'll experiment with that!

[Philippe Normand]

(In reply to comment #10)
> > 
> > > 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.
> > 
> 
> Hum, I'll experiment with that!

So you meant doing gtk_window_fullscreen() in webkit_web_view_real_entering_fullscreen() and the opposite in webkit_web_view_real_leaving_fullscreen()?

If so, that doesn't work and looks awkward, actually. Also if the UA blocks the entering-fullscreen signal the element remains zoomed-out in the un-fullscreened window.

[Philippe Normand]

Created attachment 125110 [details]
Patch

[WebKit Review Bot]

Attachment 125110 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:935:  Missing space before ( in if(  [whitespace/parens] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebview.c"
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

(In reply to comment #13)
> Attachment 125110 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
> 
> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:935:  Missing space before ( in if(  [whitespace/parens] [5]
> WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebview.c"
> Total errors found: 1 in 8 files
> 

Carlos and Martin, can you please review this patch when you find some time? :)

[Martin Robinson]

Comment on attachment 125110 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125110&action=review

Looks good to me, but you need to find one more reviewer to approve the API.

>> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:935
>> +    if(!widgetIsOnscreenToplevelWindow(window))
> 
> Missing space before ( in if(  [whitespace/parens] [5]

Please fix this before landing.

> Source/WebKit/gtk/tests/testwebview.c:368
> +    // create and send the event

You can just omit this comment or, if not, be sure to use a capital letter and a period.

> Source/WebKit/gtk/tests/testwebview.c:476
> +    g_test_add_data_func("/webkit/webview/fullscreen", (gconstpointer)FALSE, test_webkit_web_view_fullscreen);
> +    g_test_add_data_func("/webkit/webview/fullscreen-blocked", (gconstpointer)TRUE, test_webkit_web_view_fullscreen);

You should use GINT_TO_POINTER here instead of casting yourself, I think.

[Philippe Normand]

Adding more reviewers then :)

[Philippe Normand]

I'll update the patch soon but it'd be nice to have feedback about the API nonetheless. Thanks!

[Philippe Normand]

Created attachment 127550 [details]
Rebased Patch

Also addressing review comments.

[Gustavo Noronha (kov)]

Comment on attachment 127550 [details]
Rebased Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127550&action=review

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

The signal seems sensible to me, but I think we should take advantage of the DOM bindings and also provide the element that is being fullscreened.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:984
> +    gtk_window_fullscreen(GTK_WINDOW(window));

I'm not sure about this. I guess it makes sense to fullscreen by default, but I wonder if it may be problematic for non-browser use cases, to do it by default if the page requests. hmm

[Philippe Normand]

(In reply to comment #19)
> (From update of attachment 127550 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127550&action=review
> 
> > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:970
> > +    g_signal_emit_by_name(m_webView, "entering-fullscreen", &returnValue);
> 
> The signal seems sensible to me, but I think we should take advantage of the DOM bindings and also provide the element that is being fullscreened.
> 

Embedded in the signal data? Hum I guess that'd be doable.

> > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:984
> > +    gtk_window_fullscreen(GTK_WINDOW(window));
> 
> I'm not sure about this. I guess it makes sense to fullscreen by default, but I wonder if it may be problematic for non-browser use cases, to do it by default if the page requests. hmm

Well, this whole feature is protected by a web-setting which defaults to FALSE, IIRC.

[Gustavo Noronha (kov)]

(In reply to comment #20)
> > > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:970
> > > +    g_signal_emit_by_name(m_webView, "entering-fullscreen", &returnValue);
> > 
> > The signal seems sensible to me, but I think we should take advantage of the DOM bindings and also provide the element that is being fullscreened.
> > 
> 
> Embedded in the signal data? Hum I guess that'd be doable.

As a signal parameter, yeah =)

> > I'm not sure about this. I guess it makes sense to fullscreen by default, but I wonder if it may be problematic for non-browser use cases, to do it by default if the page requests. hmm
> 
> Well, this whole feature is protected by a web-setting which defaults to FALSE, IIRC.

OK, +1 from me on this then =)

[Philippe Normand]

(In reply to comment #21)
> (In reply to comment #20)
> > > > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:970
> > > > +    g_signal_emit_by_name(m_webView, "entering-fullscreen", &returnValue);
> > > 
> > > The signal seems sensible to me, but I think we should take advantage of the DOM bindings and also provide the element that is being fullscreened.
> > > 
> > 
> > Embedded in the signal data? Hum I guess that'd be doable.
> 
> As a signal parameter, yeah =)
> 

Alright, I'll send a new patch soon then, if I manage to get that working :)

[Philippe Normand]

Created attachment 127827 [details]
Patch

Now with DOM bindings support!

[Philippe Normand]

(In reply to comment #23)
> Created an attachment (id=127827) [details]
> Patch
> 
> Now with DOM bindings support!

Can we get this reviewed please? Gustavo and Martin approved the API, we only need a formal r+ now :)

[Martin Robinson]

Comment on attachment 127827 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127827&action=review

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2688
> +     * Since: 1.7.6

This should probably be 1.9.0 now, unless you want to merge this into stable then it should be 1.8.0.

[Philippe Normand]

Committed r108522: <http://trac.webkit.org/changeset/108522>

[Philippe Normand]

Reverted r108522 for reason:

Broke 4 fullscreen tests on GTK.

Committed r108527: <http://trac.webkit.org/changeset/108527>

[Philippe Normand]

(In reply to comment #27)
> Reverted r108522 for reason:
> 
> Broke 4 fullscreen tests on GTK.
> 
> Committed r108527: <http://trac.webkit.org/changeset/108527>

The issue was that ChromeClient::enterFullScreenForElement was stealing the Element reference.

[Philippe Normand]

Committed r108620: <http://trac.webkit.org/changeset/108620>