Bug 180743 – [Web Animations] Implement the "updating the finished state" procedure

• Component: Animations
• Status: RESOLVED FIXED

[Antoine Quint]

Now that we expose the playState, and prior to implement play()/pause()/finish()/cancel(), we need to implement the "updating the finished state" procedure as defined in https://drafts.csswg.org/web-animations-1/#updating-the-finished-state.

[Radar WebKit Bug Importer]

<rdar://problem/36017232>

[Antoine Quint]

Created attachment 329215 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 329215 [details]
Patch

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

> Source/WebCore/animation/DocumentTimeline.cpp:176
> +        animation->updateFinishedState(false, false);

false, false is unreadable.

> Source/WebCore/animation/WebAnimation.cpp:202
> +    // Otherwise, current time = (timeline time - start time) Ã playback rate

Maybe don't use non-ASCII in the comment.

> Source/WebCore/animation/WebAnimation.cpp:203
> +    return (m_timeline->currentTime().value() - m_startTime.value()) * m_playbackRate;

I'm not sure you need the value(); this computation should be doable in Seconds.

> Source/WebCore/animation/WebAnimation.h:86
> +    void updateFinishedState(bool, bool);

Please use enum params here.

> Source/WebCore/animation/WebAnimation.h:100
> +    std::optional<Seconds> currentTime(bool) const;

It's really unclear what the bool parameter means. Use an enum.

> LayoutTests/ChangeLog:9
> +        Rebase some WPT expectations with minor progressions due to exposing the "onfinish" property.

Do these WPT contain sufficient coverage of the logic you added? Seems like there would be many more behavior changes than this.

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 329215 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329215&action=review
> 
> > Source/WebCore/animation/WebAnimation.cpp:203
> > +    return (m_timeline->currentTime().value() - m_startTime.value()) * m_playbackRate;
> 
> I'm not sure you need the value(); this computation should be doable in
> Seconds.

These are std::optional<Seconds>, value() in this case returns Seconds. So we are working on Seconds, not doubles.

> > LayoutTests/ChangeLog:9
> > +        Rebase some WPT expectations with minor progressions due to exposing the "onfinish" property.
> 
> Do these WPT contain sufficient coverage of the logic you added? Seems like
> there would be many more behavior changes than this.

They do, BUT, the vast majority of tests use Element.animate() and so are almost all FAILs. This patch is one of a series to build up to implementing pause() and play(), where play() is a requirement before we can turn on Element.animate(). At this stage, there will be a lot more tests showing the correct behaviour of our implementation.

I will update the code to use enums for the play() variant and updateFinishedState().

[Antoine Quint]

Created attachment 329239 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 329239 [details]
Patch

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

> Source/WebCore/animation/WebAnimation.h:99
> +    enum class IgnoreHoldTime { Yes, No };

IgnoreHoldTime::No is a double negative. Might be better as RespectHoldTime { Yes, No }.

[Antoine Quint]

Committed r225862: <https://trac.webkit.org/changeset/225862>