Tweaks and fixes to the SMILTimeContainer frame scheduling logic

Tweaks and fixes to the SMILTimeContainer frame scheduling logic

Restore the old frame delay as |initialFrameDelay| and use that in
begin(). (There's a 15ms difference from DocumentTimeline::s_minimumDelay,
in case that matters to some layout test.)

Use a separate state (FutureAnimationFrame) for the cases where an
animation frame is requested later than the next frame interval. This
avoids unnecessary work in the frame callback (which is gated on the
state being AnimationFrame).

Fix an issue where an animation frame could end up being scheduled,
although a synchronization had been triggered already (via
updateAnimations() and a syncbase dependency for instance). Similarly also
avoid increasing the timer in begin() for similar cases.

BUG=231576
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169259

[fs, 2014-03-10 18:07:07]

Iteration on https://codereview.chromium.org/190963003/ - PTAL

[pdr., 2014-03-13 06:47:56]

On 2014/03/10 18:07:07, fs wrote:
> Iteration on https://codereview.chromium.org/190963003/ - PTAL

Sorry about the long review delays (just got back from a 4 day weekend).

This looks like a nice cleanup, LGTM for that.

Because there's a small change in behavior due to the delay in
SMILTimeContainer::begin, it would be nice to add a test so we don't regress it.
That may be hard in this case. Do you think this is testable (reliably)?

[fs, 2014-03-13 09:54:57]

On 2014/03/13 06:47:56, pdr wrote:
> On 2014/03/10 18:07:07, fs wrote:
> > Iteration on https://codereview.chromium.org/190963003/ - PTAL
> 
> Sorry about the long review delays (just got back from a 4 day weekend).
> 
> This looks like a nice cleanup, LGTM for that.
> 
> Because there's a small change in behavior due to the delay in
> SMILTimeContainer::begin, it would be nice to add a test so we don't regress
it.
> That may be hard in this case. Do you think this is testable (reliably)?

There should be plenty of tests affected by that change - for example all the
animate-elem-* which proved to difficult for the initial version of the
rAF-patch. The thing that initially prompted me to do the change was some
test(s) I looked at that setTimeout'd a number of tenths of milliseconds from
'onload' to complete the test (and expected a visual change to have been
effected at that time). So I believe there should be some coverage already. In
the end though it's just a quirk for those tests that sample the first frame and
expcet that to be at zero. Hopefully that can be "fixed" properly at some later
point in time...

[pdr., 2014-03-13 18:33:22]

On 2014/03/13 09:54:57, fs wrote:
> On 2014/03/13 06:47:56, pdr wrote:
> > On 2014/03/10 18:07:07, fs wrote:
> > > Iteration on https://codereview.chromium.org/190963003/ - PTAL
> > 
> > Sorry about the long review delays (just got back from a 4 day weekend).
> > 
> > This looks like a nice cleanup, LGTM for that.
> > 
> > Because there's a small change in behavior due to the delay in
> > SMILTimeContainer::begin, it would be nice to add a test so we don't regress
> it.
> > That may be hard in this case. Do you think this is testable (reliably)?
> 
> There should be plenty of tests affected by that change - for example all the
> animate-elem-* which proved to difficult for the initial version of the
> rAF-patch. The thing that initially prompted me to do the change was some
> test(s) I looked at that setTimeout'd a number of tenths of milliseconds from
> 'onload' to complete the test (and expected a visual change to have been
> effected at that time). So I believe there should be some coverage already. In
> the end though it's just a quirk for those tests that sample the first frame
and
> expcet that to be at zero. Hopefully that can be "fixed" properly at some
later
> point in time...

LGTM

[pdr., 2014-03-13 18:33:27]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-03-13 18:33:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/193053002/20001

[commit-bot: I haz the power, 2014-03-13 18:33:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 18:33:54]

Try jobs failed on following builders:
  tryserver.blink on blink_presubmit
  tryserver.blink on linux_blink_rel

[fs, 2014-03-14 16:10:46]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-03-14 16:10:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/193053002/40001

[commit-bot: I haz the power, 2014-03-14 16:11:16]

Change committed as 169259