[Mac] Adds a new view that will allow us to animate the height property of vi...

[Mac] Adds a new view that will allow us to animate the height property of views.  This class will be the base class for any view that needs to be animated.

BUG=None
TEST=No visible impact.  New unittests should pass.

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

[rohitrao (ping after 24h), 2009-10-30 15:41:58]

I ended up going with NSAnimation, as it was the only way I could reliably
cancel a running animation.  I hope to figure out the problem and change it back
to using animator proxies in the future, but for now I need to start getting
code checked in.

This will be the base class for all our animatable views.  I have infobar
animations working now, so that CL will come out once this code is in.

[TVL, 2009-10-30 15:51:05]

drive by

does it have to be a view?  so everything has to be a subclass of this, or we
have to put these in the view tree?  this can't be something external that uses
the existing views?

http://codereview.chromium.org/350002/diff/3001/3002
File chrome/browser/cocoa/animatable_view.h (right):

http://codereview.chromium.org/350002/diff/3001/3002#newcode33
Line 33: - (float)height;
CGFloats so we're safe for 64bit

http://codereview.chromium.org/350002/diff/3001/3003
File chrome/browser/cocoa/animatable_view.mm (right):

http://codereview.chromium.org/350002/diff/3001/3003#newcode45
Line 45: int height = floor(newHeight);
why floor?

[rohitrao (ping after 24h), 2009-10-30 16:06:59]

does it have to be a view?  so everything has to be a subclass of this, or we
have to put these in the view tree?  this can't be something external that uses
the existing views?

The original plan was to use standard cocoa animations, so we could just say
[[view animator] setHeight:blah].  That required height: and setHeight: methods
on each view, so putting that functionality into a common base class made the
most sense.  I've moved away from that approach for now, because I couldn't
figure out how to cancel a running animation, but I'm still holding out hope
that I'll be able to switch back.

http://codereview.chromium.org/350002/diff/3001/3002
File chrome/browser/cocoa/animatable_view.h (right):

http://codereview.chromium.org/350002/diff/3001/3002#newcode33
Line 33: - (float)height;
On 2009/10/30 15:51:06, TVL wrote:
> CGFloats so we're safe for 64bit

Ah, ok.  I'll send out another CL to clean this up in a few other places as
well.

http://codereview.chromium.org/350002/diff/3001/3003
File chrome/browser/cocoa/animatable_view.mm (right):

http://codereview.chromium.org/350002/diff/3001/3003#newcode45
Line 45: int height = floor(newHeight);
On 2009/10/30 15:51:06, TVL wrote:
> why floor? 

Slow animations (10s) looked really awful when I allowed non-integer heights.  I
have a small preference for keeping everything integer, but I'm ok with removing
it.

[TVL, 2009-10-30 16:10:16]

http://codereview.chromium.org/350002/diff/3001/3003
File chrome/browser/cocoa/animatable_view.mm (right):

http://codereview.chromium.org/350002/diff/3001/3003#newcode45
Line 45: int height = floor(newHeight);
On 2009/10/30 16:06:59, rohitrao wrote:
> On 2009/10/30 15:51:06, TVL wrote:
> > why floor? 
> 
> Slow animations (10s) looked really awful when I allowed non-integer heights. 
I
> have a small preference for keeping everything integer, but I'm ok with
removing
> it.

ah, maybe a comment then to answer that?

[Avi (use Gerrit), 2009-10-30 16:18:46]

On 2009/10/30 16:06:59, rohitrao wrote:
> I couldn't
> figure out how to cancel a running animation

In the status bubble, Mark did a set value with animation time being
std::numeric_limits<NSTimeInterval>::min().

[rohitrao (ping after 24h), 2009-10-30 16:22:29]

On 2009/10/30 16:18:46, Avi wrote:
> On 2009/10/30 16:06:59, rohitrao wrote:
> > I couldn't
> > figure out how to cancel a running animation
> 
> In the status bubble, Mark did a set value with animation time being
> std::numeric_limits<NSTimeInterval>::min().

