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
Jer Noble
2012-02-17 13:38:09 PST
Created attachment 127871 [details]
Patch
(Mac build error is due to missing files from the prerequisite bug #78926.) Created attachment 128536 [details]
Patch
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 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. (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! Committed r110216: <http://trac.webkit.org/changeset/110216> |