[webkit-reviews] review granted: [Bug 206746] [Mac wk2 Release] imported/w3c/web-platform-tests/web-animations/timing-model/animations/updating-the-finished-state.html flaky fail : [Attachment 393268] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at webkit.org>'s
request for review:
Bug 206746: [Mac wk2 Release]
imported/w3c/web-platform-tests/web-animations/timing-model/animations/updating
-the-finished-state.html flaky fail
https://bugs.webkit.org/show_bug.cgi?id=206746

Attachment 393268: Patch

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




--- Comment #13 from Dean Jackson <dino at apple.com> ---
Comment on attachment 393268
  --> https://bugs.webkit.org/attachment.cgi?id=393268
Patch

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

> Source/WebCore/ChangeLog:12
> +	   Because we could end up in situation where localTime was very
marginally smaller than endTime inside of WebAnimation::play(), we would end up
> +	   with an unresolved hold time and we would return before calling
WebAnimation::timingDidChange() and thus scheduling an animation update from
> +	   the timeline because we'd assume it was paused. As a result, the
animation would never end and the test would wait for a "finish" event which
> +	   would never come.

It's kind of weird to pick a line wrap value that is so large. Maybe you should
rewrap to 80-100 chars?

> Source/WebCore/animation/WebAnimation.cpp:931
> +    if (effectivePlaybackRate() > 0 && autoRewind == AutoRewind::Yes &&
(!localTime || localTime.value() < 0_s || (localTime.value() + timeEpsilon) >=
endTime)) {

Not now, but would it make sense to have helpers for nearly_equals or
nearly_greater than? I think we have some helpers for that already (maybe with
a better name).