Web Animations: Introduce String based KeyframeEffectModel

Web Animations: Introduce String based KeyframeEffectModel

Note: This is a rework of issue 212023004

This patch refactors KeyframeEffectModel to support different types of
keyframe values. The common keyframe logic is retained in
KeyframeEffectModelBase to be inherited by the keyframe type specialisations.

KeyframeEffectModel is then templated on Keyframe type, with a hierarchy of Keyframes
including StringKeyframe and AnimatableValueKeyframe being introduced.

String based keyframes are being introduced to properly support the JS API
for Web Animations element.animate().

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

[shans, 2014-03-31 10:50:28]

Reckon this is ready for a review now.

[alancutter (OOO until 2018), 2014-04-01 01:09:40]

Some quick comments, review ongoing.

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
File Source/core/animation/Keyframe.h (right):

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/Keyframe.h:57: RefPtrWillBeMember<AnimatableValue>
m_value;
Should this member variable be here?

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
File Source/core/animation/KeyframeEffectModel.cpp (right):

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:290: */
Commented code to be removed?

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
File Source/core/animation/KeyframeEffectModel.h (right):

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.h:56: // Represents the keyframes set
through the API.
It's not clear what this comment is attached to. KeyframeEffectModelBase, the
typedef or the inner PropertySpecificKeyframeGroup class.

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.h:123: template <class Keyframe>
This template variable name is the same as the Keyframe class which ends up
looking like template instantiation below. Perhaps KeyframeType?

[alancutter (OOO until 2018), 2014-04-01 03:12:48]

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/In...
File Source/core/animation/Interpolation.h (right):

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/In...
Source/core/animation/Interpolation.h:72: }
Adding currentValue() to StyleInterpolation is likely a remnant of
OpacityStyleInterpolation that I was supposed to remove in my patch.
My mistake, there probably shouldn't be any changes to Interpolation in this
patch.

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
File Source/core/animation/KeyframeEffectModel.cpp (right):

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:60:
PassOwnPtr<Vector<RefPtr<Interpolation> > > KeyframeEffectModelBase::sample(int
iteration, double fraction, double iterationDuration) const
We should keep the Oilpan types.

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:251: PassRefPtr<Interpolation>
AnimatableValueKeyframe::PropertySpecificKeyframe::createInterpolation(CSSPropertyID
property, Keyframe::PropertySpecificKeyframe *end) const
This function and others below are declared in AnimatableValueKeyframe.h and
StringKeyframe.h, it's a bit counter intuitive to find the definitions inside
KeyframeEffectModel.cpp. I expect others less familiar with the code would find
this arrangement confusing.

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:258: {
Perhaps this should have ASSERT_NOT_REACHED() here if AnimatableValues are only
going to be used for CSS animations and transitions.

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

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/cs...
Source/core/animation/css/CSSAnimations.cpp:503:
newFrames[0]->setPropertyValue(id, value.release());
If the changes to Interpolation are removed then these two lines can stay the
same.

[shans, 2014-04-01 04:01:26]

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/In...
File Source/core/animation/Interpolation.h (right):

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/In...
Source/core/animation/Interpolation.h:72: }
On 2014/04/01 03:12:48, alancutter wrote:
> Adding currentValue() to StyleInterpolation is likely a remnant of
> OpacityStyleInterpolation that I was supposed to remove in my patch.
> My mistake, there probably shouldn't be any changes to Interpolation in this
> patch.

Done.

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
File Source/core/animation/Keyframe.h (right):

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/Keyframe.h:57: RefPtrWillBeMember<AnimatableValue>
m_value;
On 2014/04/01 01:09:40, alancutter wrote:
> Should this member variable be here?

Nope...

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
File Source/core/animation/KeyframeEffectModel.cpp (right):

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:60:
PassOwnPtr<Vector<RefPtr<Interpolation> > > KeyframeEffectModelBase::sample(int
iteration, double fraction, double iterationDuration) const
On 2014/04/01 03:12:48, alancutter wrote:
> We should keep the Oilpan types.

