Mac: Animate the bookmark bar showing/hiding.

Mac: Animate the bookmark bar showing/hiding.

There's a huge refactoring of the bookmark bar controller in this CL, so that animation-related code is in only a few places (instead of scattered all over the place).

Changes to BookmarkBar.xib: Since BookmarkBarToolbarView now inherits from AnimatableView, I had to hook up its delegate_ member to File's Owner (i.e., the BookmarkBarController).

Not yet implemented: morphing between the detached bar (on the NTP) and anything else.

BUG=25600
TEST=Go to a normal page, show/hide the bookmark bar (Shift-Cmd-B), watch it animate.

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

[viettrungluu, 2009-11-13 01:06:00]

This animates only the normal bookmark bar, and not yet the NTP detached bar. I
want to get it out there since it involves a big refactoring....

[rohitrao (ping after 24h), 2009-11-13 02:45:19]

Quick comments while I keep looking at this more.  My one high level comment is
that I don't have a great understanding of how the animations are supposed to
work or look.  I don't know if a big comment at the top of a file will help, or
if we should just write a big design doc when we get the chance.

I'll take another look and ask for more detailed comments where I think they're
needed.

http://codereview.chromium.org/384105/diff/1/3
File chrome/browser/browser.h (right):

http://codereview.chromium.org/384105/diff/1/3#newcode718
Line 718: Profile* const profile_;
Why are these changes needed?

http://codereview.chromium.org/384105/diff/1/4
File chrome/browser/cocoa/animatable_view.h (right):

http://codereview.chromium.org/384105/diff/1/4#newcode23
Line 23: @interface AnimatableView : BackgroundGradientView<NSAnimationDelegate>
{
Are all of the other AnimatableViews completely opaque?  The ones I know of are
the infobar container, the infobar itself, and the download shelf.  I think
it'll be ok, but it's a little bit silly to be drawing a background for all
these views that don't need it.

http://codereview.chromium.org/384105/diff/1/6
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/384105/diff/1/6#newcode75
Line 75: // state. This is set to |kBarInvalid| on initialization (when the
appropriate
kInvalidState

http://codereview.chromium.org/384105/diff/1/6#newcode80
Line 80: // otherwise it should be |kBarInvalid|.
kInvalidState

http://codereview.chromium.org/384105/diff/1/6#newcode166
Line 166: - (BOOL)isShownAsDetachedBar;
Is this exclusive with respect to isShownAsToolbar?

http://codereview.chromium.org/384105/diff/1/7
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/384105/diff/1/7#newcode62
Line 62: // Moves to the given next state (from the current state), possibly
animating.
Does this stop any running animations?

http://codereview.chromium.org/384105/diff/1/7#newcode70
Line 70: // Puts stuff into the final visual state without animating.
Does this stop running animations?

http://codereview.chromium.org/384105/diff/1/7#newcode276
Line 276: //      CGFloat tempHeight = NSHeight([[self view] frame]) -
kBookmarkBarOverlap;
Delete?

http://codereview.chromium.org/384105/diff/1/20
File chrome/browser/cocoa/view_resizer_pong.mm (right):

http://codereview.chromium.org/384105/diff/1/20#newcode18
Line 18: NSWidth([[view superview] frame]) - 50, height)];
Do all the other unittests still pass with this change?

[TVL, 2009-11-13 03:56:53]

drive by

http://codereview.chromium.org/384105/diff/3001/4006
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/384105/diff/3001/4006#newcode44
Line 44: static const NSTimeInterval kBookmarkBarAnimationDuration = 0.2;
anon namespace instead of static?

http://codereview.chromium.org/384105/diff/3001/4006#newcode276
Line 276: //      CGFloat tempHeight = NSHeight([[self view] frame]) -
kBookmarkBarOverlap;
delete?

[John Grabowski, 2009-11-13 05:34:08]

I second Rohit's comments that a big high level comment is in order.  There's a
lot of stuff here; it's tough to see how it all fits together (and with what
state changes, and when).

http://codereview.chromium.org/384105/diff/3001/4001
File chrome/app/nibs/BookmarkBar.xib (right):

http://codereview.chromium.org/384105/diff/3001/4001#newcode1
Line 1: <?xml version="1.0" encoding="UTF-8"?>
Please add brief english descr of nib changes

http://codereview.chromium.org/384105/diff/3001/4006
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/384105/diff/3001/4006#newcode71
Line 71: - (void)finalizeVisualState;
What happens if you resize the browser window while the bookmark is animating? 
I don't see a finalizeVisualState call made from within -frameDidChange.
What happens if an info bar animates open or closed while the bookmark bar is
animating open or closed?

