Bug 179708 – [Web Animations] Complete support for keyframe animations

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

[Antoine Quint]

We're adding basic support for from-to animations in https://bugs.webkit.org/show_bug.cgi?id=179707, but we need to support the full complexity of multiple keyframe animations.

[Antoine Quint]

<rdar://problem/34953942>

[Antoine Quint]

Created attachment 330032 [details]
Patch

[Antoine Quint]

Created attachment 330043 [details]
Patch

[Dean Jackson]

Comment on attachment 330032 [details]
Patch

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

It stresses me out that so much code is landing without many tests.

> Source/WebCore/animation/KeyframeEffect.cpp:101
> +static inline ExceptionOr<void> processIterableKeyframes(ExecState& state, Strong<JSObject>&& keyframesInput, JSValue method, Vector<KeyframeEffect::ProcessedKeyframe>& processedKeyframes)

It might be nicer to have this function return an ExceptionOr<Vector<KeyframeEffect::ProcessedKeyframe>>, to avoid having an out parameter.

> Source/WebCore/animation/KeyframeEffect.cpp:132
> +            auto ownPropertyName = ownPropertyNames[j];
> +            auto ownPropertyNameAsString = ownPropertyName.string();
> +            auto ownPropertyRawValue = keyframe->get(&state, ownPropertyName);
> +            if (ownPropertyName == "easing")
> +                easing = convert<IDLDOMString>(state, ownPropertyRawValue);
> +            else if (ownPropertyNameAsString == "offset")
> +                offset = convert<IDLNullable<IDLDouble>>(state, ownPropertyRawValue);
> +            else if (ownPropertyName == "composite")

What is ownPropertyNameAsString for? You already compare ownPropertyName == "easing" and "composite". Why is "offset" different?

[Dean Jackson]

Comment on attachment 330043 [details]
Patch

copying review from previous patch

[Dean Jackson]

I'm sure Darin or someone will find lots of nits.

[Antoine Quint]

(In reply to Dean Jackson from comment #4)
> Comment on attachment 330032 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330032&action=review
> 
> It stresses me out that so much code is landing without many tests.

There are many tests, they're simply all using Element.animate(), which is landing next.

> > Source/WebCore/animation/KeyframeEffect.cpp:101
> > +static inline ExceptionOr<void> processIterableKeyframes(ExecState& state, Strong<JSObject>&& keyframesInput, JSValue method, Vector<KeyframeEffect::ProcessedKeyframe>& processedKeyframes)
> 
> It might be nicer to have this function return an
> ExceptionOr<Vector<KeyframeEffect::ProcessedKeyframe>>, to avoid having an
> out parameter.

Yes, I can do that. It was easier to follow the spec text by passing around the out parameter though.

> > Source/WebCore/animation/KeyframeEffect.cpp:132
> > +            auto ownPropertyName = ownPropertyNames[j];
> > +            auto ownPropertyNameAsString = ownPropertyName.string();
> > +            auto ownPropertyRawValue = keyframe->get(&state, ownPropertyName);
> > +            if (ownPropertyName == "easing")
> > +                easing = convert<IDLDOMString>(state, ownPropertyRawValue);
> > +            else if (ownPropertyNameAsString == "offset")
> > +                offset = convert<IDLNullable<IDLDouble>>(state, ownPropertyRawValue);
> > +            else if (ownPropertyName == "composite")
> 
> What is ownPropertyNameAsString for? You already compare ownPropertyName ==
> "easing" and "composite". Why is "offset" different?

I started out by only implementing offset, and then added easing and composite. But I see that JSC::Identifier, which is the type of ownPropertyName, supports string comparisons, so I'll remove ownPropertyName.

[Antoine Quint]

Committed r226234: <https://trac.webkit.org/changeset/226234>