[Mac] Add support for growing or shrinking the window during animations....

[Mac] Add support for growing or shrinking the window during animations.

BUG=http://crbug.com/14900
TEST=No visible impact, yet.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=32560

[rohitrao (ping after 24h), 2009-11-12 23:37:32]

+trung because he's been doing bookmark bar animations.

+avi because this used to be his code.  The only real addition is the call to
disableUpdatesUntilFlush, which seems to remove a whole bunch of jank.

[viettrungluu, 2009-11-13 01:43:40]

LGTM with my comments addressed.

http://codereview.chromium.org/391035/diff/4001/8
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/391035/diff/4001/8#newcode79
Line 79: BOOL initializing_;
Could you please add a comment on what this is for?

http://codereview.chromium.org/391035/diff/4001/8#newcode184
Line 184: // height is not adjusted.  If the growing the window by the requested
amount
s/If the growing/If growing/

http://codereview.chromium.org/391035/diff/4001/8#newcode188
Line 188: // this function should be followed by a call to |layoutSubviews|.
Does this move the window upwards if necessary?

http://codereview.chromium.org/391035/diff/4001/9
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/391035/diff/4001/9#newcode468
Line 468: if ([self isFullscreen] || initializing_)
What's the case in which the check for |initializing_| is needed?

You may also want to short-circuit out if |deltaH| is zero.

http://codereview.chromium.org/391035/diff/4001/9#newcode486
Line 486: // needed continue resizing upwards.  Do not resize the window to be
taller
I guess the answer to one of my previous questions is "yes".

http://codereview.chromium.org/391035/diff/4001/10
File chrome/browser/cocoa/browser_window_controller_unittest.mm (right):

http://codereview.chromium.org/391035/diff/4001/10#newcode217
Line 217: EXPECT_GE(NSMinY(finalFrame), NSMinY(initialFrame));
You could also test some other cases.... (Growing a bit up/down but max-ing out
at the height of the workarea, no growing when already max-ed out, no growing
when partly outside the workarea, etc.)

[TVL, 2009-11-13 03:41:24]

drive by

http://codereview.chromium.org/391035/diff/4001/9
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/391035/diff/4001/9#newcode471
Line 471: NSRect windowFrame = [[self window] frame];
you use [self window] it's probably worth fetching it into a local.

http://codereview.chromium.org/391035/diff/4001/9#newcode504
Line 504: [[[self window] contentView] setAutoresizesSubviews:NO];
same with content view, local var?

http://codereview.chromium.org/391035/diff/4001/9#newcode513
Line 513: - (void)resizeView:(NSView*)view newHeight:(float)height {
CGFloat

[Avi (use Gerrit), 2009-11-13 16:18:45]

Nothing more to say than the other reviewers. LG.

[rohitrao (ping after 24h), 2009-11-13 18:26:11]

http://codereview.chromium.org/391035/diff/4001/8
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/391035/diff/4001/8#newcode184
Line 184: // height is not adjusted.  If the growing the window by the requested
amount
On 2009/11/13 01:43:40, viettrungluu wrote:
> s/If the growing/If growing/

Done.

http://codereview.chromium.org/391035/diff/4001/8#newcode188
Line 188: // this function should be followed by a call to |layoutSubviews|.
On 2009/11/13 01:43:40, viettrungluu wrote:
> Does this move the window upwards if necessary?

Yup, comment added.

http://codereview.chromium.org/391035/diff/4001/9
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/391035/diff/4001/9#newcode468
Line 468: if ([self isFullscreen] || initializing_)
On 2009/11/13 01:43:40, viettrungluu wrote:
> What's the case in which the check for |initializing_| is needed?

Don't want to resize when creating the window -- the new window should open up
with the exact same size as the last one. Comment added.

> 
> You may also want to short-circuit out if |deltaH| is zero.

Added a check.

http://codereview.chromium.org/391035/diff/4001/9#newcode471
Line 471: NSRect windowFrame = [[self window] frame];
On 2009/11/13 03:41:24, TVL wrote:
> you use [self window] it's probably worth fetching it into a local.

Done.

http://codereview.chromium.org/391035/diff/4001/9#newcode504
Line 504: [[[self window] contentView] setAutoresizesSubviews:NO];
On 2009/11/13 03:41:24, TVL wrote:
> same with content view, local var?

Done.

http://codereview.chromium.org/391035/diff/4001/9#newcode513
Line 513: - (void)resizeView:(NSView*)view newHeight:(float)height {
On 2009/11/13 03:41:24, TVL wrote:
> CGFloat
Yup, this needs to be changed everywhere.  I've got a cleanup CL somewhere for
this.

http://codereview.chromium.org/391035/diff/4001/10
File chrome/browser/cocoa/browser_window_controller_unittest.mm (right):

http://codereview.chromium.org/391035/diff/4001/10#newcode217
Line 217: EXPECT_GE(NSMinY(finalFrame), NSMinY(initialFrame));
On 2009/11/13 01:43:40, viettrungluu wrote:
> You could also test some other cases.... (Growing a bit up/down but max-ing
out
> at the height of the workarea, no growing when already max-ed out, no growing
> when partly outside the workarea, etc.)

Done.  I'm a little afraid of large windows flashing all over the place during
the tests, but we can deal with that if/when people complain.

[viettrungluu, 2009-11-19 17:48:18]

Is the Mac try result for the current patch? If so, please fix the unit test(s)
and ping me again.

Otherwise, LGTM with:
- style nit fixed
- a couple more tests
- perhaps one fewer test

http://codereview.chromium.org/391035/diff/11001/12003
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/391035/diff/11001/12003#newcode559
Line 559: andState:bookmarks::kShowingState]);
Please line this up along the :.

