Bug 192774 – Flicker when exiting element fullscreen.

• Status: NEW
• Component: Media

[Jeremy Jones]

Flicker when exiting element fullscreen.

[Jeremy Jones]

rdar://problem/33088878

[Jeremy Jones]

Created attachment 357472 [details]
Patch

[Simon Fraser (smfr)]

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 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]

Created attachment 357835 [details]
Patch

[Simon Fraser (smfr)]

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]

(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]

Created attachment 357877 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 357877 [details]
Patch for landing.

Clearing flags on attachment: 357877

Committed r239475: <https://trac.webkit.org/changeset/239475>