http://codereview.chromium.org/384105/diff/3001/4007
File chrome/browser/cocoa/bookmark_bar_controller_unittest.mm (right):

http://codereview.chromium.org/384105/diff/3001/4007#newcode398
Line 398: // Short titles increase the chances of this test succeeding if the
view is
Button creation is not currently a function of window size.
View size should not be variable when running this test.

http://codereview.chromium.org/384105/diff/3001/4013
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/384105/diff/3001/4013#newcode1327
Line 1327: // Position the sheet as follows:
What if a sheet opens while a bookmark is animating?  Will it look wrong?

http://codereview.chromium.org/384105/diff/3001/4014
File chrome/browser/cocoa/browser_window_controller_unittest.mm (right):

http://codereview.chromium.org/384105/diff/3001/4014#newcode116
Line 116: #if 0
Can you fix this then?  #if 0 of a unit test "because it's broken with my CL"
kinda sucks.

[rohitrao (ping after 24h), 2009-11-13 17:05:51]

A few more comments.  I'm starting to understand how all of this is supposed to
work.

If you haven't already, you should set all of the animation durations to 10
seconds or so and test as many weird combinations as you can:
- Closing the bookmark bar while it's opening.
- Opening the bookmark bar while an infobar is animating.
- Closing tabs, switching tabs, closing the window while an animation is
running.
- Deleting or renaming a bookmark while the bar is opening or closing.
- etc :)

http://codereview.chromium.org/384105/diff/3001/4005
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/384105/diff/3001/4005#newcode58
Line 58: bookmarkBarDidChangeFromState:(bookmarks::VisualState)oldState
Thoughts on making this bookmarkBar:DidChangeFromState:toState: instead of
sender:bookmarkBarDidChangeFromState:toState:?

http://codereview.chromium.org/384105/diff/3001/4006
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/384105/diff/3001/4006#newcode1028
Line 1028: // Determines the appropriate (basic) state for the given situation.
What does "basic" mean?

