Bug 75553 - [GTK][WK2] Initial FullScreen support
Summary: [GTK][WK2] Initial FullScreen support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 76166
  Show dependency treegraph
 
Reported: 2012-01-04 09:18 PST by Philippe Normand
Modified: 2018-05-16 05:02 PDT (History)
5 users (show)

See Also:


Attachments
Patch (15.59 KB, patch)
2012-01-04 09:21 PST, Philippe Normand
gns: commit-queue-
Details | Formatted Diff | Diff
Patch (15.29 KB, patch)
2012-01-05 07:54 PST, Philippe Normand
gns: commit-queue-
Details | Formatted Diff | Diff
Patch (15.27 KB, patch)
2012-01-05 08:02 PST, Philippe Normand
gns: commit-queue-
Details | Formatted Diff | Diff
Patch (15.32 KB, patch)
2012-01-05 08:30 PST, Philippe Normand
mrobinson: review-
Details | Formatted Diff | Diff
Patch (15.42 KB, patch)
2012-01-12 04:09 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (14.58 KB, patch)
2012-01-12 09:15 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (14.58 KB, patch)
2012-01-12 09:19 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Initial FullScreen support (14.62 KB, patch)
2012-01-31 07:32 PST, Philippe Normand
mrobinson: review-
Details | Formatted Diff | Diff
Initial FullScreen support (14.33 KB, patch)
2012-02-01 10:54 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (14.27 KB, patch)
2012-02-02 01:12 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Initial FullScreen support (13.63 KB, patch)
2012-03-12 08:03 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (8.21 KB, patch)
2012-03-16 09:16 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (8.07 KB, patch)
2012-03-30 03:49 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (8.02 KB, patch)
2012-03-30 03:54 PDT, Philippe Normand
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2012-01-04 09:18:02 PST
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.
Comment 1 Philippe Normand 2012-01-04 09:21:08 PST
Created attachment 121118 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-04 09:30:06 PST
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
Comment 3 Gustavo Noronha (kov) 2012-01-04 09:30:31 PST
Comment on attachment 121118 [details]
Patch

Attachment 121118 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10915253
Comment 4 Carlos Garcia Campos 2012-01-05 00:49:43 PST
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?
Comment 5 Philippe Normand 2012-01-05 01:46:33 PST
(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!
Comment 6 Carlos Garcia Campos 2012-01-05 02:14:11 PST
(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.
Comment 7 Philippe Normand 2012-01-05 05:58:00 PST
(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 :)
Comment 8 Philippe Normand 2012-01-05 07:54:30 PST
Created attachment 121276 [details]
Patch
Comment 9 WebKit Review Bot 2012-01-05 07:57:03 PST
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.
Comment 10 Philippe Normand 2012-01-05 08:02:20 PST
Created attachment 121278 [details]
Patch
Comment 11 Carlos Garcia Campos 2012-01-05 08:07:03 PST
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>
Comment 12 Gustavo Noronha (kov) 2012-01-05 08:24:07 PST
Comment on attachment 121276 [details]
Patch

Attachment 121276 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11115383
Comment 13 Philippe Normand 2012-01-05 08:24:51 PST
(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 ;)
Comment 14 Gustavo Noronha (kov) 2012-01-05 08:27:54 PST
Comment on attachment 121278 [details]
Patch

Attachment 121278 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11120388
Comment 15 Philippe Normand 2012-01-05 08:30:57 PST
Created attachment 121284 [details]
Patch
Comment 16 Martin Robinson 2012-01-11 07:41:14 PST
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
Comment 17 Philippe Normand 2012-01-12 00:44:28 PST
(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
Comment 18 Philippe Normand 2012-01-12 04:09:29 PST
Created attachment 122209 [details]
Patch
Comment 19 Martin Robinson 2012-01-12 08:22:33 PST
(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.
Comment 20 Martin Robinson 2012-01-12 08:23:29 PST
Comment on attachment 122209 [details]
Patch

The patch still includes a few of the extra newlines that I pointed out.
Comment 21 Philippe Normand 2012-01-12 09:15:03 PST
Created attachment 122256 [details]
Patch
Comment 22 WebKit Review Bot 2012-01-12 09:16:43 PST
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.
Comment 23 Philippe Normand 2012-01-12 09:19:37 PST
Created attachment 122257 [details]
Patch
Comment 24 Philippe Normand 2012-01-31 07:32:24 PST
Created attachment 124729 [details]
Initial FullScreen support
Comment 25 WebKit Review Bot 2012-01-31 07:35:21 PST
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.
Comment 26 Martin Robinson 2012-01-31 14:15:23 PST
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.
Comment 27 Philippe Normand 2012-02-01 10:54:49 PST
Created attachment 124970 [details]
Initial FullScreen support
Comment 28 Gustavo Noronha (kov) 2012-02-01 10:59:44 PST
Comment on attachment 124970 [details]
Initial FullScreen support

Attachment 124970 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11388504
Comment 29 Philippe Normand 2012-02-01 11:53:51 PST
(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.
Comment 30 Philippe Normand 2012-02-01 11:55:17 PST
Comment on attachment 124970 [details]
Initial FullScreen support

Oh hum I actually messed up that one. Will update it soon :)
Comment 31 Philippe Normand 2012-02-02 01:12:07 PST
Created attachment 125096 [details]
Patch
Comment 32 Carlos Garcia Campos 2012-02-02 01:36:33 PST
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.
Comment 33 Philippe Normand 2012-02-20 09:48:21 PST
Comment on attachment 125096 [details]
Patch

The Mac WK2 Fullscreen API is being reworked a bit. I'll rework this patch later.
Comment 34 Philippe Normand 2012-03-12 08:03:52 PDT
Created attachment 131322 [details]
Initial FullScreen support
Comment 35 Carlos Garcia Campos 2012-03-14 01:43:18 PDT
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?
Comment 36 Philippe Normand 2012-03-16 09:16:00 PDT
Created attachment 132293 [details]
Patch
Comment 37 Carlos Garcia Campos 2012-03-19 07:22:04 PDT
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.
Comment 38 Philippe Normand 2012-03-26 01:52:07 PDT
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?
Comment 39 Carlos Garcia Campos 2012-03-26 01:53:48 PDT
(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.
Comment 40 Philippe Normand 2012-03-30 03:38:38 PDT
(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?
Comment 41 Carlos Garcia Campos 2012-03-30 03:45:49 PDT
(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.
Comment 42 Philippe Normand 2012-03-30 03:49:33 PDT
Created attachment 134766 [details]
Patch
Comment 43 WebKit Review Bot 2012-03-30 03:51:35 PDT
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.
Comment 44 Philippe Normand 2012-03-30 03:54:28 PDT
Created attachment 134767 [details]
Patch
Comment 45 Carlos Garcia Campos 2012-03-30 04:00:06 PDT
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.
Comment 46 Martin Robinson 2012-03-30 11:48:17 PDT
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?
Comment 47 Philippe Normand 2012-04-02 06:00:03 PDT
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
Comment 48 Philippe Normand 2012-04-02 06:07:15 PDT
Committed r112867: <http://trac.webkit.org/changeset/112867>
Comment 49 Martin Robinson 2012-04-02 06:20:18 PDT
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. :)