Bug 178674 – [Web Animations] Add basic timing and target properties

• Component: Animations
• Status: RESOLVED FIXED

[Antoine Quint]

[Web Animations] Add basic timing and target properties

[Antoine Quint]

Created attachment 324567 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 324567 [details]
Patch

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

> Source/WebCore/animation/AnimationEffectTiming.h:45
> +    double m_duration;

Can we use Seconds here?

> Source/WebCore/animation/WebAnimation.h:55
> +    std::optional<double> m_startTime;

MonotonicTime

[Dean Jackson]

Comment on attachment 324567 [details]
Patch

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

> Source/WebCore/animation/AnimationEffect.cpp:41
> +AnimationEffect::~AnimationEffect()
> +{
> +}

Will this eventually have code? If not, put it in the header.

> Source/WebCore/animation/WebAnimation.h:55
> +    std::optional<double> m_startTime;

As smfr probably said, use Seconds or something.

> Source/WebCore/animation/WebAnimation.idl:35
>  ] interface WebAnimation {
> +             attribute AnimationEffect? effect;
>      readonly attribute AnimationTimeline? timeline;
> +             attribute double? startTime;
>  };

Is this weird spacing standard?

> Source/WebCore/bindings/js/JSAnimationEffectCustom.cpp:42
> +JSValue toJSNewlyCreated(ExecState*, JSDOMGlobalObject* globalObject, Ref<AnimationEffect>&& value)
> +{
> +    if (value->isKeyframeEffect())
> +        return createWrapper<KeyframeEffect>(globalObject, WTFMove(value));
> +    return createWrapper<AnimationEffect>(globalObject, WTFMove(value));
> +}

Wish this code was automatic. Paging Rabbi Weinig.

> LayoutTests/ChangeLog:23
> +        * webanimations/animation-effect-expected.txt: Added.
> +        * webanimations/animation-effect-timing-expected.txt: Added.
> +        * webanimations/animation-effect-timing.html: Added.
> +        * webanimations/animation-effect.html: Added.
> +        * webanimations/animation-interface-effect-property-expected.txt: Added.
> +        * webanimations/animation-interface-effect-property.html: Added.
> +        * webanimations/animation-interface-start-time-property-expected.txt: Added.
> +        * webanimations/animation-interface-start-time-property.html: Added.
> +        * webanimations/keyframe-effect-expected.txt: Added.
> +        * webanimations/keyframe-effect-interface-timing-duration-expected.txt: Added.
> +        * webanimations/keyframe-effect-interface-timing-duration.html: Added.
> +        * webanimations/keyframe-effect.html: Added.
> +

All these tests should be written for Web Platform Tests.

Put them in http/wpt/ for now, with the goal of submitting them to WPT. Use the WPT API for testing.

[Dean Jackson]

Comment on attachment 324567 [details]
Patch

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

> Source/WebCore/animation/WebAnimation.h:49
> +    void setStartTime(std::optional<double> startTime) { m_startTime = startTime; }

I'm sure you're changing this, but no need for the optional in the parameter. If you're setting the value, it clearly exists.

[Antoine Quint]

Created attachment 324588 [details]
Patch

[Dean Jackson]

Comment on attachment 324588 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        Tests: webanimations/animation-effect-timing.html
> +               webanimations/animation-effect.html
> +               webanimations/animation-interface-effect-property.html
> +               webanimations/animation-interface-start-time-property.html
> +               webanimations/keyframe-effect-interface-timing-duration.html
> +               webanimations/keyframe-effect.html

Please file a bug to move these to WPT.

> Source/WebCore/animation/AnimationEffect.h:44
> +    enum ClassType {

Use enum class.

Then you'll probably need AnimationEffect::Foo elsewhere.

> Source/WebCore/animation/WebAnimation.h:50
> +    std::optional<double> bindingsStartTime() const;
> +    void setBindingsStartTime(std::optional<double>);
> +    std::optional<MonotonicTime> startTime() const { return m_startTime; }
> +    void setStartTime(MonotonicTime& startTime) { m_startTime = startTime; }

You're fixing my bad advice here.

[Antoine Quint]

Created attachment 324590 [details]
Patch

[Antoine Quint]

Created attachment 324591 [details]
Patch

[Antoine Quint]

Created attachment 324595 [details]
Patch

[Antoine Quint]

Created attachment 324596 [details]
Patch

[Antoine Quint]

Created attachment 324644 [details]
Patch

[Antoine Quint]

Created attachment 324645 [details]
Patch

[Antoine Quint]

Created attachment 324647 [details]
Patch

[Antoine Quint]

Created attachment 324648 [details]
Patch

[Antoine Quint]

Created attachment 324656 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 324656 [details]
Patch for landing

Clearing flags on attachment: 324656

Committed r223883: <https://trac.webkit.org/changeset/223883>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35271077>