http://codereview.chromium.org/391035/diff/11001/12004
File chrome/browser/cocoa/browser_window_controller_unittest.mm (right):

http://codereview.chromium.org/391035/diff/11001/12004#newcode209
Line 209: EXPECT_GT(NSHeight(finalFrame), NSHeight(initialFrame));
Assuming the workarea is reasonably high, couldn't you test that the window
height is exactly (or at least close to, since there might be rounding in Cocoa)
40 higher?

http://codereview.chromium.org/391035/diff/11001/12004#newcode217
Line 217: EXPECT_GT(NSHeight(finalFrame), NSHeight(initialFrame));
Ditto.

http://codereview.chromium.org/391035/diff/11001/12004#newcode234
Line 234: }
Should you have tests which grow the window (in either/both direction(s)) until
it reaches the workarea height?

http://codereview.chromium.org/391035/diff/11001/12007
File chrome/browser/cocoa/toolbar_controller_unittest.mm (right):

http://codereview.chromium.org/391035/diff/11001/12007#newcode303
Line 303: }
Maybe it's just me, but this tests seems kind of (or even more) ludicrous now.
Delete?

[rohitrao (ping after 24h), 2009-11-19 19:15:46]

http://codereview.chromium.org/391035/diff/11001/12003
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/391035/diff/11001/12003#newcode559
Line 559: andState:bookmarks::kShowingState]);
On 2009/11/19 17:48:19, viettrungluu wrote:
> Please line this up along the :.

Lining up the colons makes the line too long.  I've lined it up with
isAnimating.

http://codereview.chromium.org/391035/diff/11001/12004
File chrome/browser/cocoa/browser_window_controller_unittest.mm (right):

http://codereview.chromium.org/391035/diff/11001/12004#newcode209
Line 209: EXPECT_GT(NSHeight(finalFrame), NSHeight(initialFrame));
On 2009/11/19 17:48:19, viettrungluu wrote:
> Assuming the workarea is reasonably high, couldn't you test that the window
> height is exactly (or at least close to, since there might be rounding in
Cocoa)
> 40 higher?

I'm running into trouble with this change, especially on the bots, which have
tiny screens.  I'm not yet sure how to make this change bulletproof across all
screen sizes, so I'm going to hold off for now.

http://codereview.chromium.org/391035/diff/11001/12004#newcode234
Line 234: }
On 2009/11/19 17:48:19, viettrungluu wrote:
> Should you have tests which grow the window (in either/both direction(s))
until
> it reaches the workarea height?

Added a couple more tests.

http://codereview.chromium.org/391035/diff/11001/12007
File chrome/browser/cocoa/toolbar_controller_unittest.mm (right):

http://codereview.chromium.org/391035/diff/11001/12007#newcode303
Line 303: }
On 2009/11/19 17:48:19, viettrungluu wrote:
> Maybe it's just me, but this tests seems kind of (or even more) ludicrous now.
> Delete?

Gone.

[viettrungluu, 2009-11-19 20:08:50]

LGTM.

http://codereview.chromium.org/391035/diff/11001/12003
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/391035/diff/11001/12003#newcode559
Line 559: andState:bookmarks::kShowingState]);
On 2009/11/19 19:15:46, rohitrao wrote:
> On 2009/11/19 17:48:19, viettrungluu wrote:
> > Please line this up along the :.
> 
> Lining up the colons makes the line too long.  I've lined it up with
> isAnimating.

Lies! It would fill up 80 columns exactly. Moreover, you have an extra pair of
(round) parens.

http://codereview.chromium.org/391035/diff/11001/12004
File chrome/browser/cocoa/browser_window_controller_unittest.mm (right):

http://codereview.chromium.org/391035/diff/11001/12004#newcode209
Line 209: EXPECT_GT(NSHeight(finalFrame), NSHeight(initialFrame));
On 2009/11/19 19:15:46, rohitrao wrote:
> On 2009/11/19 17:48:19, viettrungluu wrote:
> > Assuming the workarea is reasonably high, couldn't you test that the window
> > height is exactly (or at least close to, since there might be rounding in
> Cocoa)
> > 40 higher?
> 
> I'm running into trouble with this change, especially on the bots, which have
> tiny screens.  I'm not yet sure how to make this change bulletproof across all
> screen sizes, so I'm going to hold off for now.

Are you sure it's not just a floating point rounding/comparison problem? (The
screens aren't *that* tiny.) In which case you can check something like:

EXPECT_GE(NSHeight(finalFrame), NSHeight(initialFrame) + 40 - 0.5);
EXPECT_LE(NSHeight(finalFrame), NSHeight(initialFrame) + 40 + 0.5);

http://codereview.chromium.org/391035/diff/11001/12004#newcode234
Line 234: }
On 2009/11/19 19:15:46, rohitrao wrote:
> On 2009/11/19 17:48:19, viettrungluu wrote:
> > Should you have tests which grow the window (in either/both direction(s))
> until
> > it reaches the workarea height?
> 
> Added a couple more tests.

Thanks.