Initial support for viewport animation

Add support for viewport animation

1. Extracts viewport transform state out to TimelineDisplayTransform.
2. Viewport manages currentDrawingTransform
3. Introduces ui.Animation and ui.AnimationController as a simple animation wrapper
4. Viewport exposes a setImmediate and queueAnimation to manipulate the currentDrawingTransform

Pan is implemented as an animation. Zoom executes immediately. A followup patch will make zoom smooth.

BUG=364,311
R=dsinclair@chromium.org

Committed: https://code.google.com/p/trace-viewer/source/detail?r=851

[nduca, 2013-08-05 10:15:48]

ptal

[dsinclair, 2013-08-06 14:25:44]

https://codereview.appspot.com/12451045/diff/1/src/tcmalloc/heap_instance_tra...
File src/tcmalloc/heap_instance_track.js (right):

https://codereview.appspot.com/12451045/diff/1/src/tcmalloc/heap_instance_tra...
src/tcmalloc/heap_instance_track.js:93: var dt =
this.viewport.currentDisplayTransform;
nit: s/dt/displayTransform/  (and in various other files)

https://codereview.appspot.com/12451045/diff/1/src/tracing/draw_helpers.js
File src/tracing/draw_helpers.js (right):

https://codereview.appspot.com/12451045/diff/1/src/tracing/draw_helpers.js#ne...
src/tracing/draw_helpers.js:89: var dt = vp.currentDisplayTransform;
I don't think this method uses the viewport anymore, would it be worthwhile to
change the vp param to dt and just pass in the displayTransform?

