Flicker when exiting element fullscreen.
rdar://problem/33088878
Created attachment 357472 [details] Patch
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.
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.
Created attachment 357835 [details] Patch
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?
(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()
Created attachment 357877 [details] Patch for landing.
Comment on attachment 357877 [details] Patch for landing. Clearing flags on attachment: 357877 Committed r239475: <https://trac.webkit.org/changeset/239475>