Bug 78928 – Full Screen Refactor Part 3: Animate into Full Screen mode using new animation classes.

• Component: WebKit2
• Status: RESOLVED FIXED

[Jer Noble]

Use the new WKFadeAnimation and WKScaleAnimation classes to animate into and out of full-screen mode.

[Jer Noble]

Created attachment 127847 [details]
Patch

[Jer Noble]

Created attachment 127855 [details]
Patch

Moved WKFullScreenWindow into WebCore to be shared by WebKit2 and WebKit.

[Jer Noble]

Created attachment 127859 [details]
Patch

Accidentally left in a portion of 'Part 4.5' in the last patch.

[Jer Noble]

Created attachment 128533 [details]
Patch

Added the full set of WKSI p libraries.

[Anders Carlsson]

Comment on attachment 128533 [details]
Patch

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
> +@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.

[Jer Noble]

(In reply to comment #5)
> (From update of attachment 128533 [details])
> 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.

Sorry, I'll fix that.

> > Source/WebCore/platform/mac/WebFullScreenWindow.h:29
> > +@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).

Changed.

> > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:273
> > +- (void)beganExitFullScreenWithInitialFrame:(WebCore::IntRect)initialFrame finalFrame:(WebCore::IntRect)finalFrame
> 
> These should take const references to WebCore::IntRect.

I avoided doing that in case WKFullScreenWindowController.h was included by a non-dot-mm file, but that shouldn't ever be the case.  Changed.

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

That is weird.  Changed.

> > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:316
> > +    NSDisableScreenUpdates();
> 
> Please add a comment indicating where this disable is balanced by an enable.

Sure thing.  Added.

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

I like the last option.  Changed.

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

Same resolution. :)

> > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:532
> > +    NSEnableScreenUpdates();
> 
> Please add a  comment indicating which NSDisableScreenUpdates call is balanced by.

Added.

> > 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).

Sure thing.  Changed.

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

Ditto.  Thanks!

[Jer Noble]

Committed r110215: <http://trac.webkit.org/changeset/110215>