[webkit-reviews] review granted: [Bug 78930] Full Screen Refactor Part 4: Animate into Full Screen mode using new animation classes, WebKit edition. : [Attachment 128536] Patch

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 78930: Full Screen Refactor Part 4: Animate into Full Screen mode using new
animation classes, WebKit edition.
https://bugs.webkit.org/show_bug.cgi?id=78930

Attachment 128536: Patch
https://bugs.webkit.org/attachment.cgi?id=128536&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=128536&action=review


> Source/WebKit/mac/WebView/WebFullScreenController.mm:198
> +    RetainPtr<CGImageRef> webViewContents(AdoptCF,
CGWindowListCreateImage(NSRectToCGRect(webViewFrame),
kCGWindowListOptionIncludingWindow, windowID, kCGWindowImageShouldBeOpaque));

You can just use the new adoptCF functino here.

> Source/WebKit/mac/WebView/WebFullScreenController.mm:200
> +    NSDisableScreenUpdates();

Please add a comment indicating where this is balanced.

> Source/WebKit/mac/WebView/WebFullScreenController.mm:210
> +	   _webViewPlaceholder.adoptNS([[NSImageView alloc] init]);

adoptNS function. I'm also not sure if this needs to be an NSImageView - it
looks like it could just be a plain ol' NSView. (This comment also applies to
part 3 :)

> Source/WebKit/mac/WebView/WebFullScreenController.mm:281
> +    // Screen updates to be re-enabled in
beganExitFullScreenWithInitialFrame:finalFrame:

This i like!

> Source/WebKit/mac/WebView/WebFullScreenController.mm:358
> +    // We are being asked to close rapidly, most likely because the page 
> +    // has closed or the web process has crashed.  Just walk through our
> +    // normal exit full screen sequence, but don't wait to be called back
> +    // in response.

The part about the web process is false in WK1.

> Source/WebKit/mac/WebView/WebFullScreenController.mm:456
> +    _scaleAnimation.adoptNS([[WebWindowScaleAnimation alloc]
initWithHintedDuration:duration window:[self window]
initalFrame:initialWindowFrame finalFrame:screenFrame]);

= adoptNS.

> Source/WebKit/mac/WebView/WebFullScreenController.mm:471
> +	  
_backgroundWindow.adoptNS(createBackgroundFullscreenWindow(screenFrame));

Same comment here as in part 3.

> Source/WebKit/mac/WebView/WebFullScreenController.mm:482
> +    _fadeAnimation.adoptNS([[WebWindowFadeAnimation alloc]
initWithDuration:duration 

= adoptNS.

> Source/WebKit/mac/WebView/WebFullScreenController.mm:503
> +    _scaleAnimation.adoptNS([[WebWindowScaleAnimation alloc]
initWithHintedDuration:duration window:[self window] initalFrame:currentFrame
finalFrame:initialWindowFrame]);

= adoptNS.

> Source/WebKit/mac/WebView/WebFullScreenController.mm:511
> +	  
_backgroundWindow.adoptNS(createBackgroundFullscreenWindow(screenFrame));

= adoptNS. (or fix createBackgroundFullscreenWindow).

> Source/WebKit/mac/WebView/WebFullScreenController.mm:519
> +	   [_fadeAnimation.get() setWindow:nil];

do you want to null out the animation here?

> Source/WebKit/mac/WebView/WebFullScreenController.mm:521
> +    _fadeAnimation.adoptNS([[WebWindowFadeAnimation alloc]
initWithDuration:duration 

= adoptNS.

> Source/WebKit/mac/WebView/WebFullScreenController.mm:538
> +    NSEnableScreenUpdates();

Comment.