Bug 179707 – [Web Animations] Implement basic to-from animations

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

As an initial step towards supporting keyframe animations, we should start by adding support for simple to-from animations, implementing basic parsing of keyframe arguments limited to the array form and with two values.

[Antoine Quint]

<rdar://problem/34932456>

[Antoine Quint]

Created attachment 326954 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 326954 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326954&action=review

> Source/WebCore/animation/KeyframeEffect.cpp:55
> +ExceptionOr<void> KeyframeEffect::setKeyframes(ExecState& state, Strong<JSObject>&& keyframes)

It seems really wrong for something down here in animation/ to be doing stuff with ExecState. Why isn't that limited to the bindings layer?

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 326954 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326954&action=review
> 
> > Source/WebCore/animation/KeyframeEffect.cpp:55
> > +ExceptionOr<void> KeyframeEffect::setKeyframes(ExecState& state, Strong<JSObject>&& keyframes)
> 
> It seems really wrong for something down here in animation/ to be doing
> stuff with ExecState. Why isn't that limited to the bindings layer?

Alas, we cannot currently express this in terms of IDL since the object can contain mostly arbitrary properties as dictionary keys. So we need to perform some ad-hoc parsing.

[Antoine Quint]

Created attachment 326955 [details]
Patch

[Antoine Quint]

Created attachment 326956 [details]
Patch

[Antoine Quint]

Created attachment 326958 [details]
Patch

[Sam Weinig]

I think doing this processing in KeyframeEffect.cpp is fine for now. 

I think it is really unfortunate that the spec, https://w3c.github.io/web-animations/#process-a-keyframes-argument, resorts to custom bindings here, and would much prefer that this was expressible in WebIDL, but, alas it is not.

[Sam Weinig]

Comment on attachment 326958 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326958&action=review

> Source/WebCore/animation/KeyframeEffect.cpp:55
> +ExceptionOr<void> KeyframeEffect::setKeyframes(ExecState& state, Strong<JSObject>&& keyframes)

I would extract this into a "processKeyframes" function and annotate it with the spec link, https://w3c.github.io/web-animations/#process-a-keyframes-argument (assuming that is the right link).

[Antoine Quint]

(In reply to Sam Weinig from comment #9)
> Comment on attachment 326958 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326958&action=review
> 
> > Source/WebCore/animation/KeyframeEffect.cpp:55
> > +ExceptionOr<void> KeyframeEffect::setKeyframes(ExecState& state, Strong<JSObject>&& keyframes)
> 
> I would extract this into a "processKeyframes" function and annotate it with
> the spec link,
> https://w3c.github.io/web-animations/#process-a-keyframes-argument (assuming
> that is the right link).

Can do, and it is the right link.

[Antoine Quint]

Created attachment 326988 [details]
Patch

[Antoine Quint]

Created attachment 326993 [details]
Patch

[Antti Koivisto]

Comment on attachment 326993 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326993&action=review

> Source/WebCore/animation/KeyframeEffect.cpp:114
> +        Vector<CSSPropertyID> properties;
> +        for (unsigned k = 0; k < numberOfCSSProperties; ++k) {
> +            properties.append(styleProperties->propertyAt(k).id());
> +            styleResolver.applyPropertyToStyle(styleProperties->propertyAt(k).id(), styleProperties->propertyAt(k).value(), WTFMove(renderStyle));
> +            renderStyle = styleResolver.state().takeStyle();
> +        }

We should make StyleResolver interface saner for this sort of thing at some point but this is fine for now.

[Dean Jackson]

Comment on attachment 326993 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326993&action=review

> Source/WebCore/animation/AnimationTimeline.h:73
> +    HashMap<RefPtr<Element>, Vector<RefPtr<WebAnimation>>> elementToAnimationsMap() const { return m_elementToAnimationsMap; }

Should be returning &.

> Source/WebCore/animation/KeyframeEffect.cpp:46
> +    auto result = adoptRef(*new KeyframeEffect(target));
> +
> +    auto setKeyframesResult = result->setKeyframes(state, WTFMove(keyframes));
> +    if (setKeyframesResult.hasException())
> +        return setKeyframesResult.releaseException();
> +
> +    return WTFMove(result);

I'd rename result to keyframeEffect, but whatevs.

> Source/WebCore/style/StyleTreeResolver.cpp:283
> +            std::unique_ptr<RenderStyle> animatedStyle = RenderStyle::clonePtr(*newStyle);

Could use auto here.

> LayoutTests/http/wpt/web-animations/timing-model/animations/set-the-animation-start-time-expected.txt:3
> -FAIL Setting the start time of an animation without an active timeline assert_equals: Start time remains null after setting current time expected (object) null but got (number) -999.896
> -FAIL Setting an unresolved start time an animation without an active timeline does not clear the current time assert_equals: Start time remains null after setting current time expected (object) null but got (number) -999.896
> +FAIL Setting the start time of an animation without an active timeline assert_equals: Start time remains null after setting current time expected (object) null but got (number) -999.7033
> +FAIL Setting an unresolved start time an animation without an active timeline does not clear the current time assert_equals: Start time remains null after setting current time expected (object) null but got (number) -999.7033

Why has this changed?

[Simon Fraser (smfr)]

Comment on attachment 326993 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326993&action=review

>> Source/WebCore/animation/AnimationTimeline.h:73
>> +    HashMap<RefPtr<Element>, Vector<RefPtr<WebAnimation>>> elementToAnimationsMap() const { return m_elementToAnimationsMap; }
> 
> Should be returning &.

A const &

> Source/WebCore/animation/KeyframeEffect.cpp:40
> +    auto result = adoptRef(*new KeyframeEffect(target));