Bad merge, I think.

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:251: PassRefPtr<Interpolation>
AnimatableValueKeyframe::PropertySpecificKeyframe::createInterpolation(CSSPropertyID
property, Keyframe::PropertySpecificKeyframe *end) const
On 2014/04/01 03:12:48, alancutter wrote:
> This function and others below are declared in AnimatableValueKeyframe.h and
> StringKeyframe.h, it's a bit counter intuitive to find the definitions inside
> KeyframeEffectModel.cpp. I expect others less familiar with the code would
find
> this arrangement confusing.

Ah yes, forgot to move these when I moved Keyframe out.

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:258: {
On 2014/04/01 03:12:48, alancutter wrote:
> Perhaps this should have ASSERT_NOT_REACHED() here if AnimatableValues are
only
> going to be used for CSS animations and transitions.

I think that would be a Very Bad Idea, because it would transiently push assert
failures into the API

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:290: */
On 2014/04/01 01:09:40, alancutter wrote:
> Commented code to be removed?

yeah

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
File Source/core/animation/KeyframeEffectModel.h (right):

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.h:56: // Represents the keyframes set
through the API.
On 2014/04/01 01:09:40, alancutter wrote:
> It's not clear what this comment is attached to. KeyframeEffectModelBase, the
> typedef or the inner PropertySpecificKeyframeGroup class.

Deleted.

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.h:123: template <class Keyframe>
On 2014/04/01 01:09:40, alancutter wrote:
> This template variable name is the same as the Keyframe class which ends up
> looking like template instantiation below. Perhaps KeyframeType?

I think this is fine. I don't want to make this Even More Wordy.

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

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/cs...
Source/core/animation/css/CSSAnimations.cpp:503:
newFrames[0]->setPropertyValue(id, value.release());
On 2014/04/01 03:12:48, alancutter wrote:
> If the changes to Interpolation are removed then these two lines can stay the
> same.

I've left the conversion to PassRefPtr, as I think it's more correct.

[shans, 2014-04-01 04:01:26]

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/In...
File Source/core/animation/Interpolation.h (right):

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/In...
Source/core/animation/Interpolation.h:72: }
On 2014/04/01 03:12:48, alancutter wrote:
> Adding currentValue() to StyleInterpolation is likely a remnant of
> OpacityStyleInterpolation that I was supposed to remove in my patch.
> My mistake, there probably shouldn't be any changes to Interpolation in this
> patch.

Done.

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
File Source/core/animation/Keyframe.h (right):

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/Keyframe.h:57: RefPtrWillBeMember<AnimatableValue>
m_value;
On 2014/04/01 01:09:40, alancutter wrote:
> Should this member variable be here?

Nope...

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
File Source/core/animation/KeyframeEffectModel.cpp (right):

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:60:
PassOwnPtr<Vector<RefPtr<Interpolation> > > KeyframeEffectModelBase::sample(int
iteration, double fraction, double iterationDuration) const
On 2014/04/01 03:12:48, alancutter wrote:
> We should keep the Oilpan types.

Bad merge, I think.

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:251: PassRefPtr<Interpolation>
AnimatableValueKeyframe::PropertySpecificKeyframe::createInterpolation(CSSPropertyID
property, Keyframe::PropertySpecificKeyframe *end) const
On 2014/04/01 03:12:48, alancutter wrote:
> This function and others below are declared in AnimatableValueKeyframe.h and
> StringKeyframe.h, it's a bit counter intuitive to find the definitions inside
> KeyframeEffectModel.cpp. I expect others less familiar with the code would
find
> this arrangement confusing.

