Bug 78930

Summary: Full Screen Refactor Part 4: Animate into Full Screen mode using new animation classes, WebKit edition.
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: WebKit Misc.Assignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, tkent
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 78926, 78928    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch andersca: review+

Description Jer Noble 2012-02-17 13:38:09 PST 
As in bug #78928, animate into and out of full-screen mode with new animation classes.
Comment 1 Jer Noble 2012-02-20 16:58:22 PST 
Created attachment 127871 [details]
Patch
Comment 2 Jer Noble 2012-02-21 10:29:18 PST 
(Mac build error is due to missing files from the prerequisite bug #78926.)
Comment 3 Jer Noble 2012-02-23 12:23:39 PST 
Created attachment 128536 [details]
Patch
Comment 4 WebKit Commit Bot 2012-02-24 01:59:47 PST 
Attachment 128536 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/rendering/RenderBlock.cpp:5834:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Anders Carlsson 2012-03-07 22:47:19 PST 
Comment on attachment 128536 [details]
Patch

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.
Comment 6 Jer Noble 2012-03-08 14:09:49 PST 
(In reply to comment #5)
> (From update of attachment 128536 [details])
> 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.

Sure thing.  Changed.

> > Source/WebKit/mac/WebView/WebFullScreenController.mm:200
> > +    NSDisableScreenUpdates();
> 
> Please add a comment indicating where this is balanced.

Added.

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

You're right.  Changed. :)

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

Woo!

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

You're right, I just copied the comment wholesale from WK2.  I'll redact the part about WK2.

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

Changed.

> > Source/WebKit/mac/WebView/WebFullScreenController.mm:471
> > +        _backgroundWindow.adoptNS(createBackgroundFullscreenWindow(screenFrame));
> 
> Same comment here as in part 3.

Same resolution.

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

Changed.

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

Changed.

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

Yup. Changed.

> > Source/WebKit/mac/WebView/WebFullScreenController.mm:519
> > +        [_fadeAnimation.get() setWindow:nil];
> 
> do you want to null out the animation here?

It gets nulled out on the next line (with the = adoptNS), so I didn't bother.

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

Changed.

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

Sure thing, Added.

Thanks!
Comment 7 Jer Noble 2012-03-08 15:17:23 PST 
Committed r110216: <http://trac.webkit.org/changeset/110216>