Web Animations API: Fix Synthetic keyframes + partial keyframes

Web Animations API: Fix KeyframeEffectModel::PropertySpecificKeyframeGroup::addSyntheticKeyframeIfRequired

Synthetic Keyframes:
For each property declared in a KeyframeEffectModel, addSyntheticKeyframeIfRequired adds
a property specific keyframe at offset: 0 if none exists. Similarly, it adds a keyframe
at offset: 1 if required.

The synthetic keyframes have compositeOperation add, and a neutral value for the property.

Partial Keyframes:
Previously, if an animation was constructed with a partial keyframe (a keyframe list with no 
keyframe at offset zero or no keyframe at offset one for any of the properties it declares) 
the renderer would crash. 

This change detects partial keyframes and throws a NotSupportedError in the JavaScript.

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

[rjwright, 2014-03-19 05:46:49]

ptal

[rjwright, 2014-03-19 05:48:39]

On 2014/03/19 05:46:49, rjwright wrote:
> ptal

Take a look if you want, but I forgot to check for breaking tests. Will upload
fixes shortly.

[rjwright, 2014-03-19 06:27:49]

Shane: I've removed the assert in the PropertySpecificKeyframe constructor.

[shans, 2014-03-19 09:06:20]

The problem with removing that assert is that it's possible to make
KeyframeEffectModel objects that render incorrectly (but aren't detected as
being wrong). So for example, if I were to use the C++ API to build a
KeyframeEffectModel with a single replace keyframe, then
addSyntheticKeyframeIfRequired would insert an add keyframe, but the information
that it was an add keyframe is lost as PropertySpecificKeyframes don't currently
carry composite operators.

I've suggested a fix in the 3 line-specific comments - essentially, track the
composite operator in PropertySpecificKeyframes, then assert in
ensureInterpolationEffect that it is always CompositeReplace. This should allow
KeyframeEffectModels that require synthetic keyframes to be constructed, but it
will disallow them from ever being animated, which is what we want.

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

https://codereview.chromium.org/203463009/diff/40001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:230: const AnimatableValue* end =
keyframes[0]->value();
Assert that end is CompositeReplace

https://codereview.chromium.org/203463009/diff/40001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:233: end = keyframes[i +
1]->value();
Assert that end is CompositeReplace

https://codereview.chromium.org/203463009/diff/40001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:251: m_value =
AnimatableValue::takeConstRef(value);
Store composite in an m_composite member variable of the
PropertySpecificKeyframe

[shans, 2014-03-19 09:40:56]

On 2014/03/19 09:06:20, shans wrote:
> The problem with removing that assert is that it's possible to make
> KeyframeEffectModel objects that render incorrectly (but aren't detected as
> being wrong). So for example, if I were to use the C++ API to build a
> KeyframeEffectModel with a single replace keyframe, then
> addSyntheticKeyframeIfRequired would insert an add keyframe, but the
information
> that it was an add keyframe is lost as PropertySpecificKeyframes don't
currently
> carry composite operators.
> 
> I've suggested a fix in the 3 line-specific comments - essentially, track the
> composite operator in PropertySpecificKeyframes, then assert in
> ensureInterpolationEffect that it is always CompositeReplace. This should
allow
> KeyframeEffectModels that require synthetic keyframes to be constructed, but
it
> will disallow them from ever being animated, which is what we want.
> 
>
https://codereview.chromium.org/203463009/diff/40001/Source/core/animation/Ke...
> File Source/core/animation/KeyframeEffectModel.cpp (right):
> 
>
https://codereview.chromium.org/203463009/diff/40001/Source/core/animation/Ke...
> Source/core/animation/KeyframeEffectModel.cpp:230: const AnimatableValue* end
=
> keyframes[0]->value();
> Assert that end is CompositeReplace
> 
>
https://codereview.chromium.org/203463009/diff/40001/Source/core/animation/Ke...
> Source/core/animation/KeyframeEffectModel.cpp:233: end = keyframes[i +
> 1]->value();
> Assert that end is CompositeReplace
> 
>
https://codereview.chromium.org/203463009/diff/40001/Source/core/animation/Ke...
> Source/core/animation/KeyframeEffectModel.cpp:251: m_value =
> AnimatableValue::takeConstRef(value);
> Store composite in an m_composite member variable of the
> PropertySpecificKeyframe

Having looked at your other patch, you could simply pull isReplaceOnly in here
and ASSERT that it is true inside ensureInterpolationEffect. You'd also need to
pull in the m_composite member on PropertySpecificKeyframes.