Ah yes, forgot to move these when I moved Keyframe out.

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:258: {
On 2014/04/01 03:12:48, alancutter wrote:
> Perhaps this should have ASSERT_NOT_REACHED() here if AnimatableValues are
only
> going to be used for CSS animations and transitions.

I think that would be a Very Bad Idea, because it would transiently push assert
failures into the API

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:290: */
On 2014/04/01 01:09:40, alancutter wrote:
> Commented code to be removed?

yeah

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
File Source/core/animation/KeyframeEffectModel.h (right):

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.h:56: // Represents the keyframes set
through the API.
On 2014/04/01 01:09:40, alancutter wrote:
> It's not clear what this comment is attached to. KeyframeEffectModelBase, the
> typedef or the inner PropertySpecificKeyframeGroup class.

Deleted.

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.h:123: template <class Keyframe>
On 2014/04/01 01:09:40, alancutter wrote:
> This template variable name is the same as the Keyframe class which ends up
> looking like template instantiation below. Perhaps KeyframeType?

I think this is fine. I don't want to make this Even More Wordy.

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

https://codereview.chromium.org/215883005/diff/20001/Source/core/animation/cs...
Source/core/animation/css/CSSAnimations.cpp:503:
newFrames[0]->setPropertyValue(id, value.release());
On 2014/04/01 03:12:48, alancutter wrote:
> If the changes to Interpolation are removed then these two lines can stay the
> same.

I've left the conversion to PassRefPtr, as I think it's more correct.

[alancutter (OOO until 2018), 2014-04-01 06:15:58]

lgtm

[dstockwell, 2014-04-01 06:38:12]

lgtm

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

https://codereview.chromium.org/215883005/diff/40001/Source/core/animation/An...
Source/core/animation/AnimatableValueKeyframe.h:28: // This is not used in
time-critical code, so we probably don't need to
can these bodies not be in .cpp?

https://codereview.chromium.org/215883005/diff/40001/Source/core/animation/An...
Source/core/animation/AnimatableValueKeyframe.h:59:
Keyframe::PropertySpecificKeyframe *theClone = new
PropertySpecificKeyframe(offset, m_easing, m_value);
...frame* theClone

https://codereview.chromium.org/215883005/diff/40001/Source/core/animation/Ke...
File Source/core/animation/Keyframe.h (right):

https://codereview.chromium.org/215883005/diff/40001/Source/core/animation/Ke...
Source/core/animation/Keyframe.h:18: virtual ~Keyframe() { }
nit: whitespace and grouping would make this a bit easier to digest

https://codereview.chromium.org/215883005/diff/40001/Source/core/animation/Ke...
Source/core/animation/Keyframe.h:66: Keyframe()
Would it make things much harder if we made Keyframe/PropertySpecificKeyframe
immutable?

https://codereview.chromium.org/215883005/diff/40001/Source/core/animation/Ke...
File Source/core/animation/KeyframeEffectModel.h (right):

https://codereview.chromium.org/215883005/diff/40001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.h:152: inline const
AnimatableValueKeyframeEffectModel* toAnimatableValueKeyframeEffectModel(const
AnimationEffect *base)
* should suffix the type (AnimationEffect*), please look for other instances in
the patch

[shans, 2014-04-01 12:05:32]

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

https://codereview.chromium.org/215883005/diff/40001/Source/core/animation/An...
Source/core/animation/AnimatableValueKeyframe.h:28: // This is not used in
time-critical code, so we probably don't need to
On 2014/04/01 06:38:12, dstockwell wrote:
> can these bodies not be in .cpp?

Done.

https://codereview.chromium.org/215883005/diff/40001/Source/core/animation/An...
Source/core/animation/AnimatableValueKeyframe.h:59:
Keyframe::PropertySpecificKeyframe *theClone = new
PropertySpecificKeyframe(offset, m_easing, m_value);
On 2014/04/01 06:38:12, dstockwell wrote:
> ...frame* theClone

Done.

https://codereview.chromium.org/215883005/diff/40001/Source/core/animation/Ke...
File Source/core/animation/Keyframe.h (right):

https://codereview.chromium.org/215883005/diff/40001/Source/core/animation/Ke...
Source/core/animation/Keyframe.h:18: virtual ~Keyframe() { }
On 2014/04/01 06:38:12, dstockwell wrote:
> nit: whitespace and grouping would make this a bit easier to digest

Done.

https://codereview.chromium.org/215883005/diff/40001/Source/core/animation/Ke...
Source/core/animation/Keyframe.h:66: Keyframe()
On 2014/04/01 06:38:12, dstockwell wrote:
> Would it make things much harder if we made Keyframe/PropertySpecificKeyframe
> immutable?

PropertySpecificKeyframe is already. I *think* we could make Keyframe immutable.
FIXME?

https://codereview.chromium.org/215883005/diff/40001/Source/core/animation/Ke...
File Source/core/animation/KeyframeEffectModel.h (right):

https://codereview.chromium.org/215883005/diff/40001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.h:152: inline const
AnimatableValueKeyframeEffectModel* toAnimatableValueKeyframeEffectModel(const
AnimationEffect *base)
On 2014/04/01 06:38:12, dstockwell wrote:
> * should suffix the type (AnimationEffect*), please look for other instances
in
> the patch

Done.

[esprehn, 2014-04-01 22:53:33]

LGTM,  it does seem like a ton of inline functions. I'd suggest moving most of
them out of line until you think they really need to be.

https://codereview.chromium.org/215883005/diff/80001/Source/core/animation/An...
File Source/core/animation/AnimatableValueKeyframe.h (right):

https://codereview.chromium.org/215883005/diff/80001/Source/core/animation/An...
Source/core/animation/AnimatableValueKeyframe.h:56: AnimatableValueKeyframe()
Why not use the default generated constructor?

https://codereview.chromium.org/215883005/diff/80001/Source/core/animation/An...
Source/core/animation/AnimatableValueKeyframe.h:68: return adoptPtr(new
PropertySpecificKeyframe(offset(), easing(), propertyValue(property),
composite()));
This is so much inline, does it all really need to be?

[alancutter (OOO until 2018), 2014-04-02 01:49:18]

https://codereview.chromium.org/215883005/diff/80001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/215883005/diff/80001/Source/core/core.gypi#ne...
Source/core/core.gypi:610: 'animation/StringKeyframe.cp',
AnimatableValueKeyframe.cpp and StringKeyframe.cp don't exist and should be
removed.

[shans, 2014-04-02 11:54:45]

https://codereview.chromium.org/215883005/diff/80001/Source/core/animation/An...
File Source/core/animation/AnimatableValueKeyframe.h (right):

https://codereview.chromium.org/215883005/diff/80001/Source/core/animation/An...
Source/core/animation/AnimatableValueKeyframe.h:56: AnimatableValueKeyframe()
On 2014/04/01 22:53:33, esprehn wrote:
> Why not use the default generated constructor?

Done.

https://codereview.chromium.org/215883005/diff/80001/Source/core/animation/An...
Source/core/animation/AnimatableValueKeyframe.h:68: return adoptPtr(new
PropertySpecificKeyframe(offset(), easing(), propertyValue(property),
composite()));
On 2014/04/01 22:53:33, esprehn wrote:
> This is so much inline, does it all really need to be?

Moved these and PropertySpecificKeyframe constructors above into .cpp file. Did
the same for StringKeyframe.

https://codereview.chromium.org/215883005/diff/80001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/215883005/diff/80001/Source/core/core.gypi#ne...
Source/core/core.gypi:610: 'animation/StringKeyframe.cp',
On 2014/04/02 01:49:18, alancutter wrote:
> AnimatableValueKeyframe.cpp and StringKeyframe.cp don't exist and should be
> removed.

They exist.

[shans, 2014-04-02 11:54:59]

The CQ bit was checked by shans@chromium.org

[commit-bot: I haz the power, 2014-04-02 11:55:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shans@chromium.org/215883005/100001

[commit-bot: I haz the power, 2014-04-02 12:33:08]

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

[commit-bot: I haz the power, 2014-04-02 12:33:08]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[shans, 2014-04-02 12:40:41]

The CQ bit was checked by shans@chromium.org

[commit-bot: I haz the power, 2014-04-02 12:40:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shans@chromium.org/215883005/100001

[commit-bot: I haz the power, 2014-04-02 13:08:04]

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

[commit-bot: I haz the power, 2014-04-02 13:08:05]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[shans, 2014-04-02 13:34:10]

The CQ bit was checked by shans@chromium.org

[commit-bot: I haz the power, 2014-04-02 13:34:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shans@chromium.org/215883005/100001

[commit-bot: I haz the power, 2014-04-02 14:06:45]

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

[commit-bot: I haz the power, 2014-04-02 14:06:46]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[shans, 2014-04-02 18:35:41]

The CQ bit was checked by shans@chromium.org

[commit-bot: I haz the power, 2014-04-02 18:35:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shans@chromium.org/215883005/100001

[commit-bot: I haz the power, 2014-04-02 19:44:41]

Change committed as 170690