We never use bare new. Should be KeyframeEffect::create().

>> Source/WebCore/animation/KeyframeEffect.cpp:46
>> +    return WTFMove(result);
> 
> I'd rename result to keyframeEffect, but whatevs.

+1

[Antoine Quint]

Committed r224897: <https://trac.webkit.org/changeset/224897>

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #15)
> Comment on attachment 326993 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326993&action=review
> 
> >> Source/WebCore/animation/AnimationTimeline.h:73
> >> +    HashMap<RefPtr<Element>, Vector<RefPtr<WebAnimation>>> elementToAnimationsMap() const { return m_elementToAnimationsMap; }
> > 
> > Should be returning &.
> 
> A const &

This was fixed in the commit.

> > Source/WebCore/animation/KeyframeEffect.cpp:40
> > +    auto result = adoptRef(*new KeyframeEffect(target));
> 
> We never use bare new. Should be KeyframeEffect::create().

This is the body of ::create().

> >> Source/WebCore/animation/KeyframeEffect.cpp:46
> >> +    return WTFMove(result);
> > 
> > I'd rename result to keyframeEffect, but whatevs.

This was not addressed, alas. I'll make a note to fix this in a future patch.

[Daniel Bates]

Comment on attachment 326993 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326993&action=review

>>>> Source/WebCore/animation/KeyframeEffect.cpp:46
>>>> +    return WTFMove(result);
>>> 
>>> I'd rename result to keyframeEffect, but whatevs.
>> 
>> +1
> 
> This was not addressed, alas. I'll make a note to fix this in a future patch.

Is this WTFMove() necessary?

> Source/WebCore/animation/KeyframeEffect.cpp:74
> +    Vector<Keyframe> newKeyframes { };

No need for the braces.

> Source/WebCore/animation/KeyframeEffect.cpp:82
> +    const auto* array = jsCast<const JSArray*>(keyframes.get());

No need to repeat the const on the left hand side.

> Source/WebCore/animation/KeyframeEffect.cpp:87
> +    for (unsigned i = 0; i < length; ++i) {

What is the data type of length. Is it unsigned? Otherwise, we should change to use the same data type as length.

> Source/WebCore/animation/KeyframeEffect.cpp:97
> +        for (unsigned j = 0; j < numberOfProperties; ++j) {

Although this code is unlikely to cause problems in practice, the data type of the index, j, differs from the data type of numberOfProperties. We should use the same data type for both, size_t.

> Source/WebCore/animation/KeyframeEffect.cpp:110
> +        for (unsigned k = 0; k < numberOfCSSProperties; ++k) {

We are adding every CSS property in this loop. So, we should reserve initial capacity in the Vector and use uncheckedAppend() to avoid reallocating the underlying storage of the Vector when appending items to it.

> Source/WebCore/animation/KeyframeEffect.cpp:129
> +    // FIXME: Assume animations only apply in the range [0, duration[

duration[?

> Source/WebCore/dom/Element.cpp:3709
> +    return Vector<RefPtr<WebAnimation>> { };

This should be “return { };”

[Antoine Quint]

(In reply to Daniel Bates from comment #18)
> Comment on attachment 326993 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326993&action=review
> 
> >>>> Source/WebCore/animation/KeyframeEffect.cpp:46
> >>>> +    return WTFMove(result);
> >>> 
> >>> I'd rename result to keyframeEffect, but whatevs.
> >> 
> >> +1
> > 
> > This was not addressed, alas. I'll make a note to fix this in a future patch.
> 
> Is this WTFMove() necessary?

Yes, since the return type is ExceptionOr<Ref<KeyframeEffect>>.

> > Source/WebCore/animation/KeyframeEffect.cpp:74
> > +    Vector<Keyframe> newKeyframes { };
> 
> No need for the braces.

Will fix in a cleanup patch.

> > Source/WebCore/animation/KeyframeEffect.cpp:82
> > +    const auto* array = jsCast<const JSArray*>(keyframes.get());
> 
> No need to repeat the const on the left hand side.

Will fix in a cleanup patch.

> > Source/WebCore/animation/KeyframeEffect.cpp:87
> > +    for (unsigned i = 0; i < length; ++i) {
> 
> What is the data type of length. Is it unsigned? Otherwise, we should change
> to use the same data type as length.

It is unsigned.

> > Source/WebCore/animation/KeyframeEffect.cpp:97
> > +        for (unsigned j = 0; j < numberOfProperties; ++j) {
> 
> Although this code is unlikely to cause problems in practice, the data type
> of the index, j, differs from the data type of numberOfProperties. We should
> use the same data type for both, size_t.

Will fix in a cleanup patch.

> > Source/WebCore/animation/KeyframeEffect.cpp:110
> > +        for (unsigned k = 0; k < numberOfCSSProperties; ++k) {
> 
> We are adding every CSS property in this loop. So, we should reserve initial
> capacity in the Vector and use uncheckedAppend() to avoid reallocating the
> underlying storage of the Vector when appending items to it.

Great! Will fix in a cleanup patch.

> > Source/WebCore/animation/KeyframeEffect.cpp:129
> > +    // FIXME: Assume animations only apply in the range [0, duration[
> 
> duration[?

This means the value of duration is excluded in the range, see https://en.wikipedia.org/wiki/Interval_(mathematics).

> > Source/WebCore/dom/Element.cpp:3709
> > +    return Vector<RefPtr<WebAnimation>> { };
> 
> This should be “return { };”

Will fix in a cleanup patch.

[Antoine Quint]

Clean-up is performed in https://bugs.webkit.org/show_bug.cgi?id=179777.