[webkit-reviews] review denied: [Bug 180743] [Web Animations] Implement the "updating the finished state" procedure : [Attachment 329215] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Antoine Quint
<graouts at apple.com>'s request for review:
Bug 180743: [Web Animations] Implement the "updating the finished state"
procedure
https://bugs.webkit.org/show_bug.cgi?id=180743

Attachment 329215: Patch

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




--- Comment #3 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 329215
  --> https://bugs.webkit.org/attachment.cgi?id=329215
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.