[rjwright, 2014-03-19 09:53:03]

On 2014/03/19 09:40:56, shans wrote:
> On 2014/03/19 09:06:20, shans wrote:
> > The problem with removing that assert is that it's possible to make
> > KeyframeEffectModel objects that render incorrectly (but aren't detected as
> > being wrong). So for example, if I were to use the C++ API to build a
> > KeyframeEffectModel with a single replace keyframe, then
> > addSyntheticKeyframeIfRequired would insert an add keyframe, but the
> information
> > that it was an add keyframe is lost as PropertySpecificKeyframes don't
> currently
> > carry composite operators.
> > 
> > I've suggested a fix in the 3 line-specific comments - essentially, track
the
> > composite operator in PropertySpecificKeyframes, then assert in
> > ensureInterpolationEffect that it is always CompositeReplace. This should
> allow
> > KeyframeEffectModels that require synthetic keyframes to be constructed, but
> it
> > will disallow them from ever being animated, which is what we want.
> > 
> >
>
https://codereview.chromium.org/203463009/diff/40001/Source/core/animation/Ke...
> > File Source/core/animation/KeyframeEffectModel.cpp (right):
> > 
> >
>
https://codereview.chromium.org/203463009/diff/40001/Source/core/animation/Ke...
> > Source/core/animation/KeyframeEffectModel.cpp:230: const AnimatableValue*
end
> =
> > keyframes[0]->value();
> > Assert that end is CompositeReplace
> > 
> >
>
https://codereview.chromium.org/203463009/diff/40001/Source/core/animation/Ke...
> > Source/core/animation/KeyframeEffectModel.cpp:233: end = keyframes[i +
> > 1]->value();
> > Assert that end is CompositeReplace
> > 
> >
>
https://codereview.chromium.org/203463009/diff/40001/Source/core/animation/Ke...
> > Source/core/animation/KeyframeEffectModel.cpp:251: m_value =
> > AnimatableValue::takeConstRef(value);
> > Store composite in an m_composite member variable of the
> > PropertySpecificKeyframe
> 
> Having looked at your other patch, you could simply pull isReplaceOnly in here
> and ASSERT that it is true inside ensureInterpolationEffect. You'd also need
to
> pull in the m_composite member on PropertySpecificKeyframes.

Re-uploaded local branch for https://codereview.chromium.org/196053009/ to this
issue number. Closing 196053009.

[rjwright, 2014-03-19 10:22:28]

ASSERT that it is true inside ensureInterpolationEffect

Done.

[shans, 2014-03-19 20:43:26]

lgtm

@dstockwell: I suggested Renee merge these two patches as most of the
functionality needed to assert we didn't introduce synthetic 'add' keyframes
when using the C++ API was present in the partial keyframes patch.

https://codereview.chromium.org/203463009/diff/50003/LayoutTests/web-animatio...
File LayoutTests/web-animations-api/partial-keyframes.html (right):

https://codereview.chromium.org/203463009/diff/50003/LayoutTests/web-animatio...
LayoutTests/web-animations-api/partial-keyframes.html:80: }, 'Calling new
Animation() with no offset specified for the last keyframe should create an
animation.');
unless that is the only keyframe for some property...

https://codereview.chromium.org/203463009/diff/50003/Source/core/animation/Ef...
File Source/core/animation/EffectInput.cpp (right):

https://codereview.chromium.org/203463009/diff/50003/Source/core/animation/Ef...
Source/core/animation/EffectInput.cpp:111:
exceptionState.throwDOMException(NotSupportedError, "Partial keyframes are not
supported.");
do you need to return null in this case too, or is that handled by the IDL?

[dstockwell, 2014-03-20 01:05:10]

lgtm

https://codereview.chromium.org/203463009/diff/50003/Source/core/animation/Ke...
File Source/core/animation/KeyframeEffectModelTest.cpp (right):

https://codereview.chromium.org/203463009/diff/50003/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModelTest.cpp:192:
TEST(AnimationKeyframeEffectModel, DISABLED_SingleKeyframeAtOffsetZero)
// FIXME

[rjwright, 2014-03-20 05:31:58]

The CQ bit was checked by rjwright@chromium.org

[commit-bot: I haz the power, 2014-03-20 05:31:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjwright@chromium.org/203463009/90001

[commit-bot: I haz the power, 2014-03-20 06:35:40]

Change committed as 169626