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.
<rdar://problem/36017232>
Created attachment 329215 [details] Patch
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.
(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().
Created attachment 329239 [details] Patch
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 }.
Committed r225862: <https://trac.webkit.org/changeset/225862>