Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
846443004:
mac: Implement custom AppKit Enter Fullscreen transition. (Closed)
|
Created:
5 years, 11 months ago by erikchen Modified:
5 years, 11 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, jam, penghuang+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmac: Implement custom AppKit Enter Fullscreen transition.
AppKit's default Fullscreen transition cross-fades between two static images: a
snapshot of the window before the transition began, and a snapshot of the
window resized to the full size of the screen. This has the problem that
frequently, when AppKit takes the snapshot of the final state of the window,
the web contents view has not yet drawn any content in the new size. The
animation itself looks weird, and then there's a flash when the animation
finishes where the snapshot of the old web content is replaced by the new web
content.
This custom fullscreen transition is very similar, except instead of taking a
snapshot of the window in its final state, the transition uses a live version
of the window in its final state. See the associated bug for more details about
the implementation.
BUG=414527
Committed: https://crrev.com/a48cbbba0075b7a7c135605ab576bad2cfa5a1b3
Cr-Commit-Position: refs/heads/master@{#311717}
Patch Set 1 : #
Total comments: 18
Patch Set 2 : Comments from andresantoso. #Patch Set 3 : Comments from andresantoso, round 2. #
Total comments: 24
Patch Set 4 : Comments from rsesek. #Patch Set 5 : Fix several secondary screen bugs. #Patch Set 6 : Comments from rsesek, round 2. #
Messages
Total messages: 29 (11 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
erikchen@chromium.org changed reviewers: + andresantoso@chromium.org
andresantoso: Please review.
https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.h (right): https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.h:53: // initWithWindow:window]; This is only sample code, but it's a leak without ARC. https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm (right): https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:186: positionAnimation.toValue = [NSValue valueWithPoint:NSMakePoint(0, 0)]; NSZeroPoint https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:202: opacityAnimation.toValue = @(0); @(0) is like -numberWithInt:, so this should be @(0.0) for -numberWithDouble: to avoid a conversion. Same with @(1) below. https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:234: CGFloat yScale = initialFrame.size.height / endFrame.size.height; Use NSWidth and NSHeight for consistency. https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:250: CGPointMake(CGRectGetMidX(endFrame), CGRectGetMidY(endFrame)); Mixing CG and NS works ok on 64-bits, but it would be more correct to stay in NSPoint/NSRect and use NSMidX/NSMidY. Reads better too. https://codereview.chromium.org/846443004/diff/140001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/846443004/diff/140001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:307: 'browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm', Sort before browser_window_factory_cocoa
andresantoso: PTAL https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.h (right): https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/12 06:10:03, Andre wrote: > 2015 Done. https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.h:53: // initWithWindow:window]; On 2015/01/12 06:10:03, Andre wrote: > This is only sample code, but it's a leak without ARC. I added an autorelease. There's no way to determine or enforce whether the calling code compiles with ARC, and there's no ARC-agnostic way to write the sample code, so I wrote the sample code in ARC because it saved some characters. https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm (right): https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/12 06:10:03, Andre wrote: > 2015 Done. https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:186: positionAnimation.toValue = [NSValue valueWithPoint:NSMakePoint(0, 0)]; On 2015/01/12 06:10:03, Andre wrote: > NSZeroPoint Done. https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:202: opacityAnimation.toValue = @(0); On 2015/01/12 06:10:03, Andre wrote: > @(0) is like -numberWithInt:, so this should be @(0.0) for -numberWithDouble: to > avoid a conversion. > Same with @(1) below. Done. https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:234: CGFloat yScale = initialFrame.size.height / endFrame.size.height; On 2015/01/12 06:10:03, Andre wrote: > Use NSWidth and NSHeight for consistency. Done. https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:250: CGPointMake(CGRectGetMidX(endFrame), CGRectGetMidY(endFrame)); On 2015/01/12 06:10:03, Andre wrote: > Mixing CG and NS works ok on 64-bits, but it would be more correct to stay > in NSPoint/NSRect and use NSMidX/NSMidY. > Reads better too. You're right. Changed. https://codereview.chromium.org/846443004/diff/140001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/846443004/diff/140001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:307: 'browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm', On 2015/01/12 06:10:03, Andre wrote: > Sort before browser_window_factory_cocoa Done. (refactor error).
LGTM https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm (right): https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:234: CGFloat yScale = initialFrame.size.height / endFrame.size.height; On 2015/01/12 19:18:58, erikchen wrote: > On 2015/01/12 06:10:03, Andre wrote: > > Use NSWidth and NSHeight for consistency. > > Done. Forgot to change this?
https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm (right): https://codereview.chromium.org/846443004/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:234: CGFloat yScale = initialFrame.size.height / endFrame.size.height; On 2015/01/12 19:27:20, Andre wrote: > On 2015/01/12 19:18:58, erikchen wrote: > > On 2015/01/12 06:10:03, Andre wrote: > > > Use NSWidth and NSHeight for consistency. > > > > Done. > > Forgot to change this? Oops! Done.
erikchen@chromium.org changed reviewers: + rsesek@chromium.org
rsesek: Please review.
https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm (right): https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:166: CALayer* rootLayer = [self rootLayerOfWindow:_primaryWindow]; As andre pointed out, we can save a couple of lines of code by setting the alpha of the window to zero here.
Does this not change the exit fullscreen animation? https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.h (right): https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.h:8: #import <AppKit/AppKit.h> Cocoa.h will import both of these. https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm (right): https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:29: base::scoped_nsobject<NSWindow> _primaryWindow; No other code in Chromium uses this style, and switching to leading underscores has not been discussed. So this should be consistent with existing code in the area. If you are interested in using leading underscores in new code (no real advantage here, since there's no use of synthesized properties), then this should be discussed on chromium-dev. https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:49: // Whether |_primaryWindow| was opaque before the transition began. Is this ever variable for Chromium windows? Or is this done just for thoroughness? https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:132: [_snapshotLayer setContents:static_cast<id>(windowSnapshot.get())]; Why does this need a cast? https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:150: [_snapshotLayer setFrame:[_primaryWindow frame]]; DCHEC(_snapshotLayer) ? https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:157: [[_primaryWindow backgroundColor] retain]); copy may be better here https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:163: // resized. Why? Prevent -drawRect: calls? https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:213: [group setValue:kSnapshotWindowAnimationID forKey:kAnimationIDKey]; What are these animation IDs used for? Can't you just compare the pointer value to the one held in the scoper for -animationDidEnd: ? https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:221: // As soon as the window is downsized, the opacity should be set back to 1. "Downsize" ? https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:241: transformAnimation.timingFunction = [CAMediaTimingFunction Can you not set the timing function on the group?
rsesek: PTAL This CL does not change the exit fullscreen animation. That will come in a follow up CL. I manually tested this CL on OSX 10.9 and OSX 10.10, with both single and dual monitors, and with "Displays have separate spaces" options both on and off. In the process, I discovered 2 bugs, which I fixed in patch set 5. In summary: - Using screen coordinates where window coordinates were expected (2 instances). - The custom enter fullscreen animation doesn't work well in conjunction with an AppKit bug. (https://code.google.com/p/chromium/issues/detail?id=396980#c45) Patch set 4 contains my responses to your comments. https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.h (right): https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.h:8: #import <AppKit/AppKit.h> On 2015/01/12 22:05:27, Robert Sesek wrote: > Cocoa.h will import both of these. Cocoa.h also includes CoreData.h, which is unnecessary. A search shows that it is much more common to include Cocoa.h than AppKit.h in the Chromium code base, so I've done as you've suggested. https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm (right): https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:29: base::scoped_nsobject<NSWindow> _primaryWindow; On 2015/01/12 22:05:27, Robert Sesek wrote: > No other code in Chromium uses this style, and switching to leading underscores > has not been discussed. So this should be consistent with existing code in the > area. If you are interested in using leading underscores in new code (no real > advantage here, since there's no use of synthesized properties), then this > should be discussed on chromium-dev. I have changed all the ivars to use trailing underscores. I will consider bringing this subject up on chromium-dev. https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:49: // Whether |_primaryWindow| was opaque before the transition began. On 2015/01/12 22:05:27, Robert Sesek wrote: > Is this ever variable for Chromium windows? Or is this done just for > thoroughness? This is never variable for BrowserWindow. This ivar exists for thoroughness. https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:132: [_snapshotLayer setContents:static_cast<id>(windowSnapshot.get())]; On 2015/01/12 22:05:27, Robert Sesek wrote: > Why does this need a cast? The property |contents| has type id. CGImageRef is a pointer to a C struct. This might make you think that this cast shouldn't be allowed at all, but it's explicitly allowed by the documentation of CALayer. """ If you are using the layer to display a static image, you can set this property to the CGImageRef containing the image you want to display. """ https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:150: [_snapshotLayer setFrame:[_primaryWindow frame]]; On 2015/01/12 22:05:27, Robert Sesek wrote: > DCHEC(_snapshotLayer) ? Done. https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:157: [[_primaryWindow backgroundColor] retain]); On 2015/01/12 22:05:27, Robert Sesek wrote: > copy may be better here Could you please explain the reasoning behind your comment? NSColor is immutable, so I don't see the benefit of making a copy rather than just retaining the result. https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:163: // resized. On 2015/01/12 22:05:27, Robert Sesek wrote: > Why? Prevent -drawRect: calls? That is correct. I've updated the documentation. https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:166: CALayer* rootLayer = [self rootLayerOfWindow:_primaryWindow]; On 2015/01/12 19:50:45, erikchen wrote: > As andre pointed out, we can save a couple of lines of code by setting the alpha > of the window to zero here. Unfortunately, setting the window's alpha to zero does not have the desired effect, since the goal is to have the window's frame be the full size of the window (and invisible), but the root layer to be scaled down and visible. https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:213: [group setValue:kSnapshotWindowAnimationID forKey:kAnimationIDKey]; On 2015/01/12 22:05:27, Robert Sesek wrote: > What are these animation IDs used for? Can't you just compare the pointer value > to the one held in the scoper for -animationDidEnd: ? The CAAnimation* passed to -animationDidEnd: is neither pointer equivalent nor isEqual: equivalent to the animation held in _snapshotAnimation. This is the least painful way of identifying the animation. https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:221: // As soon as the window is downsized, the opacity should be set back to 1. On 2015/01/12 22:05:27, Robert Sesek wrote: > "Downsize" ? I don't understand your question, so I've addressed both of my possible interpretations. 1. The window itself is not being scaled down, it's the window's root layer. I've updated the comment. 2. The word "downsize" might be confusing. I've changed it to "scaled down", although google seems to think that "downsize" is a reasonable word. https://www.google.com/webhp?sourceid=chrome-instant&ion=1&espv=2&es_th=1&ie=... https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:241: transformAnimation.timingFunction = [CAMediaTimingFunction On 2015/01/12 22:05:27, Robert Sesek wrote: > Can you not set the timing function on the group? I can. I have done so.
LGTM Sorry for the delay; was at an offsite this week. https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm (right): https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:157: [[_primaryWindow backgroundColor] retain]); On 2015/01/13 02:51:28, erikchen wrote: > On 2015/01/12 22:05:27, Robert Sesek wrote: > > copy may be better here > > Could you please explain the reasoning behind your comment? NSColor is > immutable, so I don't see the benefit of making a copy rather than just > retaining the result. NSColor is a value object that implements NSCopying. If this is a special system color, I don't think there's a guarantee that it is immutable.
https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm (right): https://codereview.chromium.org/846443004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_enter_fullscreen_transition.mm:157: [[_primaryWindow backgroundColor] retain]); On 2015/01/15 01:36:54, Robert Sesek wrote: > On 2015/01/13 02:51:28, erikchen wrote: > > On 2015/01/12 22:05:27, Robert Sesek wrote: > > > copy may be better here > > > > Could you please explain the reasoning behind your comment? NSColor is > > immutable, so I don't see the benefit of making a copy rather than just > > retaining the result. > > NSColor is a value object that implements NSCopying. If this is a special system > color, I don't think there's a guarantee that it is immutable. Ah, you're right. Done.
erikchen@chromium.org changed reviewers: + mark@chromium.org
mark: Looking for an OWNER review of base/mac/sdk_forward_declarations.h.
LGTM on that file.
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/846443004/240001
Message was sent while issue was closed.
Committed patchset #6 (id:240001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a48cbbba0075b7a7c135605ab576bad2cfa5a1b3 Cr-Commit-Position: refs/heads/master@{#311717}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:240001) has been created in https://codereview.chromium.org/848153005/ by maniscalco@chromium.org. The reason for reverting is: Trying to track down a sizes failure. Speculatively reverting. Will re-apply if not the cause..
Message was sent while issue was closed.
On 2015/01/15 23:32:46, maniscalco wrote: > A revert of this CL (patchset #6 id:240001) has been created in > https://codereview.chromium.org/848153005/ by mailto:maniscalco@chromium.org. > > The reason for reverting is: Trying to track down a sizes failure. > Speculatively reverting. Will re-apply if not the cause.. Bug for sizes failure: https://code.google.com/p/chromium/issues/detail?id=449333 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
