Bug 76166 – [WK2][GTK] FullScreen signals

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

We need a entering-fullscreen and a leaving-fullscreen signal.

[Philippe Normand]

Created attachment 122211 [details]
Fullscreen Signals

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Philippe Normand]

Created attachment 122227 [details]
Fullscreen Signals

[Philippe Normand]

Created attachment 124730 [details]
FullScreen signals

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   2366cac..7140fe4  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 106359 = 2366cac6c66ce637903152f5fc58cd98b5686950
r106360 = 7140fe4a50c1f3a4ce373c6fcee4f0968c84e929
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


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

[Martin Robinson]

Comment on attachment 124730 [details]
FullScreen signals

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:645
> +    signals[ENTERING_FULLSCREEN] =
> +            g_signal_new("entering-fullscreen",
> +                         G_TYPE_FROM_CLASS(webViewClass),
> +                         G_SIGNAL_RUN_LAST,
> +                         G_STRUCT_OFFSET(WebKitWebViewClass, entering_fullscreen),
> +                         g_signal_accumulator_first_wins, 0,
> +                         webkit_marshal_BOOLEAN__VOID,
> +                         G_TYPE_BOOLEAN, 0);
> +

I guess you gave up on doing this asynchronously?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:143
>      gboolean   (* decide_policy)  (WebKitWebView             *web_view,
>                                     WebKitPolicyDecision      *decision,
>                                     WebKitPolicyDecisionType  type);
> +    gboolean   (* entering_fullscreen)  (WebKitWebView  *web_view);
> +    gboolean   (* leaving_fullscreen)  (WebKitWebView  *web_view);

Please keep these aligned.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:292
> +    static gboolean enteringFullScreen(WebKitWebView*, UIClientTest* test)
> +    {
> +        if (test->m_fullScreenAllowed) {
> +            test->m_webViewEvents.append(EnteringFullScreen);
> +            g_timeout_add(200, (GSourceFunc) emitKeyStroke, test);
> +        } else {
> +            gtk_widget_destroy(gtk_widget_get_toplevel(GTK_WIDGET(test->m_webView)));
> +            g_main_loop_quit(test->m_mainLoop);
> +        }
> +        return test->m_fullScreenAllowed;
> +    }
> +
> +    static gboolean leavingFullScreen(WebKitWebView*, UIClientTest* test)
> +    {
> +        test->m_webViewEvents.append(LeavingFullScreen);
> +        gtk_widget_destroy(gtk_widget_get_toplevel(GTK_WIDGET(test->m_webView)));
> +        g_main_loop_quit(test->m_mainLoop);
> +        return TRUE;
> +    }

You should subclass this fixture instead of just adding on to it. Why do you destroy the WebView here? The test fixture cleans up the WebView.  Please do not use C style casts. 

You should be careful to keep the knowedge of the fixture internal to the fixture. If the fixture needs to call emitKeyStroke, then emitKeyStroke should be a static method of the fixture. Why do you call g_timeout_add instead of something like g_idle_add? 200 milliseconds seems like a long time to wait.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:335
> +    // create and send the event

Comments should be complete sentences that start with a capital letter and end with a period.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:336
> +    GdkEvent* pressEvent = gdk_event_new(GDK_KEY_PRESS);

You should use GOwnPtr to hold the pressEven.t

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:347
> +    GdkKeymapKey* keys;

You should use GOwnPtr here.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:354
> +    GdkEvent* releaseEvent = gdk_event_copy(pressEvent);

You should use GOwnPtr here too.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:386
> +                   "function thunk() {"
> +                   "    document.removeEventListener(eventName, thunk, false);"
> +                   "    document.documentElement.webkitRequestFullScreen();"
> +                   "}"
> +                   "document.addEventListener(eventName, thunk, false);"

Just use an anonymous function inline.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:390
> +    GtkWidget* window;
> +    window = gtk_window_new(GTK_WINDOW_POPUP);

This should be one line.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:394
> +    g_timeout_add(100, (GSourceFunc) emitKeyStroke, test);

C++ style cast.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:425
> +    test->loadHtml("<html><body>"
> +                   "<script>"
> +                   "var eventName = 'keypress';"
> +                   "function thunk() {"
> +                   "    document.removeEventListener(eventName, thunk, false);"
> +                   "    document.documentElement.webkitRequestFullScreen();"
> +                   "}"
> +                   "document.addEventListener(eventName, thunk, false);"
> +                   "</script></body></html>", 0);
> +
> +    GtkWidget* window;
> +    window = gtk_window_new(GTK_WINDOW_POPUP);
> +    gtk_container_add(GTK_CONTAINER(window), GTK_WIDGET(test->m_webView));
> +
> +    gtk_widget_show_all(window);
> +    g_timeout_add(100, (GSourceFunc) emitKeyStroke, test);
> +
> +    test->waitUntilMainLoopFinishes();