That still goes back up to the main runloop before firing one last time (correct
me if I'm wrong), but in most of my cases, the view is about to be removed, so
doing that is either wasteful or crashy.

When Mike emailed FrameworksIT about this, they said zero-duration animations
not working was a bug and gave him an setAnimations workaround with NSNull, but
I haven't yet been able to make that work.

[John Grabowski, 2009-11-01 21:20:54]

Drive by

Consider a global notification mechanism of some kind (NSNotificationCenter?) 
to stop all animations immediately.  Possibly limit it to animations in a given
window.

[pink (ping after 24hrs), 2009-11-02 16:01:00]

http://codereview.chromium.org/350002/diff/6004/6005
File chrome/browser/cocoa/animatable_view.h (right):

http://codereview.chromium.org/350002/diff/6004/6005#newcode23
Line 23: IBOutlet id delegate_;  // weak, used to send animation ended messages.
there's no way to set this other than IB?

http://codereview.chromium.org/350002/diff/6004/6005#newcode33
Line 33: - (CGFloat)height;
is this any different than [view frame].size.height? If not, why bother with an
api?

http://codereview.chromium.org/350002/diff/6004/6005#newcode43
Line 43: // Cancels any running animations.
Does it end up at the destination height, or does it just stop in its tracks?

http://codereview.chromium.org/350002/diff/6004/6006
File chrome/browser/cocoa/animatable_view.mm (right):

http://codereview.chromium.org/350002/diff/6004/6006#newcode5
Line 5: #include <QuartzCore/QuartzCore.h>
i had lots of problems if i didn't include Cocoa.h before QC.h. I'm surprised
you got away with it.

http://codereview.chromium.org/350002/diff/6004/6006#newcode9
Line 9: @interface NSHeightAnimation : NSAnimation {
class-level comment needed.

http://codereview.chromium.org/350002/diff/6004/6006#newcode10
Line 10: AnimatableView* view_;
@private?

http://codereview.chromium.org/350002/diff/6004/6006#newcode10
Line 10: AnimatableView* view_;
this looks weak, so it should have a comment.

http://codereview.chromium.org/350002/diff/6004/6006#newcode17
Line 17: - (id)initWithView:(AnimatableView*)view
shouldn't you declare this in the public api above?

http://codereview.chromium.org/350002/diff/6004/6006#newcode21
Line 21: animationCurve:NSAnimationEaseIn])) {
line up colons

http://codereview.chromium.org/350002/diff/6004/6006#newcode54
Line 54: if (currentAnimation_.get())
no need to check this.

http://codereview.chromium.org/350002/diff/6004/6006#newcode58
Line 58: finalHeight:newHeight
align colons

http://codereview.chromium.org/350002/diff/6004/6007
File chrome/browser/cocoa/animatable_view_unittest.mm (right):

http://codereview.chromium.org/350002/diff/6004/6007#newcode16
Line 16: class AnimatableViewTest : public PlatformTest {
Should this be a CocoaTest? I thought we were trying to obsolete CocoaTestHelper

http://codereview.chromium.org/350002/diff/6004/6007#newcode53
Line 53: [view_ setHeight:50];
Note that this might not test the animations since CocoaTestHelper sets
animation durations to take 0 time. You should try experimenting with changing
this locally to see if it affects anything.

[rohitrao (ping after 24h), 2009-11-02 16:34:05]

http://codereview.chromium.org/350002/diff/6004/6005
File chrome/browser/cocoa/animatable_view.h (right):

http://codereview.chromium.org/350002/diff/6004/6005#newcode23
Line 23: IBOutlet id delegate_;  // weak, used to send animation ended messages.
On 2009/11/02 16:01:01, pink wrote:
> there's no way to set this other than IB?

Added a property/synthesize.

http://codereview.chromium.org/350002/diff/6004/6005#newcode33
Line 33: - (CGFloat)height;
On 2009/11/02 16:01:01, pink wrote:
> is this any different than [view frame].size.height? If not, why bother with
an
> api?

It was needed back when I was still using cocoa animator proxies, since I had to
have both height and setHeight methods.  I don't think I'm using it anywhere
right now, but the plan is to eventually go back to animator proxies.

http://codereview.chromium.org/350002/diff/6004/6005#newcode43
Line 43: // Cancels any running animations.
On 2009/11/02 16:01:01, pink wrote:
> Does it end up at the destination height, or does it just stop in its tracks?

Stops in its tracks, since I don't yet have a use for "go immediately to
destination height."  Added a comment.

http://codereview.chromium.org/350002/diff/6004/6006
File chrome/browser/cocoa/animatable_view.mm (right):

http://codereview.chromium.org/350002/diff/6004/6006#newcode5
Line 5: #include <QuartzCore/QuartzCore.h>
On 2009/11/02 16:01:01, pink wrote:
> i had lots of problems if i didn't include Cocoa.h before QC.h. I'm surprised
> you got away with it.

I was probably getting Cocoa.h indirectly from animatable_view.h.  Added it
explicitly, and fixed all of these includes to be imports.

http://codereview.chromium.org/350002/diff/6004/6006#newcode9
Line 9: @interface NSHeightAnimation : NSAnimation {
On 2009/11/02 16:01:01, pink wrote:
> class-level comment needed.

Done, although the comment I added doesn't say much.

http://codereview.chromium.org/350002/diff/6004/6006#newcode10
Line 10: AnimatableView* view_;
On 2009/11/02 16:01:01, pink wrote:
> @private?

Done.

http://codereview.chromium.org/350002/diff/6004/6006#newcode10
Line 10: AnimatableView* view_;
On 2009/11/02 16:01:01, pink wrote:
> this looks weak, so it should have a comment.

Done.

http://codereview.chromium.org/350002/diff/6004/6006#newcode17
Line 17: - (id)initWithView:(AnimatableView*)view
On 2009/11/02 16:01:01, pink wrote:
> shouldn't you declare this in the public api above?

Done.

http://codereview.chromium.org/350002/diff/6004/6006#newcode21
Line 21: animationCurve:NSAnimationEaseIn])) {
On 2009/11/02 16:01:01, pink wrote:
> line up colons

Fixed everywhere.

http://codereview.chromium.org/350002/diff/6004/6006#newcode54
Line 54: if (currentAnimation_.get())
On 2009/11/02 16:01:01, pink wrote:
> no need to check this.

Removed.

http://codereview.chromium.org/350002/diff/6004/6007
File chrome/browser/cocoa/animatable_view_unittest.mm (right):

http://codereview.chromium.org/350002/diff/6004/6007#newcode16
Line 16: class AnimatableViewTest : public PlatformTest {
On 2009/11/02 16:01:01, pink wrote:
> Should this be a CocoaTest? I thought we were trying to obsolete
CocoaTestHelper

Changed to use CocoaTest.

http://codereview.chromium.org/350002/diff/6004/6007#newcode53
Line 53: [view_ setHeight:50];
On 2009/11/02 16:01:01, pink wrote:
> Note that this might not test the animations since CocoaTestHelper sets
> animation durations to take 0 time. You should try experimenting with changing
> this locally to see if it affects anything.

I'm not actually testing anything related to animations, since I wasn't sure
what the best way of testing animations was.  There's a TODO to followup.

[pink (ping after 24hrs), 2009-11-03 19:05:48]

http://codereview.chromium.org/350002/diff/2007/11004
File chrome/browser/cocoa/animatable_view.mm (right):

http://codereview.chromium.org/350002/diff/2007/11004#newcode68
Line 68: currentAnimation_.reset([[NSHeightAnimation alloc] initWithView:self
i don't see anything here that would cancel the animation when the view goes
away. It could still be sending messages back if the animation system has a
strong ref to the animation object, and it would be sending back msgs to a dead
object.

[rohitrao (ping after 24h), 2009-11-03 19:29:08]

http://codereview.chromium.org/350002/diff/2007/11004
File chrome/browser/cocoa/animatable_view.mm (right):

http://codereview.chromium.org/350002/diff/2007/11004#newcode68
Line 68: currentAnimation_.reset([[NSHeightAnimation alloc] initWithView:self
On 2009/11/03 19:05:48, pink wrote:
> i don't see anything here that would cancel the animation when the view goes
> away. It could still be sending messages back if the animation system has a
> strong ref to the animation object, and it would be sending back msgs to a
dead
> object.

I think I was confusing all of the various animation types and for some reason
thought NSAnimation retained its view (this makes no sense at all).  I've added
a call to stopAnimation in dealloc, with a DCHECK because I would like callers
to explicitly stop animations before removing a view.

[pink (ping after 24hrs), 2009-11-03 19:38:53]

lgtm

http://codereview.chromium.org/350002/diff/5012/5014
File chrome/browser/cocoa/animatable_view.mm (right):

http://codereview.chromium.org/350002/diff/5012/5014#newcode53
Line 53: // We should've stopped all animations before dealloc'ing the view.
I'm not sure this is a valid dcheck. What happens if you close a window while
one of these views is animating? Could the window teardown happen before the
animation completes?

[rohitrao (ping after 24h), 2009-11-03 19:53:40]

http://codereview.chromium.org/350002/diff/5012/5014
File chrome/browser/cocoa/animatable_view.mm (right):

http://codereview.chromium.org/350002/diff/5012/5014#newcode53
Line 53: // We should've stopped all animations before dealloc'ing the view.
On 2009/11/03 19:38:53, pink wrote:
> I'm not sure this is a valid dcheck. What happens if you close a window while
> one of these views is animating? Could the window teardown happen before the
> animation completes?

In practice, the infobar and download shelf are coded to kill any animations
before removing themselves, but there's no real need for this DCHECK.  Removing
it.