Update animation when changes in animation keyframes are detected.

Update animation when changes in animation keyframes are detected.

This patch detects changes in the keyframe rules of animations, and
updates the animating elements accordingly.

Firefox allows keyframes to be changed, but IE doesn't.

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

[shend, 2014-12-29 00:08:56]

shend@chromium.org changed reviewers:
+ alancutter@chromium.org, dstockwell@chromium.org, timloh@chromium.org

[shend, 2014-12-29 00:08:57]

Added code to detect changes in keyframes (via appendrule/deleterule). Also
modified the way changes to timing are handled in order to detect changes in the
easing.

[dstockwell, 2014-12-29 01:59:52]

https://codereview.chromium.org/814083003/diff/40001/Source/core/animation/An...
File Source/core/animation/AnimationNode.h (right):

https://codereview.chromium.org/814083003/diff/40001/Source/core/animation/An...
Source/core/animation/AnimationNode.h:172: unsigned m_styleChangeCounter;
This seems specific to CSSAnimations, we should store it in CSSAnimations'
m_animations map. Follow the same pattern as the TransitionMap with
RunningTransition.

[shend, 2014-12-29 03:21:37]

https://codereview.chromium.org/814083003/diff/40001/Source/core/animation/An...
File Source/core/animation/AnimationNode.h (right):

https://codereview.chromium.org/814083003/diff/40001/Source/core/animation/An...
Source/core/animation/AnimationNode.h:172: unsigned m_styleChangeCounter;
On 2014/12/29 01:59:52, dstockwell wrote:
> This seems specific to CSSAnimations, we should store it in CSSAnimations'
> m_animations map. Follow the same pattern as the TransitionMap with
> RunningTransition.

Done.

[dstockwell, 2014-12-29 23:16:34]

