Web Animations API: Sort keyframes by offset

Web Animations API: Sort keyframes by offset

Due to a recent spec change, we must now sort the keyframes if they are
not loosely sorted by offset, and all keyframes have a supplied offset.

If they are not loosely sorted, and any do not have a supplied offset,
we must throw a DOMException of type InvalidModificationError.

https://dvcs.w3.org/hg/FXTF/diff/b5b53c344189/web-animations/index.html

BUG=363569

[Eric Willigers, 2014-04-24 06:54:39]

https://codereview.chromium.org/251463003/diff/1/Source/core/animation/Elemen...
File Source/core/animation/ElementAnimation.h (left):

https://codereview.chromium.org/251463003/diff/1/Source/core/animation/Elemen...
Source/core/animation/ElementAnimation.h:64: return animateInternal(element,
EffectInput::convert(&element, keyframeDictionaryVector, exceptionState),
TimingInput::convert(timingInputDictionary));
The changes in this file are from Doug's change 254423004. When that lands, this
file can be dropped from the CL.

[Eric Willigers, 2014-04-28 06:50:01]

+dstockwell

[alancutter (OOO until 2018), 2014-04-28 10:53:45]

lgtm

https://codereview.chromium.org/251463003/diff/20001/LayoutTests/web-animatio...
File LayoutTests/web-animations-api/element-animate-list-of-keyframes.html
(right):

https://codereview.chromium.org/251463003/diff/20001/LayoutTests/web-animatio...
LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:61:
assert_equals(e1Style.opacity, '0.875');
It's not very easy to tell that this is the correct answer at a glance,
different values would make this easier.
E.g.
{opacity: '0.5', offset: 0.5},
{opacity: '1', offset: 1},
{opacity: '0', offset: 0}

https://codereview.chromium.org/251463003/diff/20001/Source/core/animation/El...
File Source/core/animation/ElementAnimation.h (right):

https://codereview.chromium.org/251463003/diff/20001/Source/core/animation/El...
Source/core/animation/ElementAnimation.h:68: return animateInternal(element,
effect.release(), TimingInput::convert(timingInputDictionary));
This is already covered by Doug's change:
https://codereview.chromium.org/254423004/

[Eric Willigers, 2014-04-29 00:14:48]

https://codereview.chromium.org/251463003/diff/20001/LayoutTests/web-animatio...
File LayoutTests/web-animations-api/element-animate-list-of-keyframes.html
(right):

https://codereview.chromium.org/251463003/diff/20001/LayoutTests/web-animatio...
LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:61:
assert_equals(e1Style.opacity, '0.875');
On 2014/04/28 10:53:46, alancutter wrote:
> It's not very easy to tell that this is the correct answer at a glance,
> different values would make this easier.
> E.g.
> {opacity: '0.5', offset: 0.5},
> {opacity: '1', offset: 1},
> {opacity: '0', offset: 0}

Done.

https://codereview.chromium.org/251463003/diff/20001/Source/core/animation/El...
File Source/core/animation/ElementAnimation.h (right):

https://codereview.chromium.org/251463003/diff/20001/Source/core/animation/El...
Source/core/animation/ElementAnimation.h:68: return animateInternal(element,
effect.release(), TimingInput::convert(timingInputDictionary));
On 2014/04/28 10:53:46, alancutter wrote:
> This is already covered by Doug's change:
> https://codereview.chromium.org/254423004/

Ack. My first comment in this review:
The changes in this file are from Doug's change 254423004. When that lands, this
file can be dropped from the CL.

[dstockwell, 2014-04-29 03:58:42]

https://codereview.chromium.org/251463003/diff/40001/LayoutTests/web-animatio...
File LayoutTests/web-animations-api/element-animate-list-of-keyframes.html
(right):

https://codereview.chromium.org/251463003/diff/40001/LayoutTests/web-animatio...
LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:66:
{opacity: '0.5'},
Do we have a test where offset is explicitly set to null?

Setting null should still fail here, not coerce the offset to 0.

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

