Bug 180120

Summary: REGRESSION(r218064): [GTK] Broke entering fullscreen mode in debug builds
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, bugs-noreply, cgarcia, commit-queue, ews-watchlist, gustavo, mcatanzaro
Priority: P2    
Version: Other   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Michael Catanzaro
Reported 2017-11-28 17:37:41 PST
r218064 added a bad assert when entering fullscreen mode. It just crashes now (in debug builds).
Attachments
Patch (1.51 KB, patch)
2017-11-28 17:40 PST, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Patch (1.96 KB, patch)
2017-11-28 17:50 PST, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Patch (2.03 KB, patch)
2017-11-29 08:14 PST, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Michael Catanzaro
Comment 1 2017-11-28 17:40:07 PST
Created attachment 327809 [details] Patch
EWS Watchlist
Comment 2 2017-11-28 17:41:31 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Michael Catanzaro
Comment 3 2017-11-28 17:50:40 PST
Created attachment 327811 [details] Patch
Carlos Garcia Campos
Comment 4 2017-11-28 23:04:10 PST
Comment on attachment 327811 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327811&action=review > Source/WebKit/ChangeLog:8 > + Remove the bad assertions. Why not fix them instead? > Source/WebKit/ChangeLog:10 > + This should hopefully fix /webkit2/WebKitWebView/fullscreen in debug mode. Hopefully? Could you check it before submitting a new patch? > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:-1308 > - ASSERT(priv->fullScreenModeActive); > - This is only called by the page client when not in fullscreen mode, the intention of the assert was to ensure that was always the case, but for some reason I added the assert checking the opposite. So, the actual fix is changing this assert to ASSERT(!priv->fullScreenModeActive); > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:-1326 > - ASSERT(!priv->fullScreenModeActive); > - Same here, we want to ensure we leave fullscreen while we are in fullscreen so the right assert is: ASSERT(priv->fullScreenModeActive);
Michael Catanzaro
Comment 5 2017-11-29 08:13:42 PST
OK, I'll swap the assertions. That works. > > Source/WebKit/ChangeLog:10 > > + This should hopefully fix /webkit2/WebKitWebView/fullscreen in debug mode. > > Hopefully? Could you check it before submitting a new patch? I thought I would need a rebuild to run API tests, but I actually don't. It works.
Michael Catanzaro
Comment 6 2017-11-29 08:14:59 PST
Created attachment 327853 [details] Patch
WebKit Commit Bot
Comment 7 2017-11-29 09:06:35 PST
The commit-queue encountered the following flaky tests while processing attachment 327853 [details]: editing/spelling/spellcheck-async.html bug 160571 (authors: g.czajkowski@samsung.com and mark.lam@apple.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 8 2017-11-29 09:07:01 PST
Comment on attachment 327853 [details] Patch Clearing flags on attachment: 327853 Committed r225267: <https://trac.webkit.org/changeset/225267>
WebKit Commit Bot
Comment 9 2017-11-29 09:07:03 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.