https://codereview.chromium.org/814083003/diff/60001/Source/core/animation/cs...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/814083003/diff/60001/Source/core/animation/cs...
Source/core/animation/css/CSSAnimations.cpp:291: if
(keyframesRule->styleChangeCounter() > runningAnimation.styleChangeCounter ||
runningAnimation.specifiedTiming != newTiming) {
Can `keyframesRule->styleChangeCounter() < runningAnimation.styleChangeCounter`
ever occur? If not, just use `!=`

https://codereview.chromium.org/814083003/diff/60001/Source/core/animation/cs...
Source/core/animation/css/CSSAnimations.cpp:381:
runningAnimation.styleChangeCounter = 0;
Shouldn't this be initialized with the current value of the counter?

https://codereview.chromium.org/814083003/diff/60001/Source/core/animation/cs...
Source/core/animation/css/CSSAnimations.cpp:381:
runningAnimation.styleChangeCounter = 0;
Shouldn't this be initialized to the current value of the counter?

https://codereview.chromium.org/814083003/diff/60001/Source/core/animation/cs...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/814083003/diff/60001/Source/core/animation/cs...
Source/core/animation/css/CSSAnimations.h:114: Timing newTiming;
Why newTiming rather than just timing?

https://codereview.chromium.org/814083003/diff/60001/Source/core/css/CSSKeyfr...
File Source/core/css/CSSKeyframeRule.h (right):

https://codereview.chromium.org/814083003/diff/60001/Source/core/css/CSSKeyfr...
Source/core/css/CSSKeyframeRule.h:69: bool operator== (const StyleKeyframe&
other) const { return keys() == keys() && cssText() == other.cssText(); }
This seems expensive, as cssText() builds a new string on each call.

https://codereview.chromium.org/814083003/diff/60001/Source/core/css/resolver...
File Source/core/css/resolver/ScopedStyleResolver.cpp (right):

https://codereview.chromium.org/814083003/diff/60001/Source/core/css/resolver...
Source/core/css/resolver/ScopedStyleResolver.cpp:110: if (it ==
m_keyframesRuleMap.end() || !(*rule == *it->value))
It shouldn't matter to set styleChanged even when `*rule == *it->value`, it's
only a minor optimization to avoid it.

[shend, 2014-12-30 00:03:40]

https://codereview.chromium.org/814083003/diff/60001/Source/core/animation/cs...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/814083003/diff/60001/Source/core/animation/cs...
Source/core/animation/css/CSSAnimations.cpp:291: if
(keyframesRule->styleChangeCounter() > runningAnimation.styleChangeCounter ||
runningAnimation.specifiedTiming != newTiming) {
On 2014/12/29 23:16:33, dstockwell wrote:
> Can `keyframesRule->styleChangeCounter() <
runningAnimation.styleChangeCounter`
> ever occur? If not, just use `!=`

Done.

https://codereview.chromium.org/814083003/diff/60001/Source/core/animation/cs...
Source/core/animation/css/CSSAnimations.cpp:381:
runningAnimation.styleChangeCounter = 0;
On 2014/12/29 23:16:33, dstockwell wrote:
> Shouldn't this be initialized with the current value of the counter?

Done.

https://codereview.chromium.org/814083003/diff/60001/Source/core/animation/cs...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/814083003/diff/60001/Source/core/animation/cs...
Source/core/animation/css/CSSAnimations.h:114: Timing newTiming;
On 2014/12/29 23:16:33, dstockwell wrote:
> Why newTiming rather than just timing?

Changed to just 'timing'. It was meant to differentiate between the actual
specified timing ('newTiming') and the specified timing fed into web animations
('timing') which always has a timing function of 'linear'.

https://codereview.chromium.org/814083003/diff/60001/Source/core/css/CSSKeyfr...
File Source/core/css/CSSKeyframeRule.h (right):

https://codereview.chromium.org/814083003/diff/60001/Source/core/css/CSSKeyfr...
Source/core/css/CSSKeyframeRule.h:69: bool operator== (const StyleKeyframe&
other) const { return keys() == keys() && cssText() == other.cssText(); }
On 2014/12/29 23:16:33, dstockwell wrote:
> This seems expensive, as cssText() builds a new string on each call.

Removed.

https://codereview.chromium.org/814083003/diff/60001/Source/core/css/resolver...
File Source/core/css/resolver/ScopedStyleResolver.cpp (right):

https://codereview.chromium.org/814083003/diff/60001/Source/core/css/resolver...
Source/core/css/resolver/ScopedStyleResolver.cpp:110: if (it ==
m_keyframesRuleMap.end() || !(*rule == *it->value))
On 2014/12/29 23:16:33, dstockwell wrote:
> It shouldn't matter to set styleChanged even when `*rule == *it->value`, it's
> only a minor optimization to avoid it.

Done.

[dstockwell, 2014-12-30 00:59:12]

https://codereview.chromium.org/814083003/diff/80001/LayoutTests/animations/k...
File LayoutTests/animations/keyframes-cssom-updates-animation.html (right):

https://codereview.chromium.org/814083003/diff/80001/LayoutTests/animations/k...
LayoutTests/animations/keyframes-cssom-updates-animation.html:26: var rules =
document.styleSheets[0].rules;
I think we need some more tests:
* can we use the CSSOM to add a new @keyframes with the same name?
* inject a new stylesheet / style element

https://codereview.chromium.org/814083003/diff/80001/Source/core/animation/cs...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/814083003/diff/80001/Source/core/animation/cs...
Source/core/animation/css/CSSAnimations.cpp:358: Animation* animation =
static_cast<Animation*>(entry.player->source());
use toAnimation() and drop the assert

https://codereview.chromium.org/814083003/diff/80001/Source/core/css/resolver...
File Source/core/css/resolver/ScopedStyleResolver.cpp (right):

https://codereview.chromium.org/814083003/diff/80001/Source/core/css/resolver...
Source/core/css/resolver/ScopedStyleResolver.cpp:111: rule->styleChanged();
I don't quite understand how this works, why are we only calling it when it's
not present in the map?

[shend, 2015-01-02 01:58:29]

Inserting @keyframes rule by appending <style> tags weren't forcing a style
recalc, so animations weren't being updated. These changes should fix it now.

https://codereview.chromium.org/814083003/diff/80001/LayoutTests/animations/k...
File LayoutTests/animations/keyframes-cssom-updates-animation.html (right):

https://codereview.chromium.org/814083003/diff/80001/LayoutTests/animations/k...
LayoutTests/animations/keyframes-cssom-updates-animation.html:26: var rules =
document.styleSheets[0].rules;
On 2014/12/30 00:59:12, dstockwell wrote:
> I think we need some more tests:
> * can we use the CSSOM to add a new @keyframes with the same name?
> * inject a new stylesheet / style element

Done.

https://codereview.chromium.org/814083003/diff/80001/Source/core/animation/cs...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/814083003/diff/80001/Source/core/animation/cs...
Source/core/animation/css/CSSAnimations.cpp:358: Animation* animation =
static_cast<Animation*>(entry.player->source());
On 2014/12/30 00:59:12, dstockwell wrote:
> use toAnimation() and drop the assert

Done.

https://codereview.chromium.org/814083003/diff/80001/Source/core/css/resolver...
File Source/core/css/resolver/ScopedStyleResolver.cpp (right):

https://codereview.chromium.org/814083003/diff/80001/Source/core/css/resolver...
Source/core/css/resolver/ScopedStyleResolver.cpp:111: rule->styleChanged();
On 2014/12/30 00:59:12, dstockwell wrote:
> I don't quite understand how this works, why are we only calling it when it's
> not present in the map?

My bad, it should just invalidate every time this function is called. Changed
now.

[shend, 2015-01-05 06:29:55]

Code now keeps track of a pointer to the current keyframes style rule, and
updates the animation if the pointer address changes.

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
File Source/core/css/resolver/ScopedStyleResolver.h (right):

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
Source/core/css/resolver/ScopedStyleResolver.h:60: const StyleRuleKeyframes*
keyframeStylesForAnimation(const StringImpl* animationName) const;
The const version isn't used, so should we just remove it?

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.h:133: const StyleRuleKeyframes*
findKeyframesRule(const Element*, const AtomicString& animationName) const;
Same as in StyleResolver, the const version isn't used, so should we just remove
it?

[dstockwell, 2015-01-05 07:31:18]

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
File Source/core/css/resolver/ScopedStyleResolver.cpp (right):

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
Source/core/css/resolver/ScopedStyleResolver.cpp:121: rule->styleChanged();
I think on Friday we discussed that we need to call this in one more place: when
we're mutating a keyframe already added to a keyframes rule.

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
File Source/core/css/resolver/ScopedStyleResolver.h (right):

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
Source/core/css/resolver/ScopedStyleResolver.h:60: const StyleRuleKeyframes*
keyframeStylesForAnimation(const StringImpl* animationName) const;
On 2015/01/05 at 06:29:54, shend wrote:
> The const version isn't used, so should we just remove it?

Yes, just remove them.

[shend, 2015-01-06 00:36:18]

Added tests to change the style of an existing keyframe, but it seems to be
already handled in ScopedStyleResolver::addKeyframesRule. Added logic to detect
changes in the key text. Are there other ways to modify a keyframe style that
hasn't been tested?

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
File Source/core/css/resolver/ScopedStyleResolver.cpp (right):

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
Source/core/css/resolver/ScopedStyleResolver.cpp:121: rule->styleChanged();
On 2015/01/05 07:31:18, dstockwell wrote:
> I think on Friday we discussed that we need to call this in one more place:
when
> we're mutating a keyframe already added to a keyframes rule.

Modifying the style of an existing keyframe seems to be handled by this (see
test), but setting the key text has to be separately handled.

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
File Source/core/css/resolver/ScopedStyleResolver.h (right):

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
Source/core/css/resolver/ScopedStyleResolver.h:60: const StyleRuleKeyframes*
keyframeStylesForAnimation(const StringImpl* animationName) const;
On 2015/01/05 07:31:18, dstockwell wrote:
> On 2015/01/05 at 06:29:54, shend wrote:
> > The const version isn't used, so should we just remove it?
> 
> Yes, just remove them.

Done.

https://codereview.chromium.org/814083003/diff/160001/LayoutTests/animations/...
File LayoutTests/animations/keyframes-cssom-updates-animation.html (right):

https://codereview.chromium.org/814083003/diff/160001/LayoutTests/animations/...
LayoutTests/animations/keyframes-cssom-updates-animation.html:78:
assert_equals(parseInt(getComputedStyle(target2).left), 300, 'left offset');
With respect to the discussion on Friday, would there be any way to modify
keyframes that requires the use of a MutationObserver?

https://codereview.chromium.org/814083003/diff/160001/Source/core/css/CSSKeyf...
File Source/core/css/CSSKeyframeRule.cpp (right):

https://codereview.chromium.org/814083003/diff/160001/Source/core/css/CSSKeyf...
Source/core/css/CSSKeyframeRule.cpp:160:
parent->m_keyframesRule->styleChanged();
Should we move this logic into StyleKeyframe so that CSSKeyframeRule doesn't
know anything about style counters? If so, is it okay to keep a pointer in
StyleKeyframe to the parent StyleKeyframes?

[dstockwell, 2015-01-07 00:18:33]

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
File Source/core/css/resolver/ScopedStyleResolver.cpp (right):

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
Source/core/css/resolver/ScopedStyleResolver.cpp:121: rule->styleChanged();
On 2015/01/06 00:36:17, shend wrote:
> On 2015/01/05 07:31:18, dstockwell wrote:
> > I think on Friday we discussed that we need to call this in one more place:
> when
> > we're mutating a keyframe already added to a keyframes rule.
> 
> Modifying the style of an existing keyframe seems to be handled by this (see
> test)

Hmm, but how does that work?

https://codereview.chromium.org/814083003/diff/160001/Source/core/css/CSSKeyf...
File Source/core/css/CSSKeyframeRule.cpp (right):

https://codereview.chromium.org/814083003/diff/160001/Source/core/css/CSSKeyf...
Source/core/css/CSSKeyframeRule.cpp:157: CSSStyleSheet::RuleMutationScope
mutationScope(this);
Is this needed? If it is, appears as though it should be called before the
mutation happens on line 153.

[shend, 2015-01-08 05:05:29]

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
File Source/core/css/resolver/ScopedStyleResolver.cpp (right):

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
Source/core/css/resolver/ScopedStyleResolver.cpp:121: rule->styleChanged();
On 2015/01/07 00:18:33, dstockwell wrote:
> On 2015/01/06 00:36:17, shend wrote:
> > On 2015/01/05 07:31:18, dstockwell wrote:
> > > I think on Friday we discussed that we need to call this in one more
place:
> > when
> > > we're mutating a keyframe already added to a keyframes rule.
> > 
> > Modifying the style of an existing keyframe seems to be handled by this (see
> > test)
> 
> Hmm, but how does that work?

I'm not exactly sure, but if you follow the call tree back up, it looks like
this method is called every time a new StyleResolver is created
(StyleEngine::ensureResolver), and a new StyleResolver seems to be created every
time I change the style of a keyframe. Is this the expected behaviour?

https://codereview.chromium.org/814083003/diff/160001/Source/core/css/CSSKeyf...
File Source/core/css/CSSKeyframeRule.cpp (right):

https://codereview.chromium.org/814083003/diff/160001/Source/core/css/CSSKeyf...
Source/core/css/CSSKeyframeRule.cpp:157: CSSStyleSheet::RuleMutationScope
mutationScope(this);
On 2015/01/07 00:18:33, dstockwell wrote:
> Is this needed? If it is, appears as though it should be called before the
> mutation happens on line 153.

Oops, you're right.

[dstockwell, 2015-01-08 05:27:23]

dstockwell@chromium.org changed reviewers:
+ shans@chromium.org

[dstockwell, 2015-01-08 05:27:24]

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
File Source/core/css/resolver/ScopedStyleResolver.cpp (right):

https://codereview.chromium.org/814083003/diff/140001/Source/core/css/resolve...
Source/core/css/resolver/ScopedStyleResolver.cpp:121: rule->styleChanged();
On 2015/01/08 at 05:05:29, shend wrote:
> On 2015/01/07 00:18:33, dstockwell wrote:
> > On 2015/01/06 00:36:17, shend wrote:
> > > On 2015/01/05 07:31:18, dstockwell wrote:
> > > > I think on Friday we discussed that we need to call this in one more
place:
> > > when
> > > > we're mutating a keyframe already added to a keyframes rule.
> > > 
> > > Modifying the style of an existing keyframe seems to be handled by this
(see
> > > test)
> > 
> > Hmm, but how does that work?
> 
> I'm not exactly sure, but if you follow the call tree back up, it looks like
this method is called every time a new StyleResolver is created
(StyleEngine::ensureResolver), and a new StyleResolver seems to be created every
time I change the style of a keyframe. Is this the expected behaviour?

Ahh, I see! I think I misunderstood the purpose of this method. I think this
means that any time the style sheet is mutated (and the style resolver cleared)
we will bump the counts on all keyframes. @shans, wdyt?

[Timothy Loh, 2015-01-08 05:28:31]

https://codereview.chromium.org/814083003/diff/200001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/814083003/diff/200001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.cpp:293: unsigned styleChangeCounter =
keyframesRule ? keyframesRule->styleChangeCounter() : 0;
keyframesRule isn't null (there's an early out above)

https://codereview.chromium.org/814083003/diff/200001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/814083003/diff/200001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.h:58: void startAnimation(const
AtomicString& animationName, PassRefPtrWillBeRawPtr<InertAnimation> animation,
const Timing& timing, PassRefPtrWillBeRawPtr<StyleRuleKeyframes> styleRule,
unsigned styleChangeCounter)
I wouldn't bother passing in the counter here since you can just get it from the
rule