http://codereview.chromium.org/384105/diff/3001/4006#newcode1112
Line 1112: [delegate_ sender:self bookmarkBarDidChangeFromState:oldVisualState
Does this need to fire before or after -[BWC setAnimationInProgress], or does it
not matter?

http://codereview.chromium.org/384105/diff/3001/4006#newcode1122
Line 1122: DCHECK([view isKindOfClass:[AnimatableView class]]);
Replace with [self animatableView].

http://codereview.chromium.org/384105/diff/3001/4006#newcode1128
Line 1128: - (void)animationDidEnd:(NSAnimation*)animation {
Make sure to call the superclass' implementation.

And remember that a stopped animation will call animationDidStop: instead, so
you'll need to implement that as well if you need to do cleanup for stopped
animations.  Ah, nevermind, looks like you manually do cleanup after each call
to stopCurrentAnimation.

http://codereview.chromium.org/384105/diff/3001/4013
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/384105/diff/3001/4013#newcode1436
Line 1436: lastBookmarkBarState == bookmarks::kShowingState)) {
There's a lot of complicated logic around the current state and the last state,
and presumably there's on the order of 10 valid combinations.  I think this test
here corresponds to "open or animating open."  Is there a good way to abstract
these states away, so callers outside of BookmarkBarController don't need to
know or care about states?  Barring that, a comment above each test involving
states would be helpful.

[viettrungluu, 2009-11-13 20:34:46]

Thanks everyone for the reviews.

I *think* I've done (or at least responded to) everything. I've slowed down
animations and made sure it works reasonably well (in particular, no crashes or
strange behaviour) if you manage to do stuff during the animations.

Oh wait, I guess I should go write a big comment explaining things (the #1
request)....

http://codereview.chromium.org/384105/diff/1/3
File chrome/browser/browser.h (right):

http://codereview.chromium.org/384105/diff/1/3#newcode718
Line 718: Profile* const profile_;
On 2009/11/13 02:45:19, rohitrao wrote:
> Why are these changes needed?

I guess they aren't, but they're the right thing to have. (It means that instead
of passing a Browser* around all the time (e.g., when initializing some object,
with the object hanging on to that reference), it's safe to just pass
Browser->profile(), for example. I guess I can remove this change (or separate
it into another CL) if you want.

http://codereview.chromium.org/384105/diff/1/4
File chrome/browser/cocoa/animatable_view.h (right):

http://codereview.chromium.org/384105/diff/1/4#newcode23
Line 23: @interface AnimatableView : BackgroundGradientView<NSAnimationDelegate>
{
On 2009/11/13 02:45:19, rohitrao wrote:
> Are all of the other AnimatableViews completely opaque?  The ones I know of
are
> the infobar container, the infobar itself, and the download shelf.  I think
> it'll be ok, but it's a little bit silly to be drawing a background for all
> these views that don't need it.

After some discussion with jrg, I think having AnimatableView subclass
BackgroundGradientView is the right thing to do by default. Possibly we should
provide a subclass of AnimatableView which doesn't draw a background.

http://codereview.chromium.org/384105/diff/1/6
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/384105/diff/1/6#newcode75
Line 75: // state. This is set to |kBarInvalid| on initialization (when the
appropriate
On 2009/11/13 02:45:19, rohitrao wrote:
> kInvalidState

Done.

http://codereview.chromium.org/384105/diff/1/6#newcode80
Line 80: // otherwise it should be |kBarInvalid|.
On 2009/11/13 02:45:19, rohitrao wrote:
> kInvalidState

Done.

http://codereview.chromium.org/384105/diff/1/6#newcode166
Line 166: - (BOOL)isShownAsDetachedBar;
On 2009/11/13 02:45:19, rohitrao wrote:
> Is this exclusive with respect to isShownAsToolbar?

Yes. Added comments.

http://codereview.chromium.org/384105/diff/1/7
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/384105/diff/1/7#newcode62
Line 62: // Moves to the given next state (from the current state), possibly
animating.
On 2009/11/13 02:45:19, rohitrao wrote:
> Does this stop any running animations?

Yes, in a technical sense. It may, however, just stop an animation mid-flight
and animate from there to the new state. Comment added.

http://codereview.chromium.org/384105/diff/1/7#newcode70
Line 70: // Puts stuff into the final visual state without animating.
On 2009/11/13 02:45:19, rohitrao wrote:
> Does this stop running animations?

Yes. Comment added.

http://codereview.chromium.org/384105/diff/1/7#newcode276
Line 276: //      CGFloat tempHeight = NSHeight([[self view] frame]) -
kBookmarkBarOverlap;
On 2009/11/13 02:45:19, rohitrao wrote:
> Delete?

Done.

http://codereview.chromium.org/384105/diff/1/20
File chrome/browser/cocoa/view_resizer_pong.mm (right):

http://codereview.chromium.org/384105/diff/1/20#newcode18
Line 18: NSWidth([[view superview] frame]) - 50, height)];
On 2009/11/13 02:45:19, rohitrao wrote:
> Do all the other unittests still pass with this change?

Yes, at least as of yesterday. ;-)

http://codereview.chromium.org/384105/diff/3001/4001
File chrome/app/nibs/BookmarkBar.xib (right):

http://codereview.chromium.org/384105/diff/3001/4001#newcode1
Line 1: <?xml version="1.0" encoding="UTF-8"?>
On 2009/11/13 05:34:08, John Grabowski wrote:
> Please add brief english descr of nib changes

Done. Thanks for noticing that I changed the nib.

http://codereview.chromium.org/384105/diff/3001/4005
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/384105/diff/3001/4005#newcode58
Line 58: bookmarkBarDidChangeFromState:(bookmarks::VisualState)oldState
On 2009/11/13 17:05:51, rohitrao wrote:
> Thoughts on making this bookmarkBar:DidChangeFromState:toState: instead of
> sender:bookmarkBarDidChangeFromState:toState:?

I'm all for it. Done.

http://codereview.chromium.org/384105/diff/3001/4005#newcode63
Line 63: bookmarkBarWillAnimateFromState:(bookmarks::VisualState)oldState
And here too, of course.

http://codereview.chromium.org/384105/diff/3001/4006
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/384105/diff/3001/4006#newcode44
Line 44: static const NSTimeInterval kBookmarkBarAnimationDuration = 0.2;
On 2009/11/13 03:56:53, TVL wrote:
> anon namespace instead of static?

Done.

http://codereview.chromium.org/384105/diff/3001/4006#newcode71
Line 71: - (void)finalizeVisualState;
On 2009/11/13 05:34:08, John Grabowski wrote:
> What happens if you resize the browser window while the bookmark is animating?

> I don't see a finalizeVisualState call made from within -frameDidChange.

