Bug 75553 – [GTK][WK2] Initial FullScreen support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

It'd be great to have a working FullScreen JavaScript API support in WebKit2GTK. One obvious use-case is about Youtube HTML5 fullscreen video playback.

Will upload a patch and ask for initial feedback.

[Philippe Normand]

Created attachment 121118 [details]
Patch

[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

[Gustavo Noronha (kov)]

Comment on attachment 121118 [details]
Patch

Attachment 121118 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10915253

[Carlos Garcia Campos]

Comment on attachment 121118 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:86
> +    RefPtr<FullScreenWindowController> fullScreenWindowController;

I think this should be part of WebKitWebViewBase, not WebKitWebView, otherwise fullscreen won't work when using the C API.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:481
> +#if ENABLE(FULLSCREEN_API)
> +    priv->pageProxy->fullScreenManager()->setWebView(webkitWebViewBase);
> +#endif

I think we can just ignore setWebView(). The fullscreen manager is created with a WebPageProxy that already has a method to get the view WebPageProxy::viewWidget();

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:24
> +#include "FullScreenWindowController.h"

I think we can avoid this object and move the implementation to WebKitWebViewBase

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:26
> +#include "IntRect.h"

Use <WebCore/IntRect.h>, we prefer angle-bracket includes for non wk2 headers.

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:61
> +    case 'f':
> +    case 'F':

Use GDK_KEY_f and GDK_KEY_F. Is it possible to exit fullscreen without the keyboard?

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:83
> +    gdk_window_fullscreen(gtk_widget_get_parent_window(GTK_WIDGET(m_webView.get())));

You should use topLevelWindow instead of getting the view parent window. Or just call gtk_window_fullscreen().

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:100
> +    gdk_window_unfullscreen(gtk_widget_get_parent_window(GTK_WIDGET(m_webView.get())));

Ditto.

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:111
> +    GdkWindow* window = gtk_widget_get_parent_window(GTK_WIDGET(m_webView.get()));

Ditto.

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:113
> +    gdk_window_get_frame_extents(window, &rect);

Isn't the FullScreenRect the screenRect()?

> Source/WebKit2/UIProcess/gtk/WebFullScreenManagerProxyGtk.cpp:50
> -    notImplemented();
> +    if (!m_webView)
> +        return;
> +
> +    WebKitWebView* webView = WEBKIT_WEB_VIEW(m_webView);
> +    RefPtr<FullScreenWindowController> controller = webkitWebViewFullscreenWindowController(webView);
> +    controller->setWebView(webView);
> +    controller->enterFullScreen();

Without the controller object this would be something like:

m_page->fullScreenManager()->willEnterFullScreen();
m_page->fullScreenManager()->beginEnterFullScreenAnimation(10);
webkitWebViewBaseEnterFullscreen(WEBKIT_WEB_VIEW_BASE(m_page->viewWidget());
m_page->fullScreenManager()->didEnterFullScreen();

WebKitWebViewBase would use a flag to know that it's in fullscreen mode and then in webkitWebViewBaseKeyPressEvent() you can check whether user pressed f, F or ESC while in fullscreen mode.

> Source/WebKit2/UIProcess/gtk/WebFullScreenManagerProxyGtk.cpp:60
> -    notImplemented();
> +    if (!m_webView)
> +        return;
> +
> +    WebKitWebView* webView = WEBKIT_WEB_VIEW(m_webView);
> +    RefPtr<FullScreenWindowController> controller = webkitWebViewFullscreenWindowController(webView);
> +    controller->exitFullScreen();

This would be just 

m_page->fullScreenManager()->willExitFullScreen();
m_page->fullScreenManager()->beginExitFullScreenAnimation(10);
webkitWebViewBaseExitFullscreen(WEBKIT_WEB_VIEW_BASE(m_page->viewWidget());
m_page->fullScreenManager()->didExitFullScreen();

> Source/WebKit2/UIProcess/gtk/WebFullScreenManagerProxyGtk.cpp:100
> -    notImplemented();
> +    if (!m_webView)
> +        return;
> +
> +    WebKitWebView* webView = WEBKIT_WEB_VIEW(m_webView);
> +    RefPtr<FullScreenWindowController> controller = webkitWebViewFullscreenWindowController(webView);
> +    rect = controller->getFullScreenRect();

Can we use PlatformScreen::screenRect() from WebCore?

[Philippe Normand]

(In reply to comment #4)
> (From update of attachment 121118 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121118&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:86
> > +    RefPtr<FullScreenWindowController> fullScreenWindowController;
> 
> I think this should be part of WebKitWebViewBase, not WebKitWebView, otherwise fullscreen won't work when using the C API.
> 

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:481
> > +#if ENABLE(FULLSCREEN_API)
> > +    priv->pageProxy->fullScreenManager()->setWebView(webkitWebViewBase);
> > +#endif
> 
> I think we can just ignore setWebView(). The fullscreen manager is created with a WebPageProxy that already has a method to get the view WebPageProxy::viewWidget();
> 

Hum will check this again, IIRC I just did like mac :)

> > Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:24
> > +#include "FullScreenWindowController.h"
> 
> I think we can avoid this object and move the implementation to WebKitWebViewBase
> 

Not sure, if we plan to implement the animation if might be better to keep this code contained in a separate class.

> > Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:26
> > +#include "IntRect.h"
> 
> Use <WebCore/IntRect.h>, we prefer angle-bracket includes for non wk2 headers.
> 

Oh, ok! Not so used to WK2 yet :)

> > Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:61
> > +    case 'f':
> > +    case 'F':
> 
> Use GDK_KEY_f and GDK_KEY_F.

Ok

> Is it possible to exit fullscreen without the keyboard?
> 

It depends! If the fullscreen element is a video you can exit fullscreen by clicking on the media-controls fullscreen button. Otherwise, no way yet.

There are 3 issues to address:

- when the window is fullscreened the UA should be notified so it can hide the widgets that are part of the window (like the top-bar of the gtk-launcher, for example)
- notify the user he can exit with the keyboard, f, F or escape
- somehow allow mouse interaction to exit fullscreen in all cases.


> > Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:83
> > +    gdk_window_fullscreen(gtk_widget_get_parent_window(GTK_WIDGET(m_webView.get())));
> 
> You should use topLevelWindow instead of getting the view parent window. Or just call gtk_window_fullscreen().
> 

Ok, will try that, not sure anymore why I used the GdkWindow approach :)

> > Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:100
> > +    gdk_window_unfullscreen(gtk_widget_get_parent_window(GTK_WIDGET(m_webView.get())));
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:111
> > +    GdkWindow* window = gtk_widget_get_parent_window(GTK_WIDGET(m_webView.get()));
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:113
> > +    gdk_window_get_frame_extents(window, &rect);
> 
> Isn't the FullScreenRect the screenRect()?
> 

Could be yes. Will double-check.

[...]

> > Source/WebKit2/UIProcess/gtk/WebFullScreenManagerProxyGtk.cpp:100
> > -    notImplemented();
> > +    if (!m_webView)
> > +        return;
> > +
> > +    WebKitWebView* webView = WEBKIT_WEB_VIEW(m_webView);
> > +    RefPtr<FullScreenWindowController> controller = webkitWebViewFullscreenWindowController(webView);
> > +    rect = controller->getFullScreenRect();
> 
> Can we use PlatformScreen::screenRect() from WebCore?

Probably yes. Will check it, thanks for the review!

[Carlos Garcia Campos]

(In reply to comment #5)
> (In reply to comment #4)
> > > Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:24
> > > +#include "FullScreenWindowController.h"
> > 
> > I think we can avoid this object and move the implementation to WebKitWebViewBase
> > 
> 
> Not sure, if we plan to implement the animation if might be better to keep this code contained in a separate class.

I prefer to keep it simple now and add the complexity later if we end up implementing the animation. But if you are sure we will need this object sooner or later, it's ok for me to add it now.

[Philippe Normand]

(In reply to comment #4)
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:481
> > +#if ENABLE(FULLSCREEN_API)
> > +    priv->pageProxy->fullScreenManager()->setWebView(webkitWebViewBase);
> > +#endif
> 
> I think we can just ignore setWebView(). The fullscreen manager is created with a WebPageProxy that already has a method to get the view WebPageProxy::viewWidget();
> 

Well I just implemented the WebFullScreenManagerProxy class, which stores both the WebPageProxy and the WebView. If the WebView is really not needed we should open another bug to address this issue.

> 
> > Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:113
> > +    gdk_window_get_frame_extents(window, &rect);
> 
> Isn't the FullScreenRect the screenRect()?
> 

Same here, this method is used by the FullScreenManagerProxy::getFullScreenRect(). If this is redundant with screenRect() it should be fixed in another patch :)

[Philippe Normand]

Created attachment 121276 [details]
Patch

[WebKit Review Bot]

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

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

Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:24:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 8 files


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

[Philippe Normand]

Created attachment 121278 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 121276 [details]
Patch

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

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:64
> +static gboolean onFullscreenGtkKeyPressEvent(GtkWidget* widget, GdkEventKey* event, FullScreenWindowController* controller)
> +{
> +    switch (event->keyval) {
> +    case GDK_KEY_Escape:
> +    case GDK_KEY_f:
> +    case GDK_KEY_F:
> +        controller->exitFullScreen();

This could be moved to webkitWebViewBaseKeyPressEvent(), checking whether it's in fullscreen or not, probably adding FullScreenWindowController::fullscreenEnabled().

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:70
> +    return TRUE;

You should only return TRUE when the event was handled (ESC, f or F was pressed).

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:85
> +    GtkWidget* topLevelWindow = gtk_widget_get_toplevel(GTK_WIDGET(m_webViewBase.get()));
> +    if (gtk_widget_is_toplevel(topLevelWindow))
> +        g_signal_connect(topLevelWindow, "key-press-event", G_CALLBACK(onFullscreenGtkKeyPressEvent), this);
> +
> +    gtk_window_fullscreen(GTK_WINDOW(topLevelWindow));

Even using the controller object I still think this should be done in the WebView. webkitWebViewBaseEnterFullScreen().

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:97
> +    GtkWidget* topLevelWindow = gtk_widget_get_toplevel(GTK_WIDGET(m_webViewBase.get()));
> +    if (gtk_widget_is_toplevel(topLevelWindow))
> +        g_signal_handlers_disconnect_by_func(topLevelWindow, reinterpret_cast<void*>(onFullscreenGtkKeyPressEvent), this);

You don't need this if you use webkitWebViewBaseKeyPressEvent().

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:102
> +    gtk_window_unfullscreen(GTK_WINDOW(topLevelWindow));

Ditto, webkitWebViewBaseExitFullscreen().

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.h:25
> +#include "GRefPtr.h"

Use <wtf/gobject/GRefPtr.h>

[Gustavo Noronha (kov)]

Comment on attachment 121276 [details]
Patch

Attachment 121276 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11115383

[Philippe Normand]

(In reply to comment #11)
> (From update of attachment 121276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121276&action=review
> 
> > Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:64
> > +static gboolean onFullscreenGtkKeyPressEvent(GtkWidget* widget, GdkEventKey* event, FullScreenWindowController* controller)
> > +{
> > +    switch (event->keyval) {
> > +    case GDK_KEY_Escape:
> > +    case GDK_KEY_f:
> > +    case GDK_KEY_F:
> > +        controller->exitFullScreen();
> 
> This could be moved to webkitWebViewBaseKeyPressEvent(), checking whether it's in fullscreen or not, probably adding FullScreenWindowController::fullscreenEnabled().
> 

But it'd need to be guarded with ifdefs there. Sorry but I still prefer this self-contained approach of the FullScreenWindowController ;)

[Gustavo Noronha (kov)]

Comment on attachment 121278 [details]
Patch

Attachment 121278 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11120388

[Philippe Normand]

Created attachment 121284 [details]
Patch

[Martin Robinson]

Comment on attachment 121284 [details]
Patch

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

Not a full review, but here are a few things I noticed.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:80
> +#if ENABLE(FULLSCREEN_API)
> +    RefPtr<FullScreenWindowController> fullScreenWindowController;
> +#endif
>  };

This doesn't actually need to be allocated on the heap. You can just do:
FullScreenWindowController fullScreenWindowController;

Be sure that it's still noncopyable though.

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:32
> +#include <WebCore/PlatformScreen.h>
> +
> +#include <gdk/gdk.h>

No extra newline necessary here.

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.h:25
> +

Extra newline here.

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.h:32
> +class FullScreenWindowController : public RefCounted<FullScreenWindowController> {

I think this can just extend from Noncopyable abfter the change above.

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.h:33
> +protected:

Why protected?

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.h:37
> +    virtual ~FullScreenWindowController();

- should probably be ~ here. :)

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.h:56
> +#endif
> +
> +#endif

For C++ files usually we do something like:

#endif // ENABLE(FULLSCREEN)

#endif // FullScreenWindowController_h

[Philippe Normand]

(In reply to comment #16)
> (From update of attachment 121284 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121284&action=review
> 
> Not a full review, but here are a few things I noticed.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:80
> > +#if ENABLE(FULLSCREEN_API)
> > +    RefPtr<FullScreenWindowController> fullScreenWindowController;
> > +#endif
> >  };
> 
> This doesn't actually need to be allocated on the heap. You can just do:
> FullScreenWindowController fullScreenWindowController;
> 
> Be sure that it's still noncopyable though.
> 

Hum yes but the WebFullScreenManagerProxyGtk needs access to the FullScreenWindowController. If I make it noncopyable and not allocated on the heap I get this error:


../../Source/WebKit2/UIProcess/gtk/FullScreenWindowController.h: In member function ‘void WebKit::WebFullScreenManagerProxy::enterFullScreen()’:
../../Source/WebKit2/UIProcess/gtk/FullScreenWindowController.h:32:14: error: ‘FullScreenWindowController::FullScreenWindowController(const FullScreenWindowController&)’ is private
../../Source/WebKit2/UIProcess/gtk/WebFullScreenManagerProxyGtk.cpp:48:100: error: within this context
../../Source/WebKit2/UIProcess/gtk/FullScreenWindowController.h: In member function ‘void WebKit::WebFullScreenManagerProxy::exitFullScreen()’:
../../Source/WebKit2/UIProcess/gtk/FullScreenWindowController.h:32:14: error: ‘FullScreenWindowController::FullScreenWindowController(const FullScreenWindowController&)’ is private

[Philippe Normand]

Created attachment 122209 [details]
Patch

[Martin Robinson]

(In reply to comment #17)

> Hum yes but the WebFullScreenManagerProxyGtk needs access to the FullScreenWindowController. If I make it noncopyable and not allocated on the heap I get this error:

This can be solved by making webkitWebViewBaseFullscreenWindowController return a const WebFullScreenManagerProxy&. A good rule to follow is if there is only one of something it should either a raw object or owned by an OwnPtr.

[Martin Robinson]

Comment on attachment 122209 [details]
Patch

The patch still includes a few of the extra newlines that I pointed out.

[Philippe Normand]

Created attachment 122256 [details]
Patch

[WebKit Review Bot]

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

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

Source/WebKit2/UIProcess/gtk/FullScreenWindowController.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
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]

Created attachment 122257 [details]
Patch

[Philippe Normand]

Created attachment 124729 [details]
Initial FullScreen support

[WebKit Review Bot]

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

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

Updating OpenSource
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 124729 [details]
Initial FullScreen support

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

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:32
> +#include <WebCore/PlatformScreen.h>
> +
> +#include <gdk/gdk.h>

Please remove the extra newline here.

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.h:48
> +    GRefPtr<WebKitWebViewBase> m_webViewBase;

This actually means that all WebKitWebViewBase will leak because the WebViewBase owns the fullscreen controller and the fullscreen controller holds a reference. :)

> Source/WebKit2/UIProcess/gtk/WebFullScreenManagerProxyGtk.cpp:32
>  #include "WebProcess.h"
>  
>  #include <WebCore/NotImplemented.h>

Please remove the extra newline here.

> Source/WebKit2/UIProcess/gtk/WebFullScreenManagerProxyGtk.cpp:60
> +    WebKitWebViewBase* webViewBase = WEBKIT_WEB_VIEW_BASE(m_webView);
> +    FullScreenWindowController& controller = webkitWebViewBaseFullscreenWindowController(webViewBase);
> +    controller.exitFullScreen();

This can be one line.

> Source/WebKit2/UIProcess/gtk/WebFullScreenManagerProxyGtk.cpp:100
> +    FullScreenWindowController& controller = webkitWebViewBaseFullscreenWindowController(webViewBase);
> +    rect = controller.getFullScreenRect();

Ditto.

[Philippe Normand]

Created attachment 124970 [details]
Initial FullScreen support

[Gustavo Noronha (kov)]

Comment on attachment 124970 [details]
Initial FullScreen support

Attachment 124970 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11388504

[Philippe Normand]

(In reply to comment #28)
> (From update of attachment 124970 [details])
> Attachment 124970 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/11388504

Needs a clean build on the EWS and bots when landing.

[Philippe Normand]

Comment on attachment 124970 [details]
Initial FullScreen support

Oh hum I actually messed up that one. Will update it soon :)

[Philippe Normand]

Created attachment 125096 [details]
Patch

[Carlos Garcia Campos]

I wonder whether there should be a WKFullscreenClient in the C API with enter/leave fullscreen callbacks. That way, we would implement it in WebKitWebView emitting signals to let the user handle them, and we don't need anything in WebKitWebViewBase, since the C API users are supposed to implement the fullscreen client.

[Philippe Normand]

Comment on attachment 125096 [details]
Patch

The Mac WK2 Fullscreen API is being reworked a bit. I'll rework this patch later.

[Philippe Normand]

Created attachment 131322 [details]
Initial FullScreen support

[Carlos Garcia Campos]

Comment on attachment 131322 [details]
Initial FullScreen support

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

I think moving the FullScreenWindowController to WebKitWebViewBase would make the things clearer and easier, specially to implement the signals in WebKitWebView

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:36
> +    : m_webViewBase(0)

I would call this webView, for wk2 internal code there's just a view.

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:97
> +    m_webViewBase = 0;

Why clearing the view after exiting fullscreen? Isn't this object owned by the view indeed?

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:108
> +#endif

I still think this object is not needed, the impl could be part of WebKitWebViewBase, so that the fullscreen manager proxy just need to call webkitWebViewBaseEnterFullscreen/ExitFullscreen(). I know the mac port has a WKFullScreenWindowController, but the WK prefix makes me think it's actually part of the API layer somehow. We could use a private object in our API layer that could be used by WebKitWebView to implement the fullscreen signals.

> Source/WebKit2/UIProcess/gtk/WebFullScreenManagerProxyGtk.cpp:50
> +    WebKitWebViewBase* webViewBase = WEBKIT_WEB_VIEW_BASE(m_webView);
> +    FullScreenWindowController& controller = webkitWebViewBaseFullscreenWindowController(webViewBase);
> +    controller.setWebViewBase(webViewBase);

It's a bit weird that you ask the view for the controller and then you need to set the view of the controller. That controller is stack allocated in the view, isn't it associated to that view forever?

[Philippe Normand]

Created attachment 132293 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 132293 [details]
Patch

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

Cool, it looks good!

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:34
> +#include <gtk/gtk.h>

gtk is already included by WebKitWebViewBase.h

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:82
> +    bool fullScreenEnabled;

enabled sounds like you allow to run fullscreen, rather than the view is currently running fullscreen. I would use something like runningFullscreen or something like that.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:606
> +#if ENABLE(FULLSCREEN_API)
> +static gboolean onFullscreenGtkKeyPressEvent(GtkWidget* widget, GdkEventKey* event, WebKitWebViewBase* webkitWebViewBase)
> +{
> +    switch (event->keyval) {
> +    case GDK_KEY_Escape:
> +    case GDK_KEY_f:
> +    case GDK_KEY_F:
> +        webkitWebViewBaseExitFullScreen(webkitWebViewBase);
> +        break;
> +    default:
> +        break;
> +    }
> +
> +    return TRUE;
> +}
> +#endif

Override key_press_event instead of connecting a callback and return early in the view is not running fullscreen

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:615
> +    WebPageProxy* webPage = webkitWebViewBaseGetPage(webkitWebViewBase);

Use priv->pageProxy directly, you don't need to check it's not NULL, it should be valid because it's created when the view is created. You could add an ASSERT if you want to make sure.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:619
> +    WebFullScreenManagerProxy* fullScreenManagerProxy = webPage->fullScreenManager();

This could be 

WebFullScreenManagerProxy* fullScreenManagerProxy = priv->pageProxy->fullScreenManager();

and you don't need to cache the WebPageProxy

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:625
> +    GtkWidget* topLevelWindow = gtk_widget_get_toplevel(GTK_WIDGET(webkitWebViewBase));
> +    if (widgetIsOnscreenToplevelWindow(topLevelWindow))
> +        g_signal_connect(topLevelWindow, "key-press-event", G_CALLBACK(onFullscreenGtkKeyPressEvent), webkitWebViewBase);

Do we need to connect to key_press_event in the toplevel? can't we just override key_press_event in the WebView?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:644
> +    WebPageProxy* webPage = webkitWebViewBaseGetPage(webkitWebViewBase);
> +    if (!webPage)
> +        return;
> +
> +    WebFullScreenManagerProxy* fullScreenManagerProxy = webPage->fullScreenManager();

WebFullScreenManagerProxy* fullScreenManagerProxy = priv->pageProxy->fullScreenManager();

> Source/WebKit2/UIProcess/gtk/WebFullScreenManagerProxyGtk.cpp:46
> +    webkitWebViewBaseEnterFullScreen(WEBKIT_WEB_VIEW_BASE(m_webView));

m_webView is a WebKitWebViewBase* so you don't need the cast.

> Source/WebKit2/UIProcess/gtk/WebFullScreenManagerProxyGtk.cpp:54
> +    webkitWebViewBaseExitFullScreen(WEBKIT_WEB_VIEW_BASE(m_webView));

Ditto.

[Philippe Normand]

Comment on attachment 132293 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:606
>> +#endif
> 
> Override key_press_event instead of connecting a callback and return early in the view is not running fullscreen

Do you mean move that code to webkitWebViewBaseKeyPressEvent?

[Carlos Garcia Campos]

(In reply to comment #38)
> (From update of attachment 132293 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132293&action=review
> 
> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:606
> >> +#endif
> > 
> > Override key_press_event instead of connecting a callback and return early in the view is not running fullscreen
> 
> Do you mean move that code to webkitWebViewBaseKeyPressEvent?

Yes.

[Philippe Normand]

(In reply to comment #37)

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:82
> > +    bool fullScreenEnabled;
> 
> enabled sounds like you allow to run fullscreen, rather than the view is currently running fullscreen. I would use something like runningFullscreen or something like that.
> 

If I wanted to make it sound as I allowed the view to run fullscreen I'd have named the variable fullScreenAllowed :)

My (probably very) personal understanding of fullScreenEnabled is that a webview display can be toggled between full screen and "normal windowed" states.

I'm not sure I like the "running" part of runningFullscreen. I'll leave that name as it is for now, naming variables is sometimes hard! Maybe Martin would have a good suggestion as well?

[Carlos Garcia Campos]

(In reply to comment #40)
> (In reply to comment #37)
> 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:82
> > > +    bool fullScreenEnabled;
> > 
> > enabled sounds like you allow to run fullscreen, rather than the view is currently running fullscreen. I would use something like runningFullscreen or something like that.
> > 
> 
> If I wanted to make it sound as I allowed the view to run fullscreen I'd have named the variable fullScreenAllowed :)
> 
> My (probably very) personal understanding of fullScreenEnabled is that a webview display can be toggled between full screen and "normal windowed" states.
> 
> I'm not sure I like the "running" part of runningFullscreen. I'll leave that name as it is for now, naming variables is sometimes hard! Maybe Martin would have a good suggestion as well?

Sure, it was just a suggestion, It sounds to me like a setting that you can enable/disable, but it's also my personal understanding, so if enable/disable sounds good to you it's perfectly fine for me.

[Philippe Normand]

Created attachment 134766 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


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

[Philippe Normand]

Created attachment 134767 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 134767 [details]
Patch

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

Looks great!

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:81
> +    bool fullScreenEnabled;

It seems this is only used inside #if ENABLE(FULLSCREEN_API) #endif blocks, so maybe we should add #if here?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:614
> +
> +

extra line here?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:616
> +

Ditto.

[Martin Robinson]

Comment on attachment 134767 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:81
>> +    bool fullScreenEnabled;
> 
> It seems this is only used inside #if ENABLE(FULLSCREEN_API) #endif blocks, so maybe we should add #if here?

For the naming of this perhaps "inFullScreenMode", "fullScreenModeActive", or "fullScreenActive" would make it clearer.

As an asides, it's a bit odd that in WebKit2 it's referred to as "FullScreen" (i.e. "full screen"), but the guard is "FULLSCREEN" (i.e. "fullscreen").

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:293
> +        switch (event->keyval) {
> +        case GDK_KEY_Escape:
> +        case GDK_KEY_f:
> +        case GDK_KEY_F:
> +            webkitWebViewBaseExitFullScreen(webViewBase);

I guess this will be an issue if a game wants to go fullscreen and use the 'f' key?

[Philippe Normand]

Comment on attachment 134767 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:81
>>> +    bool fullScreenEnabled;
>> 
>> It seems this is only used inside #if ENABLE(FULLSCREEN_API) #endif blocks, so maybe we should add #if here?
> 
> For the naming of this perhaps "inFullScreenMode", "fullScreenModeActive", or "fullScreenActive" would make it clearer.
> 
> As an asides, it's a bit odd that in WebKit2 it's referred to as "FullScreen" (i.e. "full screen"), but the guard is "FULLSCREEN" (i.e. "fullscreen").

Ok I'll go for fullScreenModeActive :)

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:293
>> +            webkitWebViewBaseExitFullScreen(webViewBase);
> 
> I guess this will be an issue if a game wants to go fullscreen and use the 'f' key?

Hum what do you mean? This code is called only when the webview is already in full screen

[Philippe Normand]

Committed r112867: <http://trac.webkit.org/changeset/112867>

[Martin Robinson]

Comment on attachment 134767 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:293
>>> +            webkitWebViewBaseExitFullScreen(webViewBase);
>> 
>> I guess this will be an issue if a game wants to go fullscreen and use the 'f' key?
> 
> Hum what do you mean? This code is called only when the webview is already in full screen

I meant that if  a game wanted to use the 'f' key for something other than toggling the fullscreen state. :)