https://codereview.chromium.org/814083003/diff/200001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.h:80: void updateAnimation(const
AtomicString& name, AnimationPlayer* player,
PassRefPtrWillBeRawPtr<InertAnimation> animation, const Timing& specifiedTiming,
PassRefPtrWillBeRawPtr<StyleRuleKeyframes> styleRule, unsigned
styleChangeCounter)
As above, just get the counter from the rule instead of using an additional
argument

[shend, 2015-01-08 06:29:48]

https://codereview.chromium.org/814083003/diff/200001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/814083003/diff/200001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.cpp:293: unsigned styleChangeCounter =
keyframesRule ? keyframesRule->styleChangeCounter() : 0;
On 2015/01/08 05:28:30, Timothy Loh wrote:
> keyframesRule isn't null (there's an early out above)

Done.

https://codereview.chromium.org/814083003/diff/200001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/814083003/diff/200001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.h:58: void startAnimation(const
AtomicString& animationName, PassRefPtrWillBeRawPtr<InertAnimation> animation,
const Timing& timing, PassRefPtrWillBeRawPtr<StyleRuleKeyframes> styleRule,
unsigned styleChangeCounter)
On 2015/01/08 05:28:30, Timothy Loh wrote:
> I wouldn't bother passing in the counter here since you can just get it from
the
> rule

