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+

Jer Noble
Reported 2012-02-17 13:38:09 PST
As in bug #78928, animate into and out of full-screen mode with new animation classes.
Attachments
Patch (51.01 KB, patch)
2012-02-20 16:58 PST, Jer Noble
no flags
Patch (51.59 KB, patch)
2012-02-23 12:23 PST, Jer Noble
andersca: review+
Jer Noble
Comment 1 2012-02-20 16:58:22 PST
Jer Noble
Comment 2 2012-02-21 10:29:18 PST
(Mac build error is due to missing files from the prerequisite bug #78926.)
Jer Noble
Comment 3 2012-02-23 12:23:39 PST
WebKit Commit Bot
Comment 4 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.
Anders Carlsson
Comment 5 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.
Jer Noble
Comment 6 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!
Jer Noble
Comment 7 2012-03-08 15:17:23 PST
Note You need to log in before you can comment on or make changes to this bug.