WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
180743
[Web Animations] Implement the "updating the finished state" procedure
https://bugs.webkit.org/show_bug.cgi?id=180743
Summary
[Web Animations] Implement the "updating the finished state" procedure
Antoine Quint
Reported
2017-12-13 02:07:00 PST
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
.
Attachments
Patch
(17.82 KB, patch)
2017-12-13 02:23 PST
,
Antoine Quint
no flags
Details
Formatted Diff
Diff
Patch
(18.37 KB, patch)
2017-12-13 11:44 PST
,
Antoine Quint
simon.fraser
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2017-12-13 02:07:21 PST
<
rdar://problem/36017232
>
Antoine Quint
Comment 2
2017-12-13 02:23:33 PST
Created
attachment 329215
[details]
Patch
Simon Fraser (smfr)
Comment 3
2017-12-13 11:01:56 PST
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
Comment 4
2017-12-13 11:25:35 PST
(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
Comment 5
2017-12-13 11:44:43 PST
Created
attachment 329239
[details]
Patch
Simon Fraser (smfr)
Comment 6
2017-12-13 11:53:44 PST
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
Comment 7
2017-12-13 12:01:57 PST
Committed
r225862
: <
https://trac.webkit.org/changeset/225862
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug