Bug 108122 - [EFL] Add Toggle fullscreen (F11) to MiniBrowser
Summary: [EFL] Add Toggle fullscreen (F11) to MiniBrowser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bruno Abinader (history only)
URL:
Keywords:
Depends on: 108201
Blocks: 108850
  Show dependency treegraph
 
Reported: 2013-01-28 15:36 PST by Bruno Abinader (history only)
Modified: 2013-02-04 12:46 PST (History)
7 users (show)

See Also:


Attachments
Patch (4.36 KB, patch)
2013-01-28 19:55 PST, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (1.69 KB, patch)
2013-01-29 07:24 PST, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.