Done.

https://codereview.chromium.org/814083003/diff/200001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.h:80: void updateAnimation(const
AtomicString& name, AnimationPlayer* player,
PassRefPtrWillBeRawPtr<InertAnimation> animation, const Timing& specifiedTiming,
PassRefPtrWillBeRawPtr<StyleRuleKeyframes> styleRule, unsigned
styleChangeCounter)
On 2015/01/08 05:28:31, Timothy Loh wrote:
> As above, just get the counter from the rule instead of using an additional
> argument

Done.

[shend, 2015-01-13 21:58:28]

On 2015/01/08 06:29:48, shend wrote:
>
https://codereview.chromium.org/814083003/diff/200001/Source/core/animation/c...
> File Source/core/animation/css/CSSAnimations.cpp (right):
> 
>
https://codereview.chromium.org/814083003/diff/200001/Source/core/animation/c...
> Source/core/animation/css/CSSAnimations.cpp:293: unsigned styleChangeCounter =
> keyframesRule ? keyframesRule->styleChangeCounter() : 0;
> On 2015/01/08 05:28:30, Timothy Loh wrote:
> > keyframesRule isn't null (there's an early out above)
> 
> Done.
> 
>
https://codereview.chromium.org/814083003/diff/200001/Source/core/animation/c...
> File Source/core/animation/css/CSSAnimations.h (right):
> 
>
https://codereview.chromium.org/814083003/diff/200001/Source/core/animation/c...
> Source/core/animation/css/CSSAnimations.h:58: void startAnimation(const
> AtomicString& animationName, PassRefPtrWillBeRawPtr<InertAnimation> animation,
> const Timing& timing, PassRefPtrWillBeRawPtr<StyleRuleKeyframes> styleRule,
> unsigned styleChangeCounter)
> On 2015/01/08 05:28:30, Timothy Loh wrote:
> > I wouldn't bother passing in the counter here since you can just get it from
> the
> > rule
> 
> Done.
> 
>
https://codereview.chromium.org/814083003/diff/200001/Source/core/animation/c...
> Source/core/animation/css/CSSAnimations.h:80: void updateAnimation(const
> AtomicString& name, AnimationPlayer* player,
> PassRefPtrWillBeRawPtr<InertAnimation> animation, const Timing&
specifiedTiming,
> PassRefPtrWillBeRawPtr<StyleRuleKeyframes> styleRule, unsigned
> styleChangeCounter)
> On 2015/01/08 05:28:31, Timothy Loh wrote:
> > As above, just get the counter from the rule instead of using an additional
> > argument
> 
> Done.

PTAL

[Timothy Loh, 2015-01-13 23:44:00]

On 2015/01/13 21:58:28, shend wrote:
> PTAL

lgtm

[shans, 2015-01-14 00:26:19]

https://codereview.chromium.org/814083003/diff/260001/Source/core/animation/T...
File Source/core/animation/Timing.h (right):

https://codereview.chromium.org/814083003/diff/260001/Source/core/animation/T...
Source/core/animation/Timing.h:97: && *timingFunction == *other.timingFunction;
Does this use pointer comparison, or the reference equality operator defined on
TimingFunction?

https://codereview.chromium.org/814083003/diff/260001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/814083003/diff/260001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.h:86: updatedAnimation.specifiedTiming =
specifiedTiming;
just 'timing' for consistency with InertAnimations?

[shend, 2015-01-14 00:42:04]

https://codereview.chromium.org/814083003/diff/260001/Source/core/animation/T...
File Source/core/animation/Timing.h (right):

https://codereview.chromium.org/814083003/diff/260001/Source/core/animation/T...
Source/core/animation/Timing.h:97: && *timingFunction == *other.timingFunction;
On 2015/01/14 00:26:19, shans wrote:
> Does this use pointer comparison, or the reference equality operator defined
on
> TimingFunction?

Reference equality: it needs to check if the type/parameters of the timing
functions have changed.

https://codereview.chromium.org/814083003/diff/260001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/814083003/diff/260001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.h:86: updatedAnimation.specifiedTiming =
specifiedTiming;
On 2015/01/14 00:26:19, shans wrote:
> just 'timing' for consistency with InertAnimations?

The 'timing' in InertAnimations isn't actually the true specified timing when
created from CSSAnimations, because it always uses the default easing of linear.
In order to detect changes in the easing, I keep track of the actual specified
timing as 'specifiedTiming' (with the actual specified easing). Maybe there's
something we can do to make this clearer?