https://codereview.appspot.com/12451045/diff/1/src/tracing/draw_helpers.js#ne...
src/tracing/draw_helpers.js:147: var dt = vp.currentDisplayTransform;
As above, should we pass in dt instead of vp?

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_display_t...
File src/tracing/timeline_display_transform.js (right):

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_display_t...
src/tracing/timeline_display_transform.js:10: function
advanceToward(currentValue, goal,
This is an internal only function, do we have a naming system for those? Just
the name, name_?

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_display_t...
src/tracing/timeline_display_transform.js:15: if (msElapsedSinceLastAdvance >
90)
90 is the total animation time? Can we make it a constant?

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_display_t...
src/tracing/timeline_display_transform.js:18: var ratioGivenElapsedTime =
ratioPer16MsFrame;
nit: User ratioPer16MsFrame in the return statement, then don't need the store.

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_display_t...
src/tracing/timeline_display_transform.js:41: var that = new
TimelineDisplayTransform();
return new TimelineDisplayTransform(this);

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_display_t...
src/tracing/timeline_display_transform.js:49: var eq = true;
if (that === undefined || that === null)
  return false;

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_display_t...
src/tracing/timeline_display_transform.js:50: eq &= this.panX == that.panX;
nit: ===

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_display_t...
src/tracing/timeline_display_transform.js:57: var eq = true;
if (that === undefined || that === null)
  return false;

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_display_t...
src/tracing/timeline_display_transform.js:90: var xDeltaWorld =
this.xViewVectorToWorld(xDeltaView);
nit: don't need to store, just assign below.

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_display_t...
src/tracing/timeline_display_transform.js:96: if (viewX == 'left') {
nit: ===

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_display_t...
src/tracing/timeline_display_transform.js:103: throw new Error('unrecognized
string for viewPos. left|center|right');
nit: left|center|right accepted

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_track_vie...
File src/tracing/timeline_track_view.js (right):

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_track_vie...
src/tracing/timeline_track_view.js:360:
vp.setDisplayTransformSmoothly(tempDisplayTransform);
These 4 cases all seem very similar:

case 68:
  this.panXTo_(-viewWidth * 0.5);
...

pan_: function(newWidth) {
  tempDisplayTranform.set(vp.goalDisplayTransform);
  tempDisplayTransform.incrementPanXInViewUnits(newWidth
  vp.setDisplayTransformSmoothly(tempDisplayTransform);
}

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_track_vie...
src/tracing/timeline_track_view.js:435: smooth == !!smooth;
= instead of ==?

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_viewport.js
File src/tracing/timeline_viewport.js (right):

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_viewport....
src/tracing/timeline_viewport.js:18: var tempDisplayTransform = new
TimelineDisplayTransform();
nit: add a comment?

Cached temporary display transform. Note, before using this transform you _must_
.set() new values or you will end up with stale data.

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_viewport....
src/tracing/timeline_viewport.js:167: var yChanged = state.panY !=
this.currentDisplayTransform_.panY;
nit: !==

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_viewport....
src/tracing/timeline_viewport.js:193: this.lastAdvanceTime_ = frameBeginTime -
16.667;
Is 16.667 always correct, or is this just our best estimate? Should it be a
constant?

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_viewport....
src/tracing/timeline_viewport.js:208:
tempDisplayTransform.set(this.goalDisplayTransform_);
This sets the tempDisplayTransform, and we stop animating, should it be doing:

currentDisplayTransform.set(this.goalDisplayTransform_);

Otherwise, isn't currentDisplayTransform going to end up being the step before
the last, minus any almostEqual differences?

https://codereview.appspot.com/12451045/diff/1/src/tracing/timeline_viewport....
src/tracing/timeline_viewport.js:231: // This code direclty manipulates the
current and goal states directly,
nit: s/direclty/directly

[dsinclair, 2013-08-09 02:36:39]

https://codereview.appspot.com/12451045/diff/10001/src/base/utils.js
File src/base/utils.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/base/utils.js#newcode75
src/base/utils.js:75: function lerp(perc, lo, hi) {
perc, now you're just screwing with me. What is perc short for?

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
File src/tracing/timeline_display_transform.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
src/tracing/timeline_display_transform.js:64: viewWidth, opt_scalingFocalPoint)
{
nit: indenting

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
File src/tracing/timeline_display_transform_animations.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
src/tracing/timeline_display_transform_animations.js:11: function
TimelineDisplayTransformPanAnimation(deltaX, deltaY) {
Do these really need TimelineDisplayTransform in the name? They don't appear to
use it specifically.

I'm guessing you'll hate this idea but what about:
src/tracing/animation/pan.js and src/tracing/animation/zoom.js? Then the class
becomes tracing.animation.Pan and tracing.animation.Zoom

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
src/tracing/timeline_display_transform_animations.js:12: this.deltaX = deltaX;
What does the delta mean? We want to get to within delta of our target? or we
want to get delta past our target?

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
src/tracing/timeline_display_transform_animations.js:29: start:
function(timestamp, target) {
What is timestamp for, seems like neither of these classes use it?

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
src/tracing/timeline_display_transform_animations.js:35: target.panX =
this.goalPanX;
Oh, so is the idea that, this class directly implements the behaviour as defined
now (i.e. jump from begin to end) but, in a next Cl the tick/start functions
will be updated to something like:

target.panY = target.panY + (timestamp - last_move_time) * distance_per_ms?

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
src/tracing/timeline_display_transform_animations.js:42: * @constructor
Could we get some @param on these?

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
src/tracing/timeline_display_transform_animations.js:47: zoomInRatioX) {
nit: arguments wrapped weird.

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
src/tracing/timeline_display_transform_animations.js:64: },
throw new Error("not implemented")?

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
File src/tracing/timeline_display_transform_test.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
src/tracing/timeline_display_transform_test.js:31: assertEquals(true,
a.equals(c));
assertTrue(a.equals(c));

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_track...
File src/tracing/timeline_track_view.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_track...
src/tracing/timeline_track_view.js:439: smooth == !!smooth;
Don't think you want ==.

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_track...
src/tracing/timeline_track_view.js:448: var animation = new
tracing.TimelineDisplayTransformZoomToAnimation(
Could the if (smooth) bit be hidden inside the Animation? i.e. they take a
smooth parameter which tells them to jump straight there or not?

Would make the users easier as they just have one code path.

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_track...
src/tracing/timeline_track_view.js:496: dt =
this.viewport.currentDisplayTransform;
nit: var?

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_track...
src/tracing/timeline_track_view.js:730: tempDisplayTransform);
nit: fits above.

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_viewp...
File src/tracing/timeline_viewport.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_viewp...
src/tracing/timeline_viewport.js:155: 'did-tick', function(e) {
I assume did-tick happens after a tick? What about posttick? So, we could have a
pretick in the future if we needed? Seems nicer then did-tick and will-tick (the
- seems inconsitent)

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_viewp...
src/tracing/timeline_viewport.js:192:
that.modelTrackContainer_.canvas.clientWidth);
nit: fits above.

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_viewp...
src/tracing/timeline_viewport.js:216: didCurentDisplayTransformChange_:
function(oldDisplayTransform) {
notifyIfCurrentDisplayTransformChanged_ ?

The did makes it sound like it will return true or false which isn't the case.

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_viewp...
src/tracing/timeline_viewport.js:226: var yChanged =
this.currentDisplayTransform.panY !=
nit: !==

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_viewp...
src/tracing/timeline_viewport.js:235:
this.dtAnimationController_.cancelQueuedAnimations();
So, if we're in the middle of animating a zoom and I use the keyboard to scroll
up the model track, could I cancel my zoom half way through?

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_viewp...
File src/tracing/timeline_viewport_test.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_viewp...
src/tracing/timeline_viewport_test.js:7:
base.require('tracing.timeline_display_transform');
This seems wrong? We should be importing the viewport for the viewport_test.

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation.js
File src/ui/animation.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation.js#newcode9
src/ui/animation.js:9: * @constructor
Docs?

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation.js#newcode30
src/ui/animation.js:30: * do what it wants here, usually nothing.
@param docs?

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation.js#newcode33
src/ui/animation.js:33: willBeTakenOverByAnotherAnimation) {
nit: indenting.

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
File src/ui/animation_controller.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
src/ui/animation_controller.js:17: * animable states of that target.
s/animable/animatable?

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
src/ui/animation_controller.js:18: *
Some classes have a inherits param, or something similar, should list
EventTarget here?

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
src/ui/animation_controller.js:43: throw new Error('target must have a clone
function');
s/clone/cloneAnimationState

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
src/ui/animation_controller.js:69: this.tickActiveAnimation_, this);
nit: fits above?

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
src/ui/animation_controller.js:72: cancelQueuedAnimations: function() {
There can be only one animation, so cancelQueuedAnimation?

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
src/ui/animation_controller.js:73: this.activeAnimation_ = undefined;
Shouldn't this do a didStopEarly call?

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
src/ui/animation_controller.js:90: this.tickActiveAnimation_, this);
nit: fits above?

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
File src/ui/animation_controller_test.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
src/ui/animation_controller_test.js:12: * @constructor
remove?

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
src/ui/animation_controller_test.js:17: this.didStart = false;
Should animation expose a stop callback that is called when we're finished
ticking? Or do we assume any cleanup will be done just before returning true in
tick?

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
src/ui/animation_controller_test.js:73: test('queueTwo', function() {
Add a test for cancel and verify didStopEarly was called?

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
src/ui/animation_controller_test.js:91: while (controller.hasActiveAnimation &&
fakeNow < 1000) {
Why run to 1000 when we know it will finish after 100? Why not finish at 150 or
something?

[nduca, 2013-08-09 09:14:18]

some comments

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
File src/tracing/timeline_display_transform_animations.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
src/tracing/timeline_display_transform_animations.js:11: function
TimelineDisplayTransformPanAnimation(deltaX, deltaY) {
On 2013/08/09 02:36:39, dsinclair wrote:
> Do these really need TimelineDisplayTransform in the name? They don't appear
to
> use it specifically.
the target that it mutates must be a TimelineDisplayTransform. The pan animation
doesn't, but the 

> 
> I'm guessing you'll hate this idea but what about:
> src/tracing/animation/pan.js and src/tracing/animation/zoom.js? Then the class
> becomes tracing.animation.Pan and tracing.animation.Zoom

Heheh. The issue with that is that its not a generalized animation but rather an
animation specific to the panX/panY/scaleX semantics. I think when I upload a
patch that includes the zoom variant, you'll see how typed this is.

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
src/tracing/timeline_display_transform_animations.js:12: this.deltaX = deltaX;
The idea here is that we the animation is supposed to move whatever target by a
certain amount in x and y. We dont know the actual value that that means until
we know the target, which you dont get until start() is called...

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
src/tracing/timeline_display_transform_animations.js:35: target.panX =
this.goalPanX;
there you go :)

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
src/tracing/timeline_display_transform_animations.js:47: zoomInRatioX) {
sorry, how? i think this is correct style..

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_track...
File src/tracing/timeline_track_view.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_track...
src/tracing/timeline_track_view.js:448: var animation = new
tracing.TimelineDisplayTransformZoomToAnimation(
In doing it this way, I weighed the surface area risk with the complexity of
supporting "instant" animations. It was easier to support cancelation of
animations and direct setting of the transform, then making the animation
controller support things that end the second they're enqueued. I think I
*could* do it, but it gets hairy. I think the interface on the viewport right
now strikes a pretty decent balance: there are two setters - one animated and
one direct, and the animation controller is still pretty lean..

Not saying it has to be this way, just this is how I got to this place. :)

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_viewp...
File src/tracing/timeline_viewport.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_viewp...
src/tracing/timeline_viewport.js:155: 'did-tick', function(e) {
i could do didtick.

i try to use did/will because its clearer than using tenses (clicked, ticked,
etc). post is funy because post is a synonym of queue (thin: posttask) which is
definitely not the association we want. did/will is something that does a great
job of being unambiguous, albeit a bit funky. Blink<->Chrome API does this a lot
and I <3 it. Personally, I'd rather we do this more :)

suggestions on what to do, given this?

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_viewp...
src/tracing/timeline_viewport.js:235:
this.dtAnimationController_.cancelQueuedAnimations();
yes in the first pass. things get funky if the current animation is itself
animating Y --- Pan and ZoomTo can (they dont always). A simple followup would
be to ask the animaiton controller for its active animation and ask that if it
mutates Y, and only cancel if it does.

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation.js
File src/ui/animation.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation.js#newcode33
src/ui/animation.js:33: willBeTakenOverByAnotherAnimation) {
sorry, this is valid, no? You can indent the arglist by 4, or aligned with the
(...

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
File src/ui/animation_controller.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
src/ui/animation_controller.js:18: *
hmm? http://usejsdoc.org/ didn't suggest anything obvious

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
File src/ui/animation_controller_test.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
src/ui/animation_controller_test.js:17: this.didStart = false;
You know, I started added it, but I didn't really have a use for it. Seems like
it could be easy to add in the future...

[dsinclair, 2013-08-09 13:59:54]

lgtm with nits

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
File src/tracing/timeline_display_transform_animations.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_displ...
src/tracing/timeline_display_transform_animations.js:47: zoomInRatioX) {
On 2013/08/09 09:14:18, nduca wrote:
> sorry, how? i think this is correct style..

I thought the rule was all on the 2nd line, or one per line?
So, shouldn't the goalFocalPointXView be on the line after goalFocalPointXWorld?

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_track...
File src/tracing/timeline_track_view.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_track...
src/tracing/timeline_track_view.js:448: var animation = new
tracing.TimelineDisplayTransformZoomToAnimation(
On 2013/08/09 09:14:18, nduca wrote:
> In doing it this way, I weighed the surface area risk with the complexity of
> supporting "instant" animations. It was easier to support cancelation of
> animations and direct setting of the transform, then making the animation
> controller support things that end the second they're enqueued. I think I
> *could* do it, but it gets hairy. I think the interface on the viewport right
> now strikes a pretty decent balance: there are two setters - one animated and
> one direct, and the animation controller is still pretty lean..
> 
> Not saying it has to be this way, just this is how I got to this place. :)

sgtm

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_viewp...
File src/tracing/timeline_viewport.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/tracing/timeline_viewp...
src/tracing/timeline_viewport.js:155: 'did-tick', function(e) {
On 2013/08/09 09:14:18, nduca wrote:
> i could do didtick.
> 
> i try to use did/will because its clearer than using tenses (clicked, ticked,
> etc). post is funy because post is a synonym of queue (thin: posttask) which
is
> definitely not the association we want. did/will is something that does a
great
> job of being unambiguous, albeit a bit funky. Blink<->Chrome API does this a
lot
> and I <3 it. Personally, I'd rather we do this more :)
> 
> suggestions on what to do, given this?

didtick is good to me.

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation.js
File src/ui/animation.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation.js#newcode33
src/ui/animation.js:33: willBeTakenOverByAnotherAnimation) {
On 2013/08/09 09:14:18, nduca wrote:
> sorry, this is valid, no? You can indent the arglist by 4, or aligned with the
> (...


Sorry, off by one bug in my line reading. Looked like the w was under the ( not
the t.

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
File src/ui/animation_controller.js (right):

https://codereview.appspot.com/12451045/diff/10001/src/ui/animation_controlle...
src/ui/animation_controller.js:18: *
On 2013/08/09 09:14:18, nduca wrote:
> hmm? http://usejsdoc.org/ didn't suggest anything obvious

* @extends {HeadingTrack}

is what we have in some other places.

https://codereview.appspot.com/12451045/diff/21001/src/tracing/timeline_displ...
File src/tracing/timeline_display_transform_animations.js (right):

https://codereview.appspot.com/12451045/diff/21001/src/tracing/timeline_displ...
src/tracing/timeline_display_transform_animations.js:26: this.durationMs =
opt_durationMs;
this.durationMs = opt_durationMs || kDefaultPanAnimationDurationMs;

https://codereview.appspot.com/12451045/diff/21001/src/tracing/timeline_displ...
src/tracing/timeline_display_transform_animations.js:86: * in order to
compenate. Thus, a ZoomTo animation is given both a focal point
s/compenate/compensate

https://codereview.appspot.com/12451045/diff/21001/src/tracing/timeline_displ...
src/tracing/timeline_display_transform_animations.js:119: },
Should this throw? or, remove it so the default ui.Animation will take over and
throw?

https://codereview.appspot.com/12451045/diff/21001/src/ui/animation.js
File src/ui/animation.js (right):

https://codereview.appspot.com/12451045/diff/21001/src/ui/animation.js#newcode50
src/ui/animation.js:50: * Called when an animation is stopped befre it finishes.
The animation can
s/befre/before

https://codereview.appspot.com/12451045/diff/21001/src/ui/animation.js#newcode54
src/ui/animation.js:54: * @param {Object} target The object being animated.
May want to add that this target maybe undefined as per the if check in
tickAnimation.

https://codereview.appspot.com/12451045/diff/21001/src/ui/animation_controlle...
File src/ui/animation_controller.js (right):

https://codereview.appspot.com/12451045/diff/21001/src/ui/animation_controlle...
src/ui/animation_controller.js:60: now = window.performance.now();
now = opt_now || window.performance.now();

https://codereview.appspot.com/12451045/diff/21001/src/ui/animation_controlle...
src/ui/animation_controller.js:66: var done = this.activeAnimation_.tick(now,
this.target_);
Is it worthwhile to tick this outside the inner-if, so in the case where we
can't take over, it's possible the animation would have finished in that
intervening amount of time and we don't have to didStopEarly?

https://codereview.appspot.com/12451045/diff/21001/src/ui/animation_controlle...
src/ui/animation_controller.js:72: this.activeAnimation_.didStopEarly(now,
this.target_, false);
this.cancelActiveAnimation(now);

https://codereview.appspot.com/12451045/diff/21001/src/ui/animation_controlle...
src/ui/animation_controller.js:91: now = window.performance.now();
now = ...

[nduca, 2013-08-09 21:09:46]

Committed patchset #4 manually as r851 (presubmit successful).