Bug 108201 - [WK2][EFL] Skip cancel fullscreen request if not requested by FullScreen API
Summary: [WK2][EFL] Skip cancel fullscreen request if not requested by FullScreen API
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)
: 112691 113674 (view as bug list)
Depends on:
Blocks: 108122
  Show dependency treegraph
Reported: 2013-01-29 09:06 PST by Bruno Abinader (history only)
Modified: 2013-04-29 09:21 PDT (History)
16 users (show)

See Also:

Patch (3.33 KB, patch)
2013-01-29 10:33 PST, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (6.64 KB, patch)
2013-01-29 12:14 PST, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (5.79 KB, patch)
2013-01-29 12:29 PST, Bruno Abinader (history only)
kling: review+
Details | Formatted Diff | Diff
Patch for landing (8.92 KB, patch)
2013-04-29 06:13 PDT, 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-29 09:06:05 PST
A call for WebProcess' requestExitFullScreen() should only happen if the fullscreen mode was activated by FullScreen API, otherwise the FullScrenManager would not have sufficient information on the element that originated the request and thus crash WebProcess. This can be achieved by implementing the UIProcess' isFullScreen() function from EFL's WebFullScreenManagerProxy.
Comment 1 Bruno Abinader (history only) 2013-01-29 10:33:51 PST
Created attachment 185262 [details]

Proposed patch
Comment 2 Chris Dumez 2013-01-29 10:40:32 PST
Comment on attachment 185262 [details]

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:941

It would be nice to add an API test that calls ewk_view_fullscreen_exit() on a view that is not in fullscreen mode. Since this used to crash, this would be a good way to prevent this to regress in the future.

> Source/WebKit2/UIProcess/efl/WebFullScreenManagerProxyEfl.cpp:34
> +static bool s_isFullScreen = false;

Shouldn't this be a member of WebFullScreenManagerProxy instead?
Comment 3 Bruno Abinader (history only) 2013-01-29 12:14:06 PST
Created attachment 185281 [details]

Added unit test, made m_isFullScreen a member of WebFullScreenManagerProxy class on EFL.
Comment 4 Bruno Abinader (history only) 2013-01-29 12:29:37 PST
Created attachment 185285 [details]

Simplified unit test, ewk_view_fullscreen_exit now returns false if no action was taken.
Comment 5 Chris Dumez 2013-01-29 12:31:21 PST
Comment on attachment 185285 [details]

Comment 6 Bruno Abinader (history only) 2013-01-29 12:58:56 PST
@Benjamin: I've seen that mac also does implement a bool isFullScreen to check whether it has been activated or not by FullScreen API. I wonder if it might be the case to remove the "#if PLATFORM(EFL)" check on m_isFullScreen and use this as default behavior for all platforms?
Comment 7 Sudarsana Nagineni (babu) 2013-03-20 05:55:47 PDT
*** Bug 112691 has been marked as a duplicate of this bug. ***
Comment 8 Chris Dumez 2013-03-31 23:46:21 PDT
Can a WK2 owner please take a look at this one? This patch is 2 month-old now and it fixes an annoying crash.
Comment 9 Jinwoo Song 2013-03-31 23:51:01 PDT
*** Bug 113674 has been marked as a duplicate of this bug. ***
Comment 10 Andreas Kling 2013-04-26 06:11:30 PDT
Comment on attachment 185285 [details]

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

r=me since it fixes your crash, and the code is platform-specific. I don't like the m_isFullScreen name though.

> Source/WebKit2/UIProcess/WebFullScreenManagerProxy.h:105
> +    bool m_isFullScreen;

This name seems a bit too general, since it's only true if full screen mode was entered through the FullScreen API.
Not sure what to call it.. maybe m_hasEnteredFullScreenMode? (Or maybe you have multiple states like the Mac port's FullScreenState? If so then it's still too general.)
Comment 11 Bruno Abinader (history only) 2013-04-29 06:13:20 PDT
Created attachment 199996 [details]
Patch for landing

Rebased/Updated patch to reflect latest changes on WebFullScreenManagerProxy implementation.
Comment 12 WebKit Commit Bot 2013-04-29 08:56:52 PDT
Comment on attachment 199996 [details]
Patch for landing

Clearing flags on attachment: 199996

Committed r149289: <http://trac.webkit.org/changeset/149289>
Comment 13 Bruno Abinader (history only) 2013-04-29 09:21:11 PDT
All reviewed patches have been landed. Closing bug.