Bug 108122

Summary: [EFL] Add Toggle fullscreen (F11) to MiniBrowser
Product: WebKit Reporter: Bruno Abinader (history only) <bruno.abinader>
Component: WebKit EFLAssignee: Bruno Abinader (history only) <bruno.abinader>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, gyuyoung.kim, kenneth, laszlo.gombos, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 108201    
Bug Blocks: 108850    
Attachments:
Description Flags
Patch
none
Patch none

Description Bruno Abinader (history only) 2013-01-28 15:36:00 PST
This bug intends to implement the ewk_view_fullscreen_enter() function as part of UIProcess' ewk_view API. This enables application to request for fullscreen (i.e. user triggered F11 key).
Comment 1 Bruno Abinader (history only) 2013-01-28 19:55:48 PST
Created attachment 185138 [details]
Patch

Proposed patch (requires patch from bug 108121).
Comment 2 EFL EWS Bot 2013-01-28 22:57:20 PST
Comment on attachment 185138 [details]
Patch

Attachment 185138 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16160927
Comment 3 Chris Dumez 2013-01-28 23:36:14 PST
Comment on attachment 185138 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:944
> +    impl->page()->fullScreenManager()->requestEnterFullScreen();

Please use C API in new patches. We are in the process of porting everything to the C API.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:802
> +EAPI Eina_Bool ewk_view_fullscreen_enter(Evas_Object *o);

Missing API test.
Comment 4 Bruno Abinader (history only) 2013-01-29 07:24:28 PST
Created attachment 185241 [details]
Patch

Simpler approach using native API suggested by Christophe
Comment 5 Chris Dumez 2013-01-29 07:49:52 PST
Comment on attachment 185241 [details]
Patch

Did you make sure that pressing "ESC" does not exit fullscreen if it has been triggered using F11?
Comment 6 Bruno Abinader (history only) 2013-01-29 07:54:24 PST
(In reply to comment #5)
> (From update of attachment 185241 [details])
> Did you make sure that pressing "ESC" does not exit fullscreen if it has been triggered using F11?

You're right, the browser crashes if ESC is pressed (my bad). However to avoid checking for FullScreen API directly, I'll add a boolean that specifies whether the fullscreen mode was activated by F11 or JS user gesture. Do you agree on this approach?
Comment 7 Chris Dumez 2013-01-29 08:07:50 PST
Hmm, ewk_view_fullscreen_exit() should not crash if full screen was not triggered by fullscreen API. We should probably fix ewk_view_fullscreen_exit() instead of adding a boolean.
Comment 8 Bruno Abinader (history only) 2013-01-29 09:09:09 PST
(In reply to comment #7)
> Hmm, ewk_view_fullscreen_exit() should not crash if full screen was not triggered by fullscreen API. We should probably fix ewk_view_fullscreen_exit() instead of adding a boolean.

Agreed, so this bug's implementation is correct. This crash is now handled on bug 108201.
Comment 9 Chris Dumez 2013-01-29 09:26:06 PST
Comment on attachment 185241 [details]
Patch

LGTM.
Comment 10 Antonio Gomes 2013-01-29 12:38:50 PST
Comment on attachment 185241 [details]
Patch

Trivial enough. r+
Comment 11 WebKit Review Bot 2013-01-29 13:06:18 PST
Comment on attachment 185241 [details]
Patch

Clearing flags on attachment: 185241

Committed r141152: <http://trac.webkit.org/changeset/141152>
Comment 12 WebKit Review Bot 2013-01-29 13:06:24 PST
All reviewed patches have been landed.  Closing bug.