[Webkit-unassigned] [Bug 75553] [GTK][WK2] Initial FullScreen support

[bugzilla-daemon at webkit.org]

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





--- Comment #37 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-03-19 07:22:04 PST ---
(From update of attachment 132293)
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.

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