Bug 108201 – [WK2][EFL] Skip cancel fullscreen request if not requested by FullScreen API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Bruno Abinader (history only)]

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.

[Bruno Abinader (history only)]

Created attachment 185262 [details]
Patch

Proposed patch

[Chris Dumez]

Comment on attachment 185262 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:941
>  #if ENABLE(FULLSCREEN_API)

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?

[Bruno Abinader (history only)]

Created attachment 185281 [details]
Patch

Added unit test, made m_isFullScreen a member of WebFullScreenManagerProxy class on EFL.

[Bruno Abinader (history only)]

Created attachment 185285 [details]
Patch

Simplified unit test, ewk_view_fullscreen_exit now returns false if no action was taken.

[Chris Dumez]

Comment on attachment 185285 [details]
Patch

LGTM.

[Bruno Abinader (history only)]

@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?

[Sudarsana Nagineni (babu)]

*** Bug 112691 has been marked as a duplicate of this bug. ***

[Chris Dumez]

Can a WK2 owner please take a look at this one? This patch is 2 month-old now and it fixes an annoying crash.

[Jinwoo Song]

*** Bug 113674 has been marked as a duplicate of this bug. ***

[Andreas Kling]

Comment on attachment 185285 [details]
Patch

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.)

[Bruno Abinader (history only)]

Created attachment 199996 [details]
Patch for landing

Rebased/Updated patch to reflect latest changes on WebFullScreenManagerProxy implementation.

[WebKit Commit Bot]

Comment on attachment 199996 [details]
Patch for landing

Clearing flags on attachment: 199996

Committed r149289: <http://trac.webkit.org/changeset/149289>

[Bruno Abinader (history only)]

All reviewed patches have been landed. Closing bug.