Web Animations: Fix CSS events to handle animations with very short durations

Web Animations: Fix CSS events to handle animations with very short durations

In order to do this reliably we need to handle the case where an animation
starts and ends within a single sample.

This is tested with a new LayoutTest which uses a CSS animation with a very 
short duration and a non-zero delay, as animations are always sampled at an
inherited time of zero.

BUG=240653
R=dstockwell

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

[Steve Block, 2013-08-20 06:49:15]

Note that this depends on https://codereview.chromium.org/23327003

[dstockwell, 2013-08-21 02:29:44]

https://codereview.chromium.org/23173007/diff/6001/Source/core/animation/Time...
File Source/core/animation/TimedItem.cpp (right):

https://codereview.chromium.org/23173007/diff/6001/Source/core/animation/Time...
Source/core/animation/TimedItem.cpp:113: if (m_eventDelegate && (isInPlay() !=
wasInPlay || isCurrent() != wasCurrent || (wasInPlay && isInPlay() &&
lastIteration != currentIteration)))
This is getting complicated. I wonder if instead of looking at a change to
isCurrent (and perhaps even isInPlay) whether we can get by just looking at
changes to the phase (and iteration)? This might also avoid needing that first
forced sample.

I think None -> !None = started. !After -> After = finished.

[dstockwell, 2013-08-21 04:37:35]

https://codereview.chromium.org/23173007/diff/6001/Source/core/animation/Time...
File Source/core/animation/TimedItem.cpp (right):

https://codereview.chromium.org/23173007/diff/6001/Source/core/animation/Time...
Source/core/animation/TimedItem.cpp:113: if (m_eventDelegate && (isInPlay() !=
wasInPlay || isCurrent() != wasCurrent || (wasInPlay && isInPlay() &&
lastIteration != currentIteration)))
On 2013/08/21 02:29:44, dstockwell wrote:
> This is getting complicated. I wonder if instead of looking at a change to
> isCurrent (and perhaps even isInPlay) whether we can get by just looking at
> changes to the phase (and iteration)? This might also avoid needing that first
> forced sample.
> 
> I think None -> !None = started. !After -> After = finished.

Oops, I guess that's: None -> (Active | After) = started

[Steve Block, 2013-08-21 05:55:16]

> This is getting complicated. I wonder if instead of looking at a change to
> isCurrent (and perhaps even isInPlay) whether we can get by just looking at
> changes to the phase (and iteration)?
OK, I'll try this. Initially I decided not to use this approach as it makes it
more difficult to enforce such a tight condition on when to make the event
callback in TimedItem::updateInheritedTime(). It's probably OK to relax this
condition though.

> None -> (Active | After) = started
In CSS animations, we should never see PhaseNone, so I assume you're referring
to using PhaseNone as an initial value (as we do currently). This logic is OK
for CSS animations, as the start event is always fired when the animation is
first sampled, even if the start delay is negative. In general however, on the
first sample, we can't determine anything about events.

It seems rather ugly for TimedItem to use PhaseNone as a special signal for
'this is the first sample' for the purposes of CSS animations, when in the
general case, PhaseNone means something different. My intent with the first
forced sample was to move this special-casing for the first sample out of
TimedItem and into CSS-specific logic. Perhaps we could compromise and have
TimedItem pass an explicit isFirstSample flag to the event callback for use by
CSS animations, which would avoid the need for the forced sample.

[Steve Block, 2013-08-21 05:59:33]

Ready for another look

[dstockwell, 2013-08-21 06:18:01]

lgtm

https://codereview.chromium.org/23173007/diff/15001/Source/core/animation/Tim...
File Source/core/animation/TimedItem.cpp (right):

https://codereview.chromium.org/23173007/diff/15001/Source/core/animation/Tim...
Source/core/animation/TimedItem.cpp:62: m_calculated.phase =
calculatePhase(activeDuration, localTime, m_specified);
Can you mirror the pattern of other calculated values. Don't update m_calculated
until ~line 93.

https://codereview.chromium.org/23173007/diff/15001/Source/core/animation/Tim...
File Source/core/animation/TimedItemTest.cpp (left):

https://codereview.chromium.org/23173007/diff/15001/Source/core/animation/Tim...
Source/core/animation/TimedItemTest.cpp:120: ASSERT_EQ(0,
timedItem->timeFraction());
test phase()

[dstockwell, 2013-08-21 06:21:52]

https://codereview.chromium.org/23173007/diff/15001/Source/core/animation/css...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/23173007/diff/15001/Source/core/animation/css...
Source/core/animation/css/CSSAnimations.cpp:238: void
CSSAnimations::EventDelegate::onEventCondition(bool isFirstSample,
TimedItem::Phase previousPhase, TimedItem::Phase phase, double
previousIteration, double currentIteration)
phase -> currentPhase or currentIteration -> iteration for consistency

[commit-bot: I haz the power, 2013-08-22 00:44:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/steveblock@chromium.org/23173007/35001

[commit-bot: I haz the power, 2013-08-22 01:10:29]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-08-22 01:33:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/steveblock@chromium.org/23173007/50001

[commit-bot: I haz the power, 2013-08-22 04:37:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/steveblock@chromium.org/23173007/61001

[commit-bot: I haz the power, 2013-08-22 04:48:09]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-08-22 04:49:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/steveblock@chromium.org/23173007/69001

[commit-bot: I haz the power, 2013-08-22 06:10:18]

Change committed as 156546