It continues to animating. Per Rohit's suggestion, I turned the animation
durations to 10 seconds (and I thought 1 second was glacial) -- it's amusing to
watch.

> What happens if an info bar animates open or closed while the bookmark bar is
> animating open or closed?

They animate together -- also quite amusing.

http://codereview.chromium.org/384105/diff/3001/4006#newcode276
Line 276: //      CGFloat tempHeight = NSHeight([[self view] frame]) -
kBookmarkBarOverlap;
On 2009/11/13 03:56:53, TVL wrote:
> delete?

Done.

http://codereview.chromium.org/384105/diff/3001/4006#newcode1028
Line 1028: // Determines the appropriate (basic) state for the given situation.
On 2009/11/13 17:05:51, rohitrao wrote:
> What does "basic" mean?

It means that this is a stale comment. ;-)

http://codereview.chromium.org/384105/diff/3001/4006#newcode1112
Line 1112: [delegate_ sender:self bookmarkBarDidChangeFromState:oldVisualState
On 2009/11/13 17:05:51, rohitrao wrote:
> Does this need to fire before or after -[BWC setAnimationInProgress], or does
it
> not matter?

I don't think it matters.

http://codereview.chromium.org/384105/diff/3001/4006#newcode1122
Line 1122: DCHECK([view isKindOfClass:[AnimatableView class]]);
On 2009/11/13 17:05:51, rohitrao wrote:
> Replace with [self animatableView].

Done.

http://codereview.chromium.org/384105/diff/3001/4006#newcode1128
Line 1128: - (void)animationDidEnd:(NSAnimation*)animation {
On 2009/11/13 17:05:51, rohitrao wrote:
> Make sure to call the superclass' implementation.

Do I have to, given that this is a delegate method and that the superclass is
NSViewController?

http://codereview.chromium.org/384105/diff/3001/4007
File chrome/browser/cocoa/bookmark_bar_controller_unittest.mm (right):

http://codereview.chromium.org/384105/diff/3001/4007#newcode398
Line 398: // Short titles increase the chances of this test succeeding if the
view is
On 2009/11/13 05:34:08, John Grabowski wrote:
> Button creation is not currently a function of window size.

No, but containment in the view's frame may depend on window size.

> View size should not be variable when running this test.

The fact that this worked previously relied on the view not making itself the
right size (by calling the resizer) on initialization. Moreover, this test
relied on the initial view size (without any resizing) happening to be big
enough to contain the buttons.

Let me put a big TODO on this test, though it's not obvious how to completely
properly test without mocking (font, etc.) metrics too.

http://codereview.chromium.org/384105/diff/3001/4013
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/384105/diff/3001/4013#newcode1327
Line 1327: // Position the sheet as follows:
On 2009/11/13 05:34:08, John Grabowski wrote:
> What if a sheet opens while a bookmark is animating?  Will it look wrong?

Yeah, for an instant (while it's animating). I suspect no one will notice. It
places the sheet to where it should be at the end of the animation, which is
correct since that's the state that'll actually be seen for more than a fraction
of a second.

The alternative is to cancel the animation (this might look bad in a different
way, especially since the sheet itself will animate). It's not (reasonably)
feasible to move the sheet along with the animation, plus it'd be kind of
ridiculous.

http://codereview.chromium.org/384105/diff/3001/4013#newcode1436
Line 1436: lastBookmarkBarState == bookmarks::kShowingState)) {
On 2009/11/13 17:05:51, rohitrao wrote:
> There's a lot of complicated logic around the current state and the last
state,
> and presumably there's on the order of 10 valid combinations.  I think this
test
> here corresponds to "open or animating open."  Is there a good way to abstract
> these states away, so callers outside of BookmarkBarController don't need to
> know or care about states?  Barring that, a comment above each test involving
> states would be helpful.

This is "showing normal bar or animating show/hide normal bar". I'll flesh out
the logic in a better way when I add more animations. I'll add a comment and a
TODO in the meantime.

http://codereview.chromium.org/384105/diff/3001/4014
File chrome/browser/cocoa/browser_window_controller_unittest.mm (right):

http://codereview.chromium.org/384105/diff/3001/4014#newcode116
Line 116: #if 0
On 2009/11/13 05:34:08, John Grabowski wrote:
> Can you fix this then?  #if 0 of a unit test "because it's broken with my CL"
> kinda sucks.

D'oh. Fixed. It *was* my fault. (New theory: It's *always* my fault.)

[viettrungluu, 2009-11-13 21:06:45]

Okay, new, big comment added.

Now to investigate a problem with BrowserWindowCocoaTest....

[John Grabowski, 2009-11-13 21:48:53]

LGTM from me but please wait for others to confirm.

[rohitrao (ping after 24h), 2009-11-14 00:05:43]

LGTM

Please make another pass with the testing of edge cases :)  I'm mostly worried
about cases like switching tabs, closing tabs, closing windows -- anything where
we may forget to kill a running animation.

