Summary: | [WK2][EFL] Skip cancel fullscreen request if not requested by FullScreen API | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Bruno Abinader (history only) <bruno.abinader> | ||||||||||
Component: | WebKit EFL | Assignee: | Bruno Abinader (history only) <bruno.abinader> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap, benjamin, cdumez, cmarcelo, commit-queue, gyuyoung.kim, jer.noble, jinwoo7.song, kenneth, lucas.de.marchi, luiz, naginenis, noam, rakuco, webkit.review.bot, zeno | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 108122 | ||||||||||||
Attachments: |
|
Description
Bruno Abinader (history only)
2013-01-29 09:06:05 PST
Created attachment 185262 [details]
Patch
Proposed patch
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? Created attachment 185281 [details]
Patch
Added unit test, made m_isFullScreen a member of WebFullScreenManagerProxy class on EFL.
Created attachment 185285 [details]
Patch
Simplified unit test, ewk_view_fullscreen_exit now returns false if no action was taken.
Comment on attachment 185285 [details]
Patch
LGTM.
@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? *** Bug 112691 has been marked as a duplicate of this bug. *** Can a WK2 owner please take a look at this one? This patch is 2 month-old now and it fixes an annoying crash. *** Bug 113674 has been marked as a duplicate of this bug. *** 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.) Created attachment 199996 [details]
Patch for landing
Rebased/Updated patch to reflect latest changes on WebFullScreenManagerProxy implementation.
Comment on attachment 199996 [details] Patch for landing Clearing flags on attachment: 199996 Committed r149289: <http://trac.webkit.org/changeset/149289> All reviewed patches have been landed. Closing bug. |