It seems like this is very similar to the code above. Can you refactor it so that it's shared?

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:78
> +    gboolean returnValue;
> +    g_signal_emit_by_name(WEBKIT_WEB_VIEW(m_webViewBase.get()), "entering-fullscreen", &returnValue);
> +    if (!returnValue)
> +        return;
> +
>      this->fullScreenManagerProxy()->willEnterFullScreen();
>      this->fullScreenManagerProxy()->beginEnterFullScreenAnimation(10);

A WebKitWebViewBase is not a WebKitWebView when you are using the C API. Perhaps we should only implement this default behavior when using GObject API. I think it makes sense to put the default behavior in the virtual method that you add above. This way, extending the WebView and chaining up works properly.

> Tools/MiniBrowser/gtk/BrowserWindow.c:225
> +static gboolean webViewFullscreenMessageWindowClose(GtkWidget *dialog)
> +{
> +    if (GTK_IS_WIDGET (dialog))
> +        g_signal_emit_by_name (GTK_DIALOG (dialog), "close");
> +    return FALSE;
> +}

You are using Gnome coding style in this file. check-webkit-style should catch all of these errors.

[Philippe Normand]

(In reply to comment #6)
> (From update of attachment 124730 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124730&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:645
> > +    signals[ENTERING_FULLSCREEN] =
> > +            g_signal_new("entering-fullscreen",
> > +                         G_TYPE_FROM_CLASS(webViewClass),
> > +                         G_SIGNAL_RUN_LAST,
> > +                         G_STRUCT_OFFSET(WebKitWebViewClass, entering_fullscreen),
> > +                         g_signal_accumulator_first_wins, 0,
> > +                         webkit_marshal_BOOLEAN__VOID,
> > +                         G_TYPE_BOOLEAN, 0);
> > +
> 
> I guess you gave up on doing this asynchronously?
> 

I never did. I think it actually works, when testing in the MiniLauncher the popup asynchronously waits for user-input. What makes you think this signal is not set up as expected?

[Philippe Normand]

(In reply to comment #6)
> (From update of attachment 124730 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124730&action=review
> 
> 
> You should subclass this fixture instead of just adding on to it.

Seriously? Why don't the other tests of this file subclass the fixture then?

> Why do you destroy the WebView here?

Because in the case where fullscreen is blocked the leavingFullScreen() is not called.

> The test fixture cleans up the WebView.  Please do not use C style casts. 
> 

Ok.

> You should be careful to keep the knowedge of the fixture internal to the fixture. If the fixture needs to call emitKeyStroke, then emitKeyStroke should be a static method of the fixture.

Ok.

> Why do you call g_timeout_add instead of something like g_idle_add? 200 milliseconds seems like a long time to wait.
> 

Right.


[...]

> 
> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:425
> > +    test->loadHtml("<html><body>"
> > +                   "<script>"
> > +                   "var eventName = 'keypress';"
> > +                   "function thunk() {"
> > +                   "    document.removeEventListener(eventName, thunk, false);"
> > +                   "    document.documentElement.webkitRequestFullScreen();"
> > +                   "}"
> > +                   "document.addEventListener(eventName, thunk, false);"
> > +                   "</script></body></html>", 0);
> > +
> > +    GtkWidget* window;
> > +    window = gtk_window_new(GTK_WINDOW_POPUP);
> > +    gtk_container_add(GTK_CONTAINER(window), GTK_WIDGET(test->m_webView));
> > +
> > +    gtk_widget_show_all(window);
> > +    g_timeout_add(100, (GSourceFunc) emitKeyStroke, test);
> > +
> > +    test->waitUntilMainLoopFinishes();
> 
> It seems like this is very similar to the code above. Can you refactor it so that it's shared?
> 

Yes. I will add a Test::addWithData(const char* suiteName, const char* testName, void (*testFunc)(ClassName*, const void*), gconstpointer data) then.

[Martin Robinson]

(In reply to comment #8)
> Seriously? Why don't the other tests of this file subclass the fixture then?

In this case the test fixture is the UIClient test fixture.  I think it's important not to bloat the fixtures with information about all the different types of tests, so I suggested creating a new fixture. At the very least, I think it deserves a rename, since fullscreen support isn't part of the UIClient.

[Philippe Normand]

Created attachment 124971 [details]
FullScreen signals

[Philippe Normand]

Comment on attachment 124971 [details]
FullScreen signals

Moving out of review queue, this patch needs a refactoring after Jer's patches for mac port.

[Philippe Normand]

Created attachment 131323 [details]
FullScreen signals

[Carlos Garcia Campos]

Comment on attachment 131323 [details]
FullScreen signals

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

Ideally, there would be a WKFullscreenClient in the C API that we would implement in WebKitWebView to emit our signals. But for some reason there isn't, so the solution, I think, could be to use an internal private GObject in the API layer, used by the WebKitWebViewBase to notify the WebKitWebView about enter/leave fullscreen.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:67
> +    ENTERING_FULLSCREEN,
> +    LEAVING_FULLSCREEN,

I would call these ENTER and LEAVE, since you can control whether to enter/leave fullscreen. Other signal names use the same pattern.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:680
> +     * signal is not handled the WebView will proceed to full screen

the #WebKitWebView

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:690
> +     * Since: 1.9.0

Don't use Since tags in WebKit2 code, there's no previous versions.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:706
> +     * Emitted when the WebView is about to restore its top level

the #WebKitWebView

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:714
> +     * Since: 1.9.0

Same here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:889
> +gboolean webkitWebViewEnteringFullScreen(WebKitWebView* webView)
> +{
> +    return FALSE;
> +}
> +
> +gboolean webkitWebViewLeavingFullScreen(WebKitWebView* webView)
> +{
> +    return FALSE;
> +}

The default behaviour of true_handled is returning FALSE, so we don't need these default impl.

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:74
> +    gboolean returnValue;
> +    g_signal_emit_by_name(WEBKIT_WEB_VIEW(m_webViewBase), "entering-fullscreen", &returnValue);
> +    if (returnValue)
> +        return;

This is API of WebKitWebView, not WebKitWebViewBase, that would produce a critical warning at run time when using the C API. Out of the API layer, WebKitWebView shouldn't be used, that's why we have the separation.

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:95
> +    gboolean returnValue;
> +    g_signal_emit_by_name(WEBKIT_WEB_VIEW(m_webViewBase), "leaving-fullscreen", &returnValue);
> +    if (returnValue)
> +        return;

Ditto.

[Philippe Normand]

Comment on attachment 131323 [details]
FullScreen signals

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:67
>> +    LEAVING_FULLSCREEN,
> 
> I would call these ENTER and LEAVE, since you can control whether to enter/leave fullscreen. Other signal names use the same pattern.

Hrmm this API was discussed in the mailing list, no one complained on this name and I implemented it for WebKit1... So... Do we want to remain consistent here or not? I don't mind changing this now but it feels late already (IMHO).

[Philippe Normand]

(In reply to comment #13)
> (From update of attachment 131323 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131323&action=review
> 
> Ideally, there would be a WKFullscreenClient in the C API that we would implement in WebKitWebView to emit our signals. But for some reason there isn't, so the solution, I think, could be to use an internal private GObject in the API layer, used by the WebKitWebViewBase to notify the WebKitWebView about enter/leave fullscreen.
> 

Hummm ok... :P

[Carlos Garcia Campos]

(In reply to comment #14)
> (From update of attachment 131323 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131323&action=review
> 
> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:67
> >> +    LEAVING_FULLSCREEN,
> > 
> > I would call these ENTER and LEAVE, since you can control whether to enter/leave fullscreen. Other signal names use the same pattern.
> 
> Hrmm this API was discussed in the mailing list, no one complained on this name and I implemented it for WebKit1... So... Do we want to remain consistent here or not? I don't mind changing this now but it feels late already (IMHO).

In WebKit1 there are other signals using that approach like WebKitWebView::hovering-over-link or WebKitWebView::resource-request-starting. The ing form sounds more like a notification, not something that you can control or avoid. In this case enter/leave fullscreen signals are not just a notification that something is going to happen, you can use them to avoid it to happen or even provide your own implementation. In WebKit2 we usually use the past for notification about things that already happened (load-changed, load-failed, mouse-target-changed, print-request) and imperative for signals that you can block or provide your own implementation (create, close, decide-policy). That's why I thought it would be better to use enter/leave instead of entering/leaving in WebKit2.

[Carlos Garcia Campos]

Created attachment 135344 [details]
New patch

Phil asked me to rework his patch, so here is a new version. It adds a new WKFullScreenClientGtk, specific to the GTK port, so that the code is the same than any other clients. I've left the minoborwser part our for a following patch.

[Gustavo Noronha (kov)]

Comment on attachment 135344 [details]
New patch

LGTM

[Carlos Garcia Campos]

Committed r113697: <http://trac.webkit.org/changeset/113697>