https://codereview.chromium.org/251463003/diff/40001/Source/core/animation/Ef...
Source/core/animation/EffectInput.cpp:72: if
(keyframeDictionaryVector[i].get("offset", offset)) {
I'm not sure this works correctly when the offset is set to null.

[Eric Willigers, 2014-04-29 07:34:44]

https://codereview.chromium.org/251463003/diff/40001/LayoutTests/web-animatio...
File LayoutTests/web-animations-api/element-animate-list-of-keyframes.html
(right):

https://codereview.chromium.org/251463003/diff/40001/LayoutTests/web-animatio...
LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:66:
{opacity: '0.5'},
On 2014/04/29 03:58:43, dstockwell wrote:
> Do we have a test where offset is explicitly set to null?
> 
> Setting null should still fail here, not coerce the offset to 0.

Done.

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

https://codereview.chromium.org/251463003/diff/40001/Source/core/animation/Ef...
Source/core/animation/EffectInput.cpp:72: if
(keyframeDictionaryVector[i].get("offset", offset)) {
On 2014/04/29 03:58:43, dstockwell wrote:
> I'm not sure this works correctly when the offset is set to null.

Done.

[alancutter (OOO until 2018), 2014-04-29 07:58:43]

lgtm new changes with nit.

https://codereview.chromium.org/251463003/diff/60001/LayoutTests/web-animatio...
File LayoutTests/web-animations-api/element-animate-list-of-keyframes.html
(right):

https://codereview.chromium.org/251463003/diff/60001/LayoutTests/web-animatio...
LayoutTests/web-animations-api/element-animate-list-of-keyframes.html:83:
player.currentTime = durationValue / 2;
This should probably check that the keyframes get sorted and test durationValue
/ 4 instead of durationValue / 2.

[dstockwell, 2014-04-29 08:11:47]

+haraken, please take a look at inline comment

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

https://codereview.chromium.org/251463003/diff/60001/Source/core/animation/Ef...
Source/core/animation/EffectInput.cpp:71: v8::Local<v8::Value> v8Value;
v8::Local is not usually used in core/ code, +haraken, is there a better way to
do this?

Basically we want to ignore if the value === null, otherwise coerce to double.

[haraken, 2014-04-29 08:16:19]

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

https://codereview.chromium.org/251463003/diff/60001/Source/core/animation/Ef...
Source/core/animation/EffectInput.cpp:71: v8::Local<v8::Value> v8Value;
On 2014/04/29 08:11:47, dstockwell wrote:
> v8::Local is not usually used in core/ code, +haraken, is there a better way
to
> do this?
> 
> Basically we want to ignore if the value === null, otherwise coerce to double.

Can we use ScriptValue?

[dstockwell, 2014-04-29 08:40:12]

On 2014/04/29 08:16:19, haraken wrote:
>
https://codereview.chromium.org/251463003/diff/60001/Source/core/animation/Ef...
> File Source/core/animation/EffectInput.cpp (right):
> 
>
https://codereview.chromium.org/251463003/diff/60001/Source/core/animation/Ef...
> Source/core/animation/EffectInput.cpp:71: v8::Local<v8::Value> v8Value;
> On 2014/04/29 08:11:47, dstockwell wrote:
> > v8::Local is not usually used in core/ code, +haraken, is there a better way
> to
> > do this?
> > 
> > Basically we want to ignore if the value === null, otherwise coerce to
double.
> 
> Can we use ScriptValue?

Looks like that could work:
get as double, if 0, get again as script value then check isNull.

Although I see "Don't use this in performance-sensitive places". This is
probably OK, as in the common case the value is not present in the dictionary at
all.

[alancutter (OOO until 2018), 2014-04-29 09:45:47]

On 2014/04/29 08:40:12, dstockwell wrote:
> On 2014/04/29 08:16:19, haraken wrote:
> >
>
https://codereview.chromium.org/251463003/diff/60001/Source/core/animation/Ef...
> > File Source/core/animation/EffectInput.cpp (right):
> > 
> >
>
https://codereview.chromium.org/251463003/diff/60001/Source/core/animation/Ef...
> > Source/core/animation/EffectInput.cpp:71: v8::Local<v8::Value> v8Value;
> > On 2014/04/29 08:11:47, dstockwell wrote:
> > > v8::Local is not usually used in core/ code, +haraken, is there a better
way
> > to
> > > do this?
> > > 
> > > Basically we want to ignore if the value === null, otherwise coerce to
> double.
> > 
> > Can we use ScriptValue?
> 
> Looks like that could work:
> get as double, if 0, get again as script value then check isNull.
> 
> Although I see "Don't use this in performance-sensitive places". This is
> probably OK, as in the common case the value is not present in the dictionary
at
> all.

Created a fork of this patch while Eric is away to fix these issues:
https://codereview.chromium.org/253093002/