Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1276383004:
Implemented fullscreen exit animation with AppKit (Closed)
DescriptionImplement a smoother animation for exiting full screen on OSX 10.10 with AppKit.
Modified BrowserWindowEnterFullscreenTransition class so that it contains the
animations for entering and exiting fullscreen.
Added a lock and overrode some methods in FramedBrowserWindow to prevent AppKit
from changing the style mask and the size of the window before the end of the
exit animation
A method is added to FullSizeWindow to allow us to force the
contentView to resize
BUG=519735
Committed: https://crrev.com/6adb1bc1f87fbbbba19b2950d48a48f67eb87d23
Cr-Commit-Position: refs/heads/master@{#343426}
Patch Set 1 #Patch Set 2 : Fixed some whitespace issues #
Total comments: 1
Patch Set 3 : Removed unnecessary changes int browser_window_controller_private #Patch Set 4 : Fixed format issues #
Total comments: 73
Patch Set 5 : Added UMA metrics for overrides and fixed some issues according to review #Patch Set 6 : Move some layout logic into FullScreen Transition #Patch Set 7 : Removed unwanted change in a browser_window_layout #Patch Set 8 : Refactored the code to log UMA metrics #
Total comments: 34
Patch Set 9 : Remove histogram changes #Patch Set 10 : Fixing some comments and etc #Patch Set 11 : Fixed comments in browser_window_fullscreen_transition.h #Patch Set 12 : Fix more comments #Patch Set 13 : Fix comments #Patch Set 14 : Fixed some issue #Patch Set 15 : Add missing period #Patch Set 16 : Made some changes according to missed comments on Patch #
Total comments: 23
Patch Set 17 : More edits #Patch Set 18 : Made some changes to FullSizeWindow #Patch Set 19 : Improved some comments #Patch Set 20 : Fix some unwrapped comments #Patch Set 21 : Fixed comment wrappings and periods #Patch Set 22 : Fix for comment wrappings and periods. #Patch Set 23 : #Patch Set 24 : Fix with comment format #
Total comments: 18
Patch Set 25 : Fixed some issues #Patch Set 26 : Fix method signature #
Total comments: 6
Patch Set 27 : Fixed FrameAndStyleLock according to C++ style guide #Patch Set 28 : Removed extra include #Messages
Total messages: 40 (11 generated)
spqchan@google.com changed reviewers: + erikchen@chromium.org
https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (left): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:709: enterFullscreenTransition_.reset(); did you mean to replace with fullscreenTransition_? https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:778: if (exitingAppKitFullscreen_) { Is it possible for this to be false? If not, you should use a DCHECK. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:789: FramedBrowserWindow* framedWindow = (FramedBrowserWindow*)[self window]; We don't use C style casts. See other examples in this file for how we get the FramedBrowserWindow. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:896: [[[self window] contentView] bounds].size : [[self window] frame].size; Can you explain this logic? Also, can you move the sizing logic into the FullScreenTransition class, so that we don't have to expose this implementation detail to BrowserWindowController? e.g. if (exitingAppKitFullscreen_) fullscreenTransition getDesiredWindowSize https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:1126: if (![self shouldUseCustomAppKitFullscreenTransition]) { nit: The Chrome style is typically to only use {} if either the conditional or following block has more than 1 line. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:97: // Begins the animations used for the custom exit fullscreen transition. This comment doesn't apply to the exit fullscreen transition, but you added the word "exit" anyways. You'll want to do some comment fixing. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:118: whitespace https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:121: whitespace https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:59: // relative the the coordinates of its current screen s/the/to https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:85: // - Adds the style mask NSFullScreenWindowMask to the current window. Doesn't this only apply to the enter fullscreen transition? https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:93: - (void)setPrimaryWindowToFinalFrame; What does this method do? Comment please. The verb "set" is typically reserved for setters. Perhaps movePrimaryWindowToFinalFrame? https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:122: finalFrame_ = initialFrame_; It looks like you're trying to keep this class around between the enter/exit animations. This is intended to be a transitory class, if you need additional state, add it to the -init method. The -init method should also determine whether this is an enter or exit fullscreen transition. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:131: CGPointMake(screenOriginX, screenOriginY); This logic can be moved into initial setup, and then -customWindowsToExitFullScreenTransition and -customWindowsToEnterFullScreenTransition will have the same implementation. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:137: // and lock the primary window so that OSX can't make changes to it All method comments should be at the declarations, not definitions. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:140: (NSTimeInterval)duration { Given the way that you've organized this class, I would have expected a single method startFullscreenAnimationWithDuration: if (exiting fullscreen) do stuff <common logic> https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:142: (FullSizeContentView*)[primaryWindow_ contentView]; No C-style casts https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:143: [content forceFrameSize:finalFrame_.size]; At some point, don't you need to undo the forceFrameSize (or at least confirm that it's the right final size, just in case something has gone wrong?) https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:206: [snapshotWindow_ orderFront:nil]; why did you add this? Right now the snapshotWindow_ has orderFront: called twice. It should only be called once. Either here, or in the original location. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:207: NSRect finalScreenFrame = [snapshotWindow_ convertRectFromScreen:finalFrame_]; I don't understand this variable. Please add comments that describe what it's supposed to be/what it does. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:246: whitespace https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:254: opacityAnimation.duration = duration; why is this necessary? Shouldn't setting the group's duration be sufficient? https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:286: @[ opacityAnimation, positionAnimation, transformAnimation]; formatting (missing whitespace) https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:296: - (void)preparePrimaryWindowForAnimation { Why did you move the location of this method? I generally try to match the order of methods in the implementation file with the order of the declarations in the private category. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:336: // size to the expected frame at the end formatting of the comment. Also periods. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:339: [primaryWindow_ setFrameAndStyleLock:NO]; It's very, very important that we remember to call setFrameAndStyleLock:NO. Can you make a C++ scoped class along the lines of https://code.google.com/p/chromium/codesearch#chromium/src/base/scoped_generi... that can be manually reset here, but whose destructor guarantees that this is unset? https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_layout.mm (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_layout.mm:268: // DCHECK_LE(maxY, parameters_.contentViewSize.height + fullscreenYOffset_); huh? https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/framed_browser_window.h (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.h:39: // of the window to be changed when it's set to YES Grammar. This lock prevents the frame and style of the window from being changed. I understand "frame", but I don't understand "style". What do you mean by the "style" of a window? https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.h:52: - (void) setFrameAndStyleLock:(BOOL)lock; extra whitespace after (void) https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/framed_browser_window.mm (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.mm:105: // Overriding this method to prevent the OSX from automatically setting grammar. This method is overridden to prevent AppKit from changing the style mask when frameAndStyleLock_ is YES. Declare your overrides in the header, and move the comments there. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.mm:107: - (void)setStyleMask:(NSUInteger)styleMask { reformat this method to meet Chrome style guidelines. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.mm:109: return; This is, in general, a very dangerous operation. I'd feel a lot better if we could add some safety checks. e.g., if we expect to override one AppKit invocation, then upload UMA metrics if it happens more than once. ditto for below. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.mm:114: // Overriding this method to prevent the OSX from automatically setting ditto https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.mm:118: animate:(BOOL)animateFlag { reformat this method to meet Chrome style guidelines. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.mm:202: - (void) setFrameAndStyleLock:(BOOL)lock { run "git cl format" to clang-format your entire CL. clang-format has as couple of bugs, but it will catch all of these small nits. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/full_size_content_window.h (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/full_size_content_window.h:25: @interface FullSizeContentView : NSView You moved the @interface from the .mm to the .h, but failed to move the comment over. This makes it very difficult to tell what this class is used for, and what your subsequent method forceFrameSize: does.
https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (left): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:709: enterFullscreenTransition_.reset(); On 2015/08/10 18:06:24, erikchen wrote: > did you mean to replace with fullscreenTransition_? Done https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:778: if (exitingAppKitFullscreen_) { On 2015/08/10 18:06:24, erikchen wrote: > Is it possible for this to be false? If not, you should use a DCHECK. Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:789: FramedBrowserWindow* framedWindow = (FramedBrowserWindow*)[self window]; On 2015/08/10 18:06:24, erikchen wrote: > We don't use C style casts. See other examples in this file for how we get the > FramedBrowserWindow. Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:1126: if (![self shouldUseCustomAppKitFullscreenTransition]) { On 2015/08/10 18:06:24, erikchen wrote: > nit: The Chrome style is typically to only use {} if either the conditional or > following block has more than 1 line. Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:97: // Begins the animations used for the custom exit fullscreen transition. On 2015/08/10 18:06:25, erikchen wrote: > This comment doesn't apply to the exit fullscreen transition, but you added the > word "exit" anyways. You'll want to do some comment fixing. Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:118: On 2015/08/10 18:06:25, erikchen wrote: > whitespace Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:121: On 2015/08/10 18:06:25, erikchen wrote: > whitespace Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:59: // relative the the coordinates of its current screen On 2015/08/10 18:06:25, erikchen wrote: > s/the/to Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:143: [content forceFrameSize:finalFrame_.size]; On 2015/08/10 18:06:25, erikchen wrote: > At some point, don't you need to undo the forceFrameSize (or at least confirm > that it's the right final size, just in case something has gone wrong?) Once the NSWindow is resized, the content will also resize. The override actually prevents the content from shrinking by setting the content view size to windows.bounds.size I'll add the a confirmation. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:246: On 2015/08/10 18:06:25, erikchen wrote: > whitespace Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:246: On 2015/08/10 18:06:25, erikchen wrote: > whitespace Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:254: opacityAnimation.duration = duration; On 2015/08/10 18:06:25, erikchen wrote: > why is this necessary? Shouldn't setting the group's duration be sufficient? Removed it. This is dead code from an experiment. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:286: @[ opacityAnimation, positionAnimation, transformAnimation]; On 2015/08/10 18:06:25, erikchen wrote: > formatting (missing whitespace) Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:336: // size to the expected frame at the end On 2015/08/10 18:06:25, erikchen wrote: > formatting of the comment. Also periods. Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:339: [primaryWindow_ setFrameAndStyleLock:NO]; On 2015/08/10 18:06:25, erikchen wrote: > It's very, very important that we remember to call setFrameAndStyleLock:NO. Can > you make a C++ scoped class along the lines of > > https://code.google.com/p/chromium/codesearch#chromium/src/base/scoped_generi... > > that can be manually reset here, but whose destructor guarantees that this is > unset? Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_layout.mm (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_layout.mm:268: // DCHECK_LE(maxY, parameters_.contentViewSize.height + fullscreenYOffset_); On 2015/08/10 18:06:25, erikchen wrote: > huh? Removed this. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/framed_browser_window.h (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.h:52: - (void) setFrameAndStyleLock:(BOOL)lock; On 2015/08/10 18:06:25, erikchen wrote: > extra whitespace after (void) Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/framed_browser_window.mm (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.mm:105: // Overriding this method to prevent the OSX from automatically setting On 2015/08/10 18:06:26, erikchen wrote: > grammar. > > This method is overridden to prevent AppKit from changing the style mask when > frameAndStyleLock_ is YES. > > Declare your overrides in the header, and move the comments there. Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.mm:107: - (void)setStyleMask:(NSUInteger)styleMask { On 2015/08/10 18:06:26, erikchen wrote: > reformat this method to meet Chrome style guidelines. ditto to below. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.mm:109: return; On 2015/08/10 18:06:25, erikchen wrote: > This is, in general, a very dangerous operation. I'd feel a lot better if we > could add some safety checks. > > e.g., if we expect to override one AppKit invocation, then upload UMA metrics if > it happens more than once. > > ditto for below. Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.mm:114: // Overriding this method to prevent the OSX from automatically setting On 2015/08/10 18:06:26, erikchen wrote: > ditto Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.mm:118: animate:(BOOL)animateFlag { On 2015/08/10 18:06:26, erikchen wrote: > reformat this method to meet Chrome style guidelines. I moved the overriden methods to the top. I'm confused about this though, are the parameters not supposed to match up? https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.mm:202: - (void) setFrameAndStyleLock:(BOOL)lock { On 2015/08/10 18:06:26, erikchen wrote: > run "git cl format" to clang-format your entire CL. clang-format has as couple > of bugs, but it will catch all of these small nits. Done. This is magical https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/full_size_content_window.h (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/full_size_content_window.h:25: @interface FullSizeContentView : NSView On 2015/08/10 18:06:26, erikchen wrote: > You moved the @interface from the .mm to the .h, but failed to move the comment > over. This makes it very difficult to tell what this class is used for, and what > your subsequent method forceFrameSize: does. Done.
The CQ bit was checked by spqchan@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1276383004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1276383004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
PTAL
On 2015/08/11 21:52:05, spqchan1 wrote: > PTAL (more comments to follow) Please update your CL description.
spqchan: I started a review pass, but it appears that you have not yet responded to all of my comments from the last review pass. Please respond to those, and these, before asking for another review pass. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/framed_browser_window.mm (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.mm:118: animate:(BOOL)animateFlag { On 2015/08/11 21:43:48, spqchan1 wrote: > On 2015/08/10 18:06:26, erikchen wrote: > > reformat this method to meet Chrome style guidelines. > > I moved the overriden methods to the top. I'm confused about this though, are > the parameters not supposed to match up? The rest of the method is formatted incorrectly. The first 3 lines are fine. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.mm:1953: why did you add whitespace here? https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller_private.mm:225: please don't randomly add whitespace https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller_private.mm:715: fullscreenTransition_.reset(); I see that you changed s/enterFullscreenTransition_/fullscreenTransition_, but why did you move its location? You moved it under a comment, which implies that the comment applies to it. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h (right): https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:10: #import "chrome/browser/ui/cocoa/framed_browser_window.h" You can just a forward declaration instead. @class FramedBrowserWindow; https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:44: // but is used for the animation is exiting full screen grammar check. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:89: - (instancetype)initEnterWithWindow:(FramedBrowserWindow*)window; Please add a comment indicating when each of these two initializers should be used. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:97: // These two methods begins animation for exit and enter fullscreen transitions which two methods? https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:103: // In startCustomExitFullScreenAnimationWithDuration, the content view gets Can you be more precise? Are you referring to the exit or enter animation? How does it get resized? Does it become bigger or smaller? https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:104: // resized in order remove newline. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:105: // to avoid clipping from the window missing period at end. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:120: // During exit fullscreen transition, the content size shrinks while the During /the/ exit fullscreen... https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:121: // window frame stays the same. When that happens, we want to set the Precision of language: We are not setting BrowserWindowLayout to the content size, we are passing it a size. Also, your use of the word "window" in the first line conflicts with your use of the word |window| in the last line. Your later sentence is correct - this method is not returning the size of the window, which your first sentence contradicts. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right): https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:30: // This class locks and unlocks the FrameBrowserWindow. Its deconstructor s/deconstructor/destructor https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:34: base::scoped_nsobject<FramedBrowserWindow> window_; window_ should be private. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:74: // to the current screen coordinates missing a period https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:96: // - Sets the size to the screen's size. s/Sets/Set https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:106: - (NSArray*)customWindowsForFullScreenTransition; This method is present in the header, there's no need to redefine the in .mm file. Move the comments over to the header file.
PTAL https://codereview.chromium.org/1276383004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/1276383004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:787: [framedWindow setFrameAndStyleLock:NO]; Removed the lock here because reset() should get rid of it. Think I should create a DCHECK for this? https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/framed_browser_window.h (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.h:39: // of the window to be changed when it's set to YES On 2015/08/10 18:06:25, erikchen wrote: > Grammar. > > This lock prevents the frame and style of the window from being changed. > > I understand "frame", but I don't understand "style". What do you mean by the > "style" of a window? Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/framed_browser_window.mm (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/framed_browser_window.mm:118: animate:(BOOL)animateFlag { On 2015/08/11 22:42:54, erikchen wrote: > On 2015/08/11 21:43:48, spqchan1 wrote: > > On 2015/08/10 18:06:26, erikchen wrote: > > > reformat this method to meet Chrome style guidelines. > > > > I moved the overriden methods to the top. I'm confused about this though, are > > the parameters not supposed to match up? > > The rest of the method is formatted incorrectly. The first 3 lines are fine. Done. Removed the curly brace https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.mm:1953: On 2015/08/11 22:42:54, erikchen wrote: > why did you add whitespace here? Done. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller_private.mm:225: On 2015/08/11 22:42:54, erikchen wrote: > please don't randomly add whitespace Done. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller_private.mm:715: fullscreenTransition_.reset(); On 2015/08/11 22:42:54, erikchen wrote: > I see that you changed s/enterFullscreenTransition_/fullscreenTransition_, but > why did you move its location? You moved it under a comment, which implies that > the comment applies to it. Originally, fullscreenTransition was going take care of both enter and exit transition. The object would be created at enterTransition and then removed at exitTransition. However, after refactoring the code, it made more sense to create the object at the beginning of enterTransition and then destroy it at the end of enterTransition. The same thing will happen with exitTransition. As such, I added back fullscreenTransition_.reset() Anyways, I'll move it back to the top of the method. Note: in windowDidExitFullScreen, I moved reset() to the end because I want the layout to be updated before getting rid of the fullscreenTransition object (which contains the window size layout parameter logic) https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h (right): https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:10: #import "chrome/browser/ui/cocoa/framed_browser_window.h" On 2015/08/11 22:42:54, erikchen wrote: > You can just a forward declaration instead. @class FramedBrowserWindow; Done. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:44: // but is used for the animation is exiting full screen On 2015/08/11 22:42:54, erikchen wrote: > grammar check. Done. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:89: - (instancetype)initEnterWithWindow:(FramedBrowserWindow*)window; On 2015/08/11 22:42:54, erikchen wrote: > Please add a comment indicating when each of these two initializers should be > used. Done. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:97: // These two methods begins animation for exit and enter fullscreen transitions On 2015/08/11 22:42:54, erikchen wrote: > which two methods? Done. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:103: // In startCustomExitFullScreenAnimationWithDuration, the content view gets On 2015/08/11 22:42:54, erikchen wrote: > Can you be more precise? Are you referring to the exit or enter animation? How > does it get resized? Does it become bigger or smaller? Done. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:104: // resized in order On 2015/08/11 22:42:55, erikchen wrote: > remove newline. Done. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:105: // to avoid clipping from the window On 2015/08/11 22:42:55, erikchen wrote: > missing period at end. Done. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:120: // During exit fullscreen transition, the content size shrinks while the On 2015/08/11 22:42:54, erikchen wrote: > During /the/ exit fullscreen... Done. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:121: // window frame stays the same. When that happens, we want to set the On 2015/08/11 22:42:54, erikchen wrote: > Precision of language: We are not setting BrowserWindowLayout to the content > size, we are passing it a size. Also, your use of the word "window" in the first > line conflicts with your use of the word |window| in the last line. > > Your later sentence is correct - this method is not returning the size of the > window, which your first sentence contradicts. Fixed the comments so that it's more precise https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right): https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:30: // This class locks and unlocks the FrameBrowserWindow. Its deconstructor On 2015/08/11 22:42:55, erikchen wrote: > s/deconstructor/destructor Done. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:34: base::scoped_nsobject<FramedBrowserWindow> window_; On 2015/08/11 22:42:55, erikchen wrote: > window_ should be private. Done. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:74: // to the current screen coordinates On 2015/08/11 22:42:55, erikchen wrote: > missing a period Done. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:96: // - Sets the size to the screen's size. On 2015/08/11 22:42:55, erikchen wrote: > s/Sets/Set Done. https://codereview.chromium.org/1276383004/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:106: - (NSArray*)customWindowsForFullScreenTransition; On 2015/08/11 22:42:55, erikchen wrote: > This method is present in the header, there's no need to redefine the in .mm > file. Move the comments over to the header file. Removed it
PTAL Sorry for the missing the comments in Patch 4, I added changes according to the ones I missed and responded to them https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:896: [[[self window] contentView] bounds].size : [[self window] frame].size; On 2015/08/10 18:06:24, erikchen wrote: > Can you explain this logic? Also, can you move the sizing logic into the > FullScreenTransition class, so that we don't have to expose this implementation > detail to BrowserWindowController? > > e.g. > if (exitingAppKitFullscreen_) > fullscreenTransition getDesiredWindowSize Done, moved logic into "getDesiredWindowLayoutSize" in BrowserWindowFullscreenTransition. Explanation is included its comments https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right): https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:85: // - Adds the style mask NSFullScreenWindowMask to the current window. On 2015/08/10 18:06:25, erikchen wrote: > Doesn't this only apply to the enter fullscreen transition? Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:93: - (void)setPrimaryWindowToFinalFrame; On 2015/08/10 18:06:25, erikchen wrote: > What does this method do? Comment please. > > The verb "set" is typically reserved for setters. Perhaps > movePrimaryWindowToFinalFrame? Changed to changePrimaryWindowToFinalFrame https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:122: finalFrame_ = initialFrame_; On 2015/08/10 18:06:25, erikchen wrote: > It looks like you're trying to keep this class around between the enter/exit > animations. This is intended to be a transitory class, if you need additional > state, add it to the -init method. > > The -init method should also determine whether this is an enter or exit > fullscreen transition. Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:131: CGPointMake(screenOriginX, screenOriginY); On 2015/08/10 18:06:25, erikchen wrote: > This logic can be moved into initial setup, and then > -customWindowsToExitFullScreenTransition and > -customWindowsToEnterFullScreenTransition will have the same implementation. Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:137: // and lock the primary window so that OSX can't make changes to it On 2015/08/10 18:06:25, erikchen wrote: > All method comments should be at the declarations, not definitions. Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:140: (NSTimeInterval)duration { On 2015/08/10 18:06:25, erikchen wrote: > Given the way that you've organized this class, I would have expected a single > method startFullscreenAnimationWithDuration: > > if (exiting fullscreen) > do stuff > <common logic> Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:142: (FullSizeContentView*)[primaryWindow_ contentView]; On 2015/08/10 18:06:25, erikchen wrote: > No C-style casts Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:206: [snapshotWindow_ orderFront:nil]; On 2015/08/10 18:06:25, erikchen wrote: > why did you add this? Right now the snapshotWindow_ has orderFront: called > twice. It should only be called once. Either here, or in the original location. Remove it from the original location https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:207: NSRect finalScreenFrame = [snapshotWindow_ convertRectFromScreen:finalFrame_]; On 2015/08/10 18:06:25, erikchen wrote: > I don't understand this variable. Please add comments that describe what it's > supposed to be/what it does. Done. https://codereview.chromium.org/1276383004/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:296: - (void)preparePrimaryWindowForAnimation { On 2015/08/10 18:06:25, erikchen wrote: > Why did you move the location of this method? I generally try to match the order > of methods in the implementation file with the order of the declarations in the > private category. Done
The CQ bit was checked by spqchan@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1276383004/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1276383004/290001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
overall looks good, just another pass for nits. https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller_private.mm:776: [self layoutSubviews]; Why is this necessary? Shouldn't all the subviews already be laid out correctly? (This may very well be necessary, but it's not clear to me why. Add a comment?) https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h (right): https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:102: frame:(NSRect)frame; What is the caller supposed to pass in for |frame|? Comments please. https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:107: // This method begins animation for exit or enter fullscreen transition begins /the/ animation missing a period. https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (left): https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:160: // Make |primaryWindow_| invisible. This must happen before the window is Why did you remove these comments? https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right): https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:76: // to the current screen coordinates. It looks like initialFrameRelativeToScreen_ is used as the frame of the root layer at t=0 (which is always relative to the NSWindow). I would say: """ // At the beginning of the exit fullscreen animation, the root layer of the window is immediately moved and resized to match its expected frame at the end of the animation. This ivar represents that frame in the coordinate system of the NSWindow, while the NSWindow is still the same size as the screen. """ https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:77: NSRect initialFrameRelativeToScreen_; If this ivar is only used for the exit animation, you should add a comment to that effect. https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:163: // and lock the primary window so that OSX can't make changes to it The lock only happens for the exit animation, right? https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:227: root.frame = initialFrameRelativeToScreen_; The enter animation didn't need to set the root's frame. You've changed that behavior. Why? (It looks like initialFrameRelativeToScreen_ isn't even initialized for the enterAnimation). https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:242: [snapshotWindow_ orderFront:nil]; Why did you move this from makeAndPrepareSnapshotWindow? https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/framed_browser_window.h (right): https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/framed_browser_window.h:52: // This method sets a lock which prevents the the frame and style please reformat your comment to wrap at 80 columns. Grammar check, please. I suggest: """ When the lock is set, the frame and style of the Window cannot be changed. This is used to prevent undesired AppKit resizes of the Window. """ https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/full_size_content_window.h (right): https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/full_size_content_window.h:36: - (void)forceFrameSize:(NSSize)size; (I should have made this comment in my first review pass, apologies) This class is an implementation detail of FullSizeContentWindow. Instead of exposing this, can you add a method to FullSizeContentWindow that can force a change in the size of the content view? e.g. -[FullSizeContentWindow forceContentViewSize:]
PTAL https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller_private.mm:776: [self layoutSubviews]; On 2015/08/12 22:08:52, erikchen wrote: > Why is this necessary? Shouldn't all the subviews already be laid out correctly? > > (This may very well be necessary, but it's not clear to me why. Add a comment?) We want to make sure that the subviews are layout correctly at the end of exit fullscreen, since we've been changing the contentView size. https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h (right): https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:102: frame:(NSRect)frame; On 2015/08/12 22:08:52, erikchen wrote: > What is the caller supposed to pass in for |frame|? Comments please. Done. https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:107: // This method begins animation for exit or enter fullscreen transition On 2015/08/12 22:08:52, erikchen wrote: > begins /the/ animation > missing a period. Done. https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (left): https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:160: // Make |primaryWindow_| invisible. This must happen before the window is On 2015/08/12 22:08:52, erikchen wrote: > Why did you remove these comments? Whoops, these were accidentally removed when I was recombining the two classes. Added them back https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right): https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:76: // to the current screen coordinates. On 2015/08/12 22:08:52, erikchen wrote: > It looks like initialFrameRelativeToScreen_ is used as the frame of the root > layer at t=0 (which is always relative to the NSWindow). I would say: > > """ > // At the beginning of the exit fullscreen animation, the root layer of the > window is immediately moved and resized to match its expected frame at the end > of the animation. This ivar represents that frame in the coordinate system of > the NSWindow, while the NSWindow is still the same size as the screen. > """ Done. I modified the name so that it's less confusing now https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:77: NSRect initialFrameRelativeToScreen_; On 2015/08/12 22:08:52, erikchen wrote: > If this ivar is only used for the exit animation, you should add a comment to > that effect. Done. https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:163: // and lock the primary window so that OSX can't make changes to it On 2015/08/12 22:08:52, erikchen wrote: > The lock only happens for the exit animation, right? Done. Moved this comment to the inside of preparePrimaryWindowForAnimation https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:227: root.frame = initialFrameRelativeToScreen_; On 2015/08/12 22:08:52, erikchen wrote: > The enter animation didn't need to set the root's frame. You've changed that > behavior. Why? > > (It looks like initialFrameRelativeToScreen_ isn't even initialized for the > enterAnimation). This should only be used for the exit animation. I added a check for this. This oversight didn't affect the enter animation because in the code, after setting the root frame, the primaryWindow's frame is set to the finalFrame. This automatically sets the layer's frame to the new one. https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:242: [snapshotWindow_ orderFront:nil]; On 2015/08/12 22:08:52, erikchen wrote: > Why did you move this from makeAndPrepareSnapshotWindow? I moved this out of makeAndPrepareSnapshotWindow because it doesn't work in exit animation. In makeAndPrepareSnapshotWindow, snapshotWindow cannot move in front of primaryWindow because primaryWindow is in fullscreen. As a result, it orderedFront will have to placed at the beginning of the exit transition. I decided to move it to animateSnapshotWindowWithDuration for both enter and exit animation so that it's simpler to read. Alternatively, I can look into manually setting the levels (maybe setting snapshotWindow level to CGShieldingWindowLevel). However, the NSWindow documentation mentioned about being careful with doing this https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/framed_browser_window.h (right): https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/framed_browser_window.h:52: // This method sets a lock which prevents the the frame and style On 2015/08/12 22:08:53, erikchen wrote: > please reformat your comment to wrap at 80 columns. > > Grammar check, please. I suggest: > """ > When the lock is set, the frame and style of the Window cannot be changed. This > is used to prevent undesired AppKit resizes of the Window. > """ Done. https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/full_size_content_window.h (right): https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/full_size_content_window.h:36: - (void)forceFrameSize:(NSSize)size; On 2015/08/12 22:08:53, erikchen wrote: > (I should have made this comment in my first review pass, apologies) > > This class is an implementation detail of FullSizeContentWindow. Instead of > exposing this, can you add a method to FullSizeContentWindow that can force a > change in the size of the content view? > e.g. > -[FullSizeContentWindow forceContentViewSize:] Done. https://codereview.chromium.org/1276383004/diff/290001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/full_size_content_window.h:36: - (void)forceFrameSize:(NSSize)size; On 2015/08/12 22:08:53, erikchen wrote: > (I should have made this comment in my first review pass, apologies) > > This class is an implementation detail of FullSizeContentWindow. Instead of > exposing this, can you add a method to FullSizeContentWindow that can force a > change in the size of the content view? > e.g. > -[FullSizeContentWindow forceContentViewSize:] Done.
The CQ bit was checked by spqchan@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1276383004/450001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1276383004/450001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Your CL description is inaccurate (it talks about UMA), and it isn't column wrapped to 80. Other than that, lgtm.
spqchan@google.com changed reviewers: + andresantoso@chromium.org
Added andresantoso as a reviewer
On 2015/08/13 20:52:31, spqchan1 wrote: > Added andresantoso as a reviewer Your CL description is still inaccurate: There are no UMA changes in your CL.
https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h (right): https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:69: // startCustomFullScreenAnimationWithDuration:duration]; nit: I think this will fit in a single line without wrapping. https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:88: // - windowDidExitFullScreen: nit: no spaces after '-' https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:120: nit: this blank line makes the above comment not part of the below method. Add // to this line if you want a blank line. https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:138: - (NSSize)getDesiredWindowLayoutSize; We don't usually use the 'get' prefix for Objective-C getter methods. https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right): https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:34: FrameAndStyleLock(FramedBrowserWindow* window) { The style guide requires explicit for one argument constructors. https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:43: base::scoped_nsobject<FramedBrowserWindow> window_; DISALLOW_COPY_AND_ASSIGN(); https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:150: exitLayerFrame_.origin = CGPointMake(screenOriginX, screenOriginY); nit (optional): I prefer this without local variables: exitLayerFrame_.origin = CGPointMake( finalFrame_.origin.x - initialFrame_.origin.x, finalFrame_.origin.y - initialFrame_.origin.y); https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:174: return (isEnteringFullscreen_) ? [primaryWindow_ frame].size Why the parenthesis around isEnteringFullscreen_? https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/framed_browser_window.h (right): https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/framed_browser_window.h:64: - (void)setFrame:(NSRect)frameRect display:(BOOL)flag animate:(BOOL)animateFlag; nit: flag is not a good parameter name. I suggest following NSWindow's documentation, which use displayViews and and performAnimation (or just animate).
PTAL https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h (right): https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:69: // startCustomFullScreenAnimationWithDuration:duration]; On 2015/08/13 23:03:18, Andre wrote: > nit: I think this will fit in a single line without wrapping. Done. https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:88: // - windowDidExitFullScreen: On 2015/08/13 23:03:19, Andre wrote: > nit: no spaces after '-' Done. https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:120: On 2015/08/13 23:03:19, Andre wrote: > nit: this blank line makes the above comment not part of the below method. > Add // to this line if you want a blank line. Done. https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.h:138: - (NSSize)getDesiredWindowLayoutSize; On 2015/08/13 23:03:19, Andre wrote: > We don't usually use the 'get' prefix for Objective-C getter methods. Done. https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right): https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:34: FrameAndStyleLock(FramedBrowserWindow* window) { On 2015/08/13 23:03:19, Andre wrote: > The style guide requires explicit for one argument constructors. Done. https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:43: base::scoped_nsobject<FramedBrowserWindow> window_; On 2015/08/13 23:03:19, Andre wrote: > DISALLOW_COPY_AND_ASSIGN(); Done. https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:150: exitLayerFrame_.origin = CGPointMake(screenOriginX, screenOriginY); On 2015/08/13 23:03:19, Andre wrote: > nit (optional): I prefer this without local variables: > exitLayerFrame_.origin = CGPointMake( > finalFrame_.origin.x - initialFrame_.origin.x, > finalFrame_.origin.y - initialFrame_.origin.y); Done. https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:174: return (isEnteringFullscreen_) ? [primaryWindow_ frame].size On 2015/08/13 23:03:19, Andre wrote: > Why the parenthesis around isEnteringFullscreen_? Done. https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/framed_browser_window.h (right): https://codereview.chromium.org/1276383004/diff/450001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/framed_browser_window.h:64: - (void)setFrame:(NSRect)frameRect display:(BOOL)flag animate:(BOOL)animateFlag; On 2015/08/13 23:03:19, Andre wrote: > nit: flag is not a good parameter name. > I suggest following NSWindow's documentation, which use displayViews and and > performAnimation (or just animate). Done.
LGTM (You still need an owner to review)
erikchen@chromium.org changed reviewers: + rsesek@chromium.org
+rsesek for OWNER review.
Just a few style nits. https://codereview.chromium.org/1276383004/diff/490001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right): https://codereview.chromium.org/1276383004/diff/490001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:35: window_.reset([window retain]); You can place this in an initializer list. https://codereview.chromium.org/1276383004/diff/490001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:38: void setLock(bool lock) { [window_ setFrameAndStyleMaskLock:lock]; } naming: set_lock https://codereview.chromium.org/1276383004/diff/490001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:40: ~FrameAndStyleLock() { setLock(NO); } This should come after the ctor.
PTAL https://codereview.chromium.org/1276383004/diff/490001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right): https://codereview.chromium.org/1276383004/diff/490001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:35: window_.reset([window retain]); On 2015/08/14 02:27:05, Robert Sesek (slow this week) wrote: > You can place this in an initializer list. Done. https://codereview.chromium.org/1276383004/diff/490001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:38: void setLock(bool lock) { [window_ setFrameAndStyleMaskLock:lock]; } On 2015/08/14 02:27:05, Robert Sesek (slow this week) wrote: > naming: set_lock Done. https://codereview.chromium.org/1276383004/diff/490001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:40: ~FrameAndStyleLock() { setLock(NO); } On 2015/08/14 02:27:05, Robert Sesek (slow this week) wrote: > This should come after the ctor. Done.
LGTM
The CQ bit was checked by spqchan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org, andresantoso@chromium.org Link to the patchset: https://codereview.chromium.org/1276383004/#ps530001 (title: "Removed extra include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1276383004/530001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1276383004/530001
Message was sent while issue was closed.
Committed patchset #28 (id:530001)
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/6adb1bc1f87fbbbba19b2950d48a48f67eb87d23 Cr-Commit-Position: refs/heads/master@{#343426} |