[shans, 2015-01-15 00:29:52]

https://codereview.chromium.org/814083003/diff/260001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/814083003/diff/260001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.h:86: updatedAnimation.specifiedTiming =
specifiedTiming;
On 2015/01/14 00:42:04, shend wrote:
> On 2015/01/14 00:26:19, shans wrote:
> > just 'timing' for consistency with InertAnimations?
> 
> The 'timing' in InertAnimations isn't actually the true specified timing when
> created from CSSAnimations, because it always uses the default easing of
linear.
> In order to detect changes in the easing, I keep track of the actual specified
> timing as 'specifiedTiming' (with the actual specified easing). Maybe there's
> something we can do to make this clearer?

Could InertAnimations have the easing from the CSSAnimation? That might make
tracking the timing slightly easier too?

[shend, 2015-01-15 01:51:29]

https://codereview.chromium.org/814083003/diff/260001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/814083003/diff/260001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.h:86: updatedAnimation.specifiedTiming =
specifiedTiming;
On 2015/01/15 00:29:52, shans wrote:
> On 2015/01/14 00:42:04, shend wrote:
> > On 2015/01/14 00:26:19, shans wrote:
> > > just 'timing' for consistency with InertAnimations?
> > 
> > The 'timing' in InertAnimations isn't actually the true specified timing
when
> > created from CSSAnimations, because it always uses the default easing of
> linear.
> > In order to detect changes in the easing, I keep track of the actual
specified
> > timing as 'specifiedTiming' (with the actual specified easing). Maybe
there's
> > something we can do to make this clearer?
> 
> Could InertAnimations have the easing from the CSSAnimation? That might make
> tracking the timing slightly easier too?

I originally talked to Sam about this, but we thought that it was better to keep
CSS animation specific things out of web animations. What do you think?

[shend, 2015-01-21 02:49:58]

PTAL

[esprehn, 2015-01-21 03:00:46]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2015-01-21 03:00:47]

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
File Source/core/css/CSSKeyframesRule.h (right):

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
Source/core/css/CSSKeyframesRule.h:65: unsigned styleChangeCounter() const {
return m_styleChangeCounter; }
This doesn't seem like the right way to solve this, why are keyframes special
compared to all the other rule types?

[shend, 2015-01-21 03:25:49]

On 2015/01/21 03:00:47, esprehn wrote:
>
https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
> File Source/core/css/CSSKeyframesRule.h (right):
> 
>
https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
> Source/core/css/CSSKeyframesRule.h:65: unsigned styleChangeCounter() const {
> return m_styleChangeCounter; }
> This doesn't seem like the right way to solve this, why are keyframes special
> compared to all the other rule types?

When keyframe styles are changed, we want to avoid the overhead of re-creating
all the animations. This approach just allows us to update the keyframes of an
animation without affecting the animation players etc.

[Timothy Loh, 2015-01-21 05:04:15]

On 2015/01/21 03:25:49, shend wrote:
> On 2015/01/21 03:00:47, esprehn wrote:
> >
>
https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
> > File Source/core/css/CSSKeyframesRule.h (right):
> > 
> >
>
https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
> > Source/core/css/CSSKeyframesRule.h:65: unsigned styleChangeCounter() const {
> > return m_styleChangeCounter; }
> > This doesn't seem like the right way to solve this, why are keyframes
special
> > compared to all the other rule types?
> 
> When keyframe styles are changed, we want to avoid the overhead of re-creating
> all the animations. This approach just allows us to update the keyframes of an
> animation without affecting the animation players etc.

Changing style rules doesn't have this sort of logic as in each style recalc we
just read all the properties declarations from a StylePropertySet. The
animations engine generally avoids this and instead builds Interpolations which
can quickly be applied to RenderStyles. We want to avoid recreating
Interpolations as much as possible (these are intended to only be created once
for an animation) but need to do so when keyframe rules are changed. We decided
to only do this when keyframe rules have actually changed, instead of (for
example) on every non-animation style change.

Perhaps adding more context to the patch description would be a good idea here.

[esprehn, 2015-01-21 05:34:41]

On 2015/01/21 at 05:04:15, timloh wrote:
> On 2015/01/21 03:25:49, shend wrote:
> > On 2015/01/21 03:00:47, esprehn wrote:
> > >
> >
https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
> > > File Source/core/css/CSSKeyframesRule.h (right):
> > > 
> > >
> >
https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
> > > Source/core/css/CSSKeyframesRule.h:65: unsigned styleChangeCounter() const
{
> > > return m_styleChangeCounter; }
> > > This doesn't seem like the right way to solve this, why are keyframes
special
> > > compared to all the other rule types?
> > 
> > When keyframe styles are changed, we want to avoid the overhead of
re-creating
> > all the animations. This approach just allows us to update the keyframes of
an
> > animation without affecting the animation players etc.
> 
> Changing style rules doesn't have this sort of logic as in each style recalc
we just read all the properties declarations from a StylePropertySet. The
animations engine generally avoids this and instead builds Interpolations which
can quickly be applied to RenderStyles. We want to avoid recreating
Interpolations as much as possible (these are intended to only be created once
for an animation) but need to do so when keyframe rules are changed. We decided
to only do this when keyframe rules have actually changed, instead of (for
example) on every non-animation style change.
> 
> Perhaps adding more context to the patch description would be a good idea
here.

That seems like premature optimization, what benchmark modifies the CSSOM to
mutate keyframes? What real page needs this? CSSOM mutation is very rare on the
web. I don't think we should keep adding special cases for the animation engine.

[Timothy Loh, 2015-01-21 23:23:18]

