[webkit-reviews] review granted: [Bug 78928] Full Screen Refactor Part 3: Animate into Full Screen mode using new animation classes. : [Attachment 128533] 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 78928: Full Screen Refactor Part 3: Animate into Full Screen mode using new
animation classes.
https://bugs.webkit.org/show_bug.cgi?id=78928

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

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


> Source/WebKit2/ChangeLog:8
> +	   Boilerplate changes to WebKit2 XPC messages and supporting
functions.

These aren't XPC messages, they're IPC messages.

> Source/WebCore/platform/mac/WebFullScreenWindow.h:29
> + at interface WebFullScreenWindow : NSWindow

For WebCore Objective-C classes we use the WebCore prefix, so this should be
WebCoreFullScreenWindow (and the files should be named accordingly).

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:273
> +- (void)beganExitFullScreenWithInitialFrame:(WebCore::IntRect)initialFrame
finalFrame:(WebCore::IntRect)finalFrame

These should take const references to WebCore::IntRect.

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:311
> -	   return;
> +	       return;

Weird indentation here.

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:316
> +    NSDisableScreenUpdates();

Please add a comment indicating where this disable is balanced by an enable.

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:505
> +	  
_backgroundWindow.adoptNS(createBackgroundFullscreenWindow(screenFrame));

You can just use the new adoptNS function here:

backgroundWindow = adoptNS(createBackgroundFullscreenWindow(screenFrame));

or you could just make createBackgroundFullscreenWindow return a
RetainPtr<NSWindow>

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:515
> +    _fadeAnimation.adoptNS([[WebWindowFadeAnimation alloc]
initWithDuration:duration 

Same comment about adoptNS here.

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:532
> +    NSEnableScreenUpdates();

Please add a  comment indicating which NSDisableScreenUpdates call is balanced
by.

> Source/WebKit2/UIProcess/mac/WebFullScreenManagerProxyMac.mm:61
> +void WebFullScreenManagerProxy::beganEnterFullScreen(const WebCore::IntRect&
initialFrame, const WebCore::IntRect& finalFrame)

I don't think you need WebCore:: here (if you do, just add using namespace
WebCore to the top of the source file).

> Source/WebKit2/UIProcess/mac/WebFullScreenManagerProxyMac.mm:67
> +void WebFullScreenManagerProxy::beganExitFullScreen(const WebCore::IntRect&
initialFrame, const WebCore::IntRect& finalFrame)

Ditto.