Bug 78928 - Full Screen Refactor Part 3: Animate into Full Screen mode using new animation classes.
Summary: Full Screen Refactor Part 3: Animate into Full Screen mode using new animatio...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on: 78926
Blocks: 78929 78930
  Show dependency treegraph
 
Reported: 2012-02-17 13:33 PST by Jer Noble
Modified: 2012-03-08 15:17 PST (History)
0 users

See Also:


Attachments
Patch (57.67 KB, patch)
2012-02-20 13:52 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (59.25 KB, patch)
2012-02-20 15:24 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (56.07 KB, patch)
2012-02-20 15:29 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (439.57 KB, patch)
2012-02-23 12:20 PST, Jer Noble
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2012-02-17 13:33:47 PST
Use the new WKFadeAnimation and WKScaleAnimation classes to animate into and out of full-screen mode.
Comment 1 Jer Noble 2012-02-20 13:52:41 PST
Created attachment 127847 [details]
Patch
Comment 2 Jer Noble 2012-02-20 15:24:02 PST
Created attachment 127855 [details]
Patch

Moved WKFullScreenWindow into WebCore to be shared by WebKit2 and WebKit.
Comment 3 Jer Noble 2012-02-20 15:29:30 PST
Created attachment 127859 [details]
Patch

Accidentally left in a portion of 'Part 4.5' in the last patch.
Comment 4 Jer Noble 2012-02-23 12:20:46 PST
Created attachment 128533 [details]
Patch

Added the full set of WKSI p libraries.
Comment 5 Anders Carlsson 2012-03-07 22:41:23 PST
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.
Comment 6 Jer Noble 2012-03-08 13:29:39 PST
(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!
Comment 7 Jer Noble 2012-03-08 15:17:09 PST
Committed r110215: <http://trac.webkit.org/changeset/110215>