Calculate animation_floor_time_ only when actually updating animations

Calculate animation_floor_time_ only when actually updating animations

This ensures that imperative animations still hit the target framerate if
we can render fast enough regardless of when in the animation loop the page
calls webkitRequestAnimationFrame().

BUG=71379
TEST=

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

[jamesr, 2011-01-31 09:16:24]

Here's a test page to check the behavior of this patch:
http://webstuff.nfshost.com/anim-timing/raftime.html

I've also dropped the target framerate to 60FPS (actually 1000/16 = 62.5 FPS) as
with this less retarded scheduling it seems that we get much closer to the
target.

[darin (slow to review), 2011-01-31 19:01:08]

http://codereview.chromium.org/6409007/diff/2002/chrome/renderer/render_widge...
File chrome/renderer/render_widget.cc (left):

http://codereview.chromium.org/6409007/diff/2002/chrome/renderer/render_widge...
chrome/renderer/render_widget.cc:524: UpdateAnimationsIfNeeded();
it seems like you should still consider calling WebWidget::animate() here if it
is time to do so.  this will help you not fall behind in extreme cases, no?

[jamesr, 2011-01-31 21:43:32]

Good point.  I've added that call back and added another flag to ensure that we
don't call animate() more than once per paint if the animate() call causes
invalidations.

[darin (slow to review), 2011-01-31 22:25:57]

http://codereview.chromium.org/6409007/diff/1002/chrome/renderer/render_widge...
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/6409007/diff/1002/chrome/renderer/render_widge...
chrome/renderer/render_widget.cc:497: base::TimeDelta::FromMilliseconds(16); //
Target 60FPS.
nit: 4 space indent instead of 3
nit: 2 spaces before a comment at the end of a statement

http://codereview.chromium.org/6409007/diff/1002/chrome/renderer/render_widge...
chrome/renderer/render_widget.cc:501: animation_waiting_for_paint_ = true;
nit: 2 space indent

http://codereview.chromium.org/6409007/diff/1002/chrome/renderer/render_widge...
chrome/renderer/render_widget.cc:513: (animation_floor_time_ -
now).InMillisecondsRoundedUp());
if we are waiting for paint, then why bother scheduling another call to
UpdateAnimationsIfNeeded?

http://codereview.chromium.org/6409007/diff/1002/chrome/renderer/render_widge...
chrome/renderer/render_widget.cc:532: UpdateAnimationsIfNeeded();
nit: 2 space indent

http://codereview.chromium.org/6409007/diff/1002/chrome/renderer/render_widge...
chrome/renderer/render_widget.cc:532: UpdateAnimationsIfNeeded();
maybe this should just call animate() directly?  you don't really need to call
layout() here since it will be called below.  you also don't need to kick the
animation timer since that'll happen when scheduleAnimation is called again,
right?

[jamesr, 2011-02-01 03:12:36]

Hopefully this is slightly clearer.  I've started a brief document including a
state transition diagram here:
https://sites.google.com/a/chromium.org/dev/developers/design-documents/reque...
that might help.

[jamesr, 2011-02-01 03:16:45]

On 2011/01/31 22:25:57, darin wrote:
> nit: 2 space indent (and others)

oops :) new .vimrc, hopefully I've sorted out the indentation issues.  I'm
spoiled by webkit-patch upload running lint before actually uploading.

> 
>
http://codereview.chromium.org/6409007/diff/1002/chrome/renderer/render_widge...
> chrome/renderer/render_widget.cc:513: (animation_floor_time_ -
> now).InMillisecondsRoundedUp());
> if we are waiting for paint, then why bother scheduling another call to
> UpdateAnimationsIfNeeded?
> 

Right - this isn't necessary and is removed.

> 
>
http://codereview.chromium.org/6409007/diff/1002/chrome/renderer/render_widge...
> chrome/renderer/render_widget.cc:532: UpdateAnimationsIfNeeded();
> maybe this should just call animate() directly?  you don't really need to call
> layout() here since it will be called below.  you also don't need to kick the
> animation timer since that'll happen when scheduleAnimation is called again,
> right?

Updated to call UpdateAnimationsAndFloorTime(), which calls animate() and also
updates the animation floor time and pending state.  The one tricky case in
DoDeferredUpdate() is that we get into this situation:

scheduleAnimation() called
animate() called, causes invalidations, calls scheduleAnimation() again
(some crap gunks up the message loop for a while)
DoDeferredUpdate() runs - at this point we have a pending update and we're ahead
of the animation floor time, but we don't want to call animate() again because
then we'd be doing the work for 2 frames while only painting one. What this code
does is post a task to call UpdateAnimationsIfNeeded() after DoDeferredUpdate()
so it can spin up the next frame if necessary.  I left a comment in
DoDeferredUpdate that I hope clarifies this (although I'm sure Maciej would
disagree).

[darin (slow to review), 2011-02-03 20:46:54]

LGTM !