http://codereview.chromium.org/384105/diff/1/3
File chrome/browser/browser.h (right):

http://codereview.chromium.org/384105/diff/1/3#newcode718
Line 718: Profile* const profile_;
On 2009/11/13 20:34:47, viettrungluu wrote:
> On 2009/11/13 02:45:19, rohitrao wrote:
> > Why are these changes needed?
> 
> I guess they aren't, but they're the right thing to have. (It means that
instead
> of passing a Browser* around all the time (e.g., when initializing some
object,
> with the object hanging on to that reference), it's safe to just pass
> Browser->profile(), for example. I guess I can remove this change (or separate
> it into another CL) if you want.

They're fine to keep in here.  Was just wondering if code would fail to compile
without the consts.

http://codereview.chromium.org/384105/diff/3028/8025
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/384105/diff/3028/8025#newcode56
Line 56: // display update.
Should we detail exactly what the delegate is supposed to do in these methods
(compress the toolbar, remove the divider, etc).

Maybe it's breaking the abstraction a little bit, but I think the concepts of
"BWC" and "toolbar" are already referenced in so many places that it wouldn't be
too bad.  Having a concise description somewhere of what these methods should do
would be helpful.

http://codereview.chromium.org/384105/diff/3028/8026
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/384105/diff/3028/8026#newcode415
Line 415: if ((lastVisualState_ == bookmarks::kHiddenState &&
Can you add a line explaining what these two cases are?  (Animating open and
animating closed, I think.)

http://codereview.chromium.org/384105/diff/3028/8026#newcode430
Line 430: if ((lastVisualState_ == bookmarks::kHiddenState &&
Animating open and animating closed?

http://codereview.chromium.org/384105/diff/3028/8026#newcode1195
Line 1195: // Delegate method for |AnimatableView| (a superclass of
Sorry, before I thought we were in a view class.  If this is the controller,
there's obviously no super implementation to call :)

http://codereview.chromium.org/384105/diff/3028/8033
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/384105/diff/3028/8033#newcode1332
Line 1332: //    the sheet below the bookmark bar.
//  - If the bar is currently animating, position the sheet according to where
the bar will be when the animation ends.

[viettrungluu, 2009-11-14 00:59:26]

I took another pass at getting it to crash and I failed (to crash it). We're
actually pretty zealous about cancelling animations though. (Overzealous,
leading to some incorrect cancellations, though this is really only noticeable
because I've turned up the animation durations; possibly this occurs on Windows
too.... I'll file a bug against myself.)

http://codereview.chromium.org/384105/diff/3028/8025
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/384105/diff/3028/8025#newcode56
Line 56: // display update.
On 2009/11/14 00:05:44, rohitrao wrote:
> Should we detail exactly what the delegate is supposed to do in these methods
> (compress the toolbar, remove the divider, etc).
> 
> Maybe it's breaking the abstraction a little bit, but I think the concepts of
> "BWC" and "toolbar" are already referenced in so many places that it wouldn't
be
> too bad.  Having a concise description somewhere of what these methods should
do
> would be helpful.

Done.

http://codereview.chromium.org/384105/diff/3028/8026
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/384105/diff/3028/8026#newcode415
Line 415: if ((lastVisualState_ == bookmarks::kHiddenState &&
On 2009/11/14 00:05:44, rohitrao wrote:
> Can you add a line explaining what these two cases are?  (Animating open and
> animating closed, I think.)

Done.

http://codereview.chromium.org/384105/diff/3028/8026#newcode430
Line 430: if ((lastVisualState_ == bookmarks::kHiddenState &&
On 2009/11/14 00:05:44, rohitrao wrote:
> Animating open and animating closed?

Yes. Comment added (also explaining the "< 1" part below).

http://codereview.chromium.org/384105/diff/3028/8033
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/384105/diff/3028/8033#newcode1332
Line 1332: //    the sheet below the bookmark bar.
On 2009/11/14 00:05:44, rohitrao wrote:
> //  - If the bar is currently animating, position the sheet according to where
> the bar will be when the animation ends.

Done.