RESOLVED FIXED192774
Flicker when exiting element fullscreen. 
https://bugs.webkit.org/show_bug.cgi?id=192774
Summary Flicker when exiting element fullscreen.
Jeremy Jones
Reported 2018-12-17 13:35:33 PST
Flicker when exiting element fullscreen.
Attachments
Patch (6.63 KB, patch)
2018-12-17 13:40 PST, Jeremy Jones 		no flags
DetailsFormatted DiffDiff
Patch (6.91 KB, patch)
2018-12-20 11:11 PST, Jeremy Jones 		jer.noble: review+
DetailsFormatted DiffDiff
Patch for landing. (7.63 KB, patch)
2018-12-20 15:16 PST, Jeremy Jones 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Jeremy Jones
Comment 1 2018-12-17 13:36:57 PST
rdar://problem/33088878
Jeremy Jones
Comment 2 2018-12-17 13:40:47 PST
Created attachment 357472 [details] Patch
Simon Fraser (smfr)
Comment 3 2018-12-17 14:43:25 PST
Comment on attachment 357472 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357472&action=review > Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm:471 > + webViewContents = createImageWithCopiedData(webViewContents.get()); Why is this necessary? > Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm:479 > + [CATransaction commit]; > + [CATransaction flush]; > + > + [CATransaction begin]; > + [CATransaction setDisableActions:YES]; Do we need to start a second transaction? > Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm:526 > + [_exitPlaceholder removeFromSuperview]; > + [[_exitPlaceholder layer] setContents:nil]; I think it would be cleaner to just make a new _exitPlaceholder each time.
Jeremy Jones
Comment 4 2018-12-20 10:27:10 PST
Comment on attachment 357472 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357472&action=review >> Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm:471 >> + webViewContents = createImageWithCopiedData(webViewContents.get()); > > Why is this necessary? Copied explanatory comment from the other place this function is used. >> Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm:479 >> + [CATransaction setDisableActions:YES]; > > Do we need to start a second transaction? I believe so, since we only want to setDisableActions for this group of changes. >> Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm:526 >> + [[_exitPlaceholder layer] setContents:nil]; > > I think it would be cleaner to just make a new _exitPlaceholder each time. Done.
Jeremy Jones
Comment 5 2018-12-20 11:11:26 PST
Created attachment 357835 [details] Patch
Simon Fraser (smfr)
Comment 6 2018-12-20 11:40:58 PST
Comment on attachment 357835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357835&action=review > Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm:143 > - > + Whitespace. > Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm:464 > + RetainPtr<CGImageRef> webViewContents = adoptCF(CGWindowListCreateImage(NSRectToCGRect(exitPlaceholderScreenRect), kCGWindowListOptionIncludingWindow, [[_webView window] windowNumber], kCGWindowImageDefault)); Why not CGSHWCaptureWindowList() like the takeWindowSnapshot() code? > Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm:468 > + // Using the returned CGImage directly would result in calls to the WindowServer every time > + // the image was painted. Instead, copy the image data into our own process to eliminate that > + // future overhead. Is this really true?
Jeremy Jones
Comment 7 2018-12-20 15:10:04 PST
(In reply to Simon Fraser (smfr) from comment #6) > Comment on attachment 357835 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357835&action=review > > > Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm:143 > > - > > + > > Whitespace. > > > Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm:464 > > + RetainPtr<CGImageRef> webViewContents = adoptCF(CGWindowListCreateImage(NSRectToCGRect(exitPlaceholderScreenRect), kCGWindowListOptionIncludingWindow, [[_webView window] windowNumber], kCGWindowImageDefault)); > > Why not CGSHWCaptureWindowList() like the takeWindowSnapshot() code? I've switched to this method. > > > Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm:468 > > + // Using the returned CGImage directly would result in calls to the WindowServer every time > > + // the image was painted. Instead, copy the image data into our own process to eliminate that > > + // future overhead. > > Is this really true? No longer relevant since switching to CGSHWCaptureWindowList()
Jeremy Jones
Comment 8 2018-12-20 15:16:19 PST
Created attachment 357877 [details] Patch for landing.
WebKit Commit Bot
Comment 9 2018-12-20 16:27:58 PST
Comment on attachment 357877 [details] Patch for landing. Clearing flags on attachment: 357877 Committed r239475: <https://trac.webkit.org/changeset/239475>
Frédéric Wang Nélar
Comment 10 2026-10-13 04:39:26 PDT
This landed as https://commits.webkit.org/207514@main but the bug was not closed.
Note You need to log in before you can comment on or make changes to this bug.