Bug 180120 - REGRESSION(r218064): [GTK] Broke entering fullscreen mode in debug builds
Summary: REGRESSION(r218064): [GTK] Broke entering fullscreen mode in debug builds
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-11-28 17:37 PST by Michael Catanzaro
Modified: 2017-11-29 09:07 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.51 KB, patch)
2017-11-28 17:40 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (1.96 KB, patch)
2017-11-28 17:50 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.03 KB, patch)
2017-11-29 08:14 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2017-11-28 17:37:41 PST
r218064 added a bad assert when entering fullscreen mode. It just crashes now (in debug builds).
Comment 1 Michael Catanzaro 2017-11-28 17:40:07 PST
Created attachment 327809 [details]
Patch
Comment 2 EWS Watchlist 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
Comment 3 Michael Catanzaro 2017-11-28 17:50:40 PST
Created attachment 327811 [details]
Patch
Comment 4 Carlos Garcia Campos 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);
Comment 5 Michael Catanzaro 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.
Comment 6 Michael Catanzaro 2017-11-29 08:14:59 PST
Created attachment 327853 [details]
Patch
Comment 7 WebKit Commit Bot 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.
Comment 8 WebKit Commit Bot 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>
Comment 9 WebKit Commit Bot 2017-11-29 09:07:03 PST
All reviewed patches have been landed.  Closing bug.