On 2015/01/21 05:34:41, esprehn wrote:
> On 2015/01/21 at 05:04:15, timloh wrote:
> > On 2015/01/21 03:25:49, shend wrote:
> > > On 2015/01/21 03:00:47, esprehn wrote:
> > > >
> > >
>
https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
> > > > File Source/core/css/CSSKeyframesRule.h (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
> > > > Source/core/css/CSSKeyframesRule.h:65: unsigned styleChangeCounter()
const
> {
> > > > return m_styleChangeCounter; }
> > > > This doesn't seem like the right way to solve this, why are keyframes
> special
> > > > compared to all the other rule types?
> > > 
> > > When keyframe styles are changed, we want to avoid the overhead of
> re-creating
> > > all the animations. This approach just allows us to update the keyframes
of
> an
> > > animation without affecting the animation players etc.
> > 
> > Changing style rules doesn't have this sort of logic as in each style recalc
> we just read all the properties declarations from a StylePropertySet. The
> animations engine generally avoids this and instead builds Interpolations
which
> can quickly be applied to RenderStyles. We want to avoid recreating
> Interpolations as much as possible (these are intended to only be created once
> for an animation) but need to do so when keyframe rules are changed. We
decided
> to only do this when keyframe rules have actually changed, instead of (for
> example) on every non-animation style change.
> > 
> > Perhaps adding more context to the patch description would be a good idea
> here.
> 
> That seems like premature optimization, what benchmark modifies the CSSOM to
> mutate keyframes? What real page needs this? CSSOM mutation is very rare on
the
> web. I don't think we should keep adding special cases for the animation
engine.

Just to be clear, the counter isn't for optimising updating keyframes. The other
options we came up with were copying an @keyframes rule to compare against, or
simply recreating Interpolations on every non-animation style recalc, both which
have will slightly regress non-animation style recalcs for elements running CSS
animations. It probably isn't common to have lots of non-animation style recalcs
on an animation running a main-thread CSS animation, but we're trying to avoid
slowing down existing pages.

[dstockwell, 2015-01-23 05:32:21]

On 2015/01/21 at 23:23:18, timloh wrote:
> On 2015/01/21 05:34:41, esprehn wrote:
> > On 2015/01/21 at 05:04:15, timloh wrote:
> > > On 2015/01/21 03:25:49, shend wrote:
> > > > On 2015/01/21 03:00:47, esprehn wrote:
> > > > >
> > > >
> >
https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
> > > > > File Source/core/css/CSSKeyframesRule.h (right):
> > > > > 
> > > > >
> > > >
> >
https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
> > > > > Source/core/css/CSSKeyframesRule.h:65: unsigned styleChangeCounter()
const
> > {
> > > > > return m_styleChangeCounter; }
> > > > > This doesn't seem like the right way to solve this, why are keyframes
> > special
> > > > > compared to all the other rule types?
> > > > 
> > > > When keyframe styles are changed, we want to avoid the overhead of
> > re-creating
> > > > all the animations. This approach just allows us to update the keyframes
of
> > an
> > > > animation without affecting the animation players etc.
> > > 
> > > Changing style rules doesn't have this sort of logic as in each style
recalc
> > we just read all the properties declarations from a StylePropertySet. The
> > animations engine generally avoids this and instead builds Interpolations
which
> > can quickly be applied to RenderStyles. We want to avoid recreating
> > Interpolations as much as possible (these are intended to only be created
once
> > for an animation) but need to do so when keyframe rules are changed. We
decided
> > to only do this when keyframe rules have actually changed, instead of (for
> > example) on every non-animation style change.
> > > 
> > > Perhaps adding more context to the patch description would be a good idea
> > here.
> > 
> > That seems like premature optimization, what benchmark modifies the CSSOM to
> > mutate keyframes? What real page needs this? CSSOM mutation is very rare on
the
> > web. I don't think we should keep adding special cases for the animation
engine.
> 
> Just to be clear, the counter isn't for optimising updating keyframes. The
other options we came up with were copying an @keyframes rule to compare
against, or simply recreating Interpolations on every non-animation style
recalc, both which have will slightly regress non-animation style recalcs for
elements running CSS animations. It probably isn't common to have lots of
non-animation style recalcs on an animation running a main-thread CSS animation,
but we're trying to avoid slowing down existing pages.

Sounds like it's worthwhile gathering some data. We should avoid the counter and
rely solely on animation-style-change if possible.

[esprehn, 2015-01-27 21:52:17]

btw, What does the spec say to do? Not doing it like IE certainly seems simpler.
I've been pondering this recently and perhaps this counter is the simplest way
to go.

An alternative would be to keep the "compiled" animation data hanging off the
side of the StyleKeyframesRule. Whenever you modify it you'd mark it dirty() and
rebuild it, then the animation engine just has a RefPtr to it and does:

if (animation->keyframes() != animation->definition()->keframes()) {
  // keyframes were modified, need to do something here.
}

perhaps that's too much work though. You can land this for now if you want.

[shend, 2015-01-27 22:12:58]

On 2015/01/27 21:52:17, esprehn wrote:
> btw, What does the spec say to do? Not doing it like IE certainly seems
simpler.
> I've been pondering this recently and perhaps this counter is the simplest way
> to go.
> 
> An alternative would be to keep the "compiled" animation data hanging off the
> side of the StyleKeyframesRule. Whenever you modify it you'd mark it dirty()
and
> rebuild it, then the animation engine just has a RefPtr to it and does:
> 
> if (animation->keyframes() != animation->definition()->keframes()) {
>   // keyframes were modified, need to do something here.
> }
> 
> perhaps that's too much work though. You can land this for now if you want

According to the 2012-10-29 minutes in the spec, "@keyframes can be dynamically
changed". http://dev.w3.org/csswg/css-animations/#wg-resolutions-pending

The alternative method would work nicely if there's an efficient way to compare
two keyframe style definitions.
It seems to me like we'd have to compare (and possibly "compile") every keyframe
during every frame of the animation, which may be too costly.

[esprehn, 2015-01-27 22:16:12]

On 2015/01/27 at 22:12:58, shend wrote:
> On 2015/01/27 21:52:17, esprehn wrote:
> > btw, What does the spec say to do? Not doing it like IE certainly seems
simpler.
> > I've been pondering this recently and perhaps this counter is the simplest
way
> > to go.
> > 
> > An alternative would be to keep the "compiled" animation data hanging off
the
> > side of the StyleKeyframesRule. Whenever you modify it you'd mark it dirty()
and
> > rebuild it, then the animation engine just has a RefPtr to it and does:
> > 
> > if (animation->keyframes() != animation->definition()->keframes()) {
> >   // keyframes were modified, need to do something here.
> > }
> > 
> > perhaps that's too much work though. You can land this for now if you want
> 
> According to the 2012-10-29 minutes in the spec, "@keyframes can be
dynamically changed".
http://dev.w3.org/csswg/css-animations/#wg-resolutions-pending
> 
> The alternative method would work nicely if there's an efficient way to
compare two keyframe style definitions.
> It seems to me like we'd have to compare (and possibly "compile") every
keyframe during every frame of the animation, which may be too costly.

You just compare the ptrs to the compiled keyframes. The keyframes is an array
of StylePropertySets that contain CSSValue pointers parsed from the string
definitions in the @keyframes. When you change them we just reallocate the array
so the pointer changes. If you wanted to do deep comparison then do a memcmp()
just like we do for the matched properties cache. The StylePropertySet holds the
properties off the end of it so the memory region itself is the enough.

[esprehn, 2015-01-29 04:23:19]

lgtm w/ nits fixed. I might call it animationVersion() instead of
styleChangeCounter().

https://codereview.chromium.org/814083003/diff/280001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimationUpdate.h (right):

https://codereview.chromium.org/814083003/diff/280001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:85: unsigned styleChangeCounter;
zero init.

https://codereview.chromium.org/814083003/diff/280001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:103: unsigned styleChangeCounter;
zero init.

https://codereview.chromium.org/814083003/diff/280001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/814083003/diff/280001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.cpp:348: runningAnimation.styleRule =
entry.styleRule;
maybe a method on RunningAnimation like update(entry) ?

https://codereview.chromium.org/814083003/diff/280001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.cpp:363: RunningAnimation
runningAnimation;
RunningAnimation runningAnimation(player, entry) and move logic into the
constructor.

https://codereview.chromium.org/814083003/diff/280001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/814083003/diff/280001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.h:82: unsigned styleChangeCounter;
constructor needs to set this to 0

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
File Source/core/css/CSSKeyframeRule.cpp (right):

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
Source/core/css/CSSKeyframeRule.cpp:58: parent->m_keyframesRule->styleChanged();
CSSKeyframesRule want a method styleChanged(), don't reach into private vars.

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
File Source/core/css/CSSKeyframesRule.h (right):

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
Source/core/css/CSSKeyframesRule.h:74: unsigned m_styleChangeCounter;
: 31;
unsigned m_isPrefixed : 1;

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/resolve...
File Source/core/css/resolver/ScopedStyleResolver.cpp (right):

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/resolve...
Source/core/css/resolver/ScopedStyleResolver.cpp:109: rule->styleChanged();
I don't think you need this. This is just filling internal maps.

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.h:133:
PassRefPtrWillBeRawPtr<StyleRuleKeyframes> findKeyframesRule(const Element*,
const AtomicString& animationName);
This doesn't seem like it needs to be a ref ptr. The caller doesn't take
ownership.

[shend, 2015-01-30 00:03:10]

https://codereview.chromium.org/814083003/diff/280001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimationUpdate.h (right):

https://codereview.chromium.org/814083003/diff/280001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:85: unsigned styleChangeCounter;
On 2015/01/29 04:23:19, esprehn wrote:
> zero init.

Done.

https://codereview.chromium.org/814083003/diff/280001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:103: unsigned styleChangeCounter;
On 2015/01/29 04:23:19, esprehn wrote:
> zero init.

Done.

https://codereview.chromium.org/814083003/diff/280001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/814083003/diff/280001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.cpp:348: runningAnimation.styleRule =
entry.styleRule;
On 2015/01/29 04:23:19, esprehn wrote:
> maybe a method on RunningAnimation like update(entry) ?

Done.

https://codereview.chromium.org/814083003/diff/280001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.cpp:363: RunningAnimation
runningAnimation;
On 2015/01/29 04:23:19, esprehn wrote:
> RunningAnimation runningAnimation(player, entry) and move logic into the
> constructor.

Done.

https://codereview.chromium.org/814083003/diff/280001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/814083003/diff/280001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.h:82: unsigned styleChangeCounter;
On 2015/01/29 04:23:19, esprehn wrote:
> constructor needs to set this to 0

Done.

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
File Source/core/css/CSSKeyframeRule.cpp (right):

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
Source/core/css/CSSKeyframeRule.cpp:58: parent->m_keyframesRule->styleChanged();
On 2015/01/29 04:23:19, esprehn wrote:
> CSSKeyframesRule want a method styleChanged(), don't reach into private vars.

Done.

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
File Source/core/css/CSSKeyframesRule.h (right):

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/CSSKeyf...
Source/core/css/CSSKeyframesRule.h:74: unsigned m_styleChangeCounter;
On 2015/01/29 04:23:19, esprehn wrote:
> : 31;
> unsigned m_isPrefixed : 1;

Done.

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/resolve...
File Source/core/css/resolver/ScopedStyleResolver.cpp (right):

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/resolve...
Source/core/css/resolver/ScopedStyleResolver.cpp:109: rule->styleChanged();
On 2015/01/29 04:23:19, esprehn wrote:
> I don't think you need this. This is just filling internal maps.

Done.

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/814083003/diff/280001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.h:133:
PassRefPtrWillBeRawPtr<StyleRuleKeyframes> findKeyframesRule(const Element*,
const AtomicString& animationName);
On 2015/01/29 04:23:19, esprehn wrote:
> This doesn't seem like it needs to be a ref ptr. The caller doesn't take
> ownership.

Done.

[shend, 2015-01-30 02:35:15]

I changed a lot of code since the last review, could someone please re-review
the patch?

[Timothy Loh, 2015-01-30 02:43:44]

Didn't look at the rest of it, but...

https://codereview.chromium.org/814083003/diff/300001/LayoutTests/animations/...
File LayoutTests/animations/keyframes-cssom-updates-animation.html (right):

https://codereview.chromium.org/814083003/diff/300001/LayoutTests/animations/...
LayoutTests/animations/keyframes-cssom-updates-animation.html:36:
test(function() {
Split this up into separate tests

[esprehn, 2015-01-30 02:50:28]

https://codereview.chromium.org/814083003/diff/300001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimationUpdate.h (right):

https://codereview.chromium.org/814083003/diff/300001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:26: NewAnimation newAnimation;
Since you're adding a constructor you can just pass (animation, timing,
styleRule) here. animation->name() is the animationName, styleRuleVersion you
can fill from ->version etc.

https://codereview.chromium.org/814083003/diff/300001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:48: UpdatedAnimation
updatedAnimation;
ditto for the constructor.

https://codereview.chromium.org/814083003/diff/300001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/814083003/diff/300001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.cpp:369: m_animations.set(entry.name,
RunningAnimation(player, entry));
We should probably store pointers to RunningAnimation objects and do new
RunningAnimation here. It'll reduce the bloat in the map.

https://codereview.chromium.org/814083003/diff/300001/Source/core/css/Propert...
File Source/core/css/PropertySetCSSStyleDeclaration.h (right):

https://codereview.chromium.org/814083003/diff/300001/Source/core/css/Propert...
Source/core/css/PropertySetCSSStyleDeclaration.h:153: class
KeyframeStyleRuleCSSStyleDeclaration final : public
PropertySetCSSStyleDeclaration {
It'd be nice to put this and the impl in new .h and .cpp files. You should just
extend the StyleRuleCSSStyleDeclaration too, you can remove the final. That
saves you from having so much duplicate ref counting and such code.

[shend, 2015-02-01 23:49:14]

Fixed nits. Any other comments before I commit this?

https://codereview.chromium.org/814083003/diff/300001/LayoutTests/animations/...
File LayoutTests/animations/keyframes-cssom-updates-animation.html (right):

https://codereview.chromium.org/814083003/diff/300001/LayoutTests/animations/...
LayoutTests/animations/keyframes-cssom-updates-animation.html:36:
test(function() {
On 2015/01/30 02:43:44, Timothy Loh wrote:
> Split this up into separate tests

Done.

https://codereview.chromium.org/814083003/diff/300001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimationUpdate.h (right):

https://codereview.chromium.org/814083003/diff/300001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:26: NewAnimation newAnimation;
On 2015/01/30 02:50:28, esprehn wrote:
> Since you're adding a constructor you can just pass (animation, timing,
> styleRule) here. animation->name() is the animationName, styleRuleVersion you
> can fill from ->version etc.

Done.

https://codereview.chromium.org/814083003/diff/300001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:48: UpdatedAnimation
updatedAnimation;
On 2015/01/30 02:50:28, esprehn wrote:
> ditto for the constructor.

Done.

https://codereview.chromium.org/814083003/diff/300001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/814083003/diff/300001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.cpp:369: m_animations.set(entry.name,
RunningAnimation(player, entry));
On 2015/01/30 02:50:28, esprehn wrote:
> We should probably store pointers to RunningAnimation objects and do new
> RunningAnimation here. It'll reduce the bloat in the map.

Done.

https://codereview.chromium.org/814083003/diff/300001/Source/core/css/Propert...
File Source/core/css/PropertySetCSSStyleDeclaration.h (right):

https://codereview.chromium.org/814083003/diff/300001/Source/core/css/Propert...
Source/core/css/PropertySetCSSStyleDeclaration.h:153: class
KeyframeStyleRuleCSSStyleDeclaration final : public
PropertySetCSSStyleDeclaration {
On 2015/01/30 02:50:28, esprehn wrote:
> It'd be nice to put this and the impl in new .h and .cpp files. You should
just
> extend the StyleRuleCSSStyleDeclaration too, you can remove the final. That
> saves you from having so much duplicate ref counting and such code.

Done.

[esprehn, 2015-02-02 00:11:21]

lgtm w/ nits. Just fix them and land. :)

https://codereview.chromium.org/814083003/diff/340001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimationUpdate.h (right):

https://codereview.chromium.org/814083003/diff/340001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:23: struct NewAnimation {
class

https://codereview.chromium.org/814083003/diff/340001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:52: struct UpdatedAnimation {
class

https://codereview.chromium.org/814083003/diff/340001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/814083003/diff/340001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.h:72: class RunningAnimation : public
RefCountedWillBeGarbageCollected<RunningAnimation>  {
final

[shend, 2015-02-02 00:53:54]

https://codereview.chromium.org/814083003/diff/340001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimationUpdate.h (right):

https://codereview.chromium.org/814083003/diff/340001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:23: struct NewAnimation {
On 2015/02/02 00:11:20, esprehn wrote:
> class

Done.

https://codereview.chromium.org/814083003/diff/340001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:52: struct UpdatedAnimation {
On 2015/02/02 00:11:20, esprehn wrote:
> class

Done.

https://codereview.chromium.org/814083003/diff/340001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/814083003/diff/340001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.h:72: class RunningAnimation : public
RefCountedWillBeGarbageCollected<RunningAnimation>  {
On 2015/02/02 00:11:20, esprehn wrote:
> final

Done.

[shend, 2015-02-02 00:54:03]

The CQ bit was checked by shend@chromium.org

[commit-bot: I haz the power, 2015-02-02 00:54:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/814083003/360001

[commit-bot: I haz the power, 2015-02-02 02:15:53]

Committed patchset #19 (id:360001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189311