Bug 182039 – [Web Animations] Compute the progress and currentIteration properties on getComputedTiming()

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

We need to account for various timing properties (delay, iterations, iterationStart, direction, fill modes) when computing the "progress" and "currentIteration" properties returned by getComputedTiming().

[Radar WebKit Bug Importer]

<rdar://problem/36813568>

[Antoine Quint]

Created attachment 332132 [details]
Patch

[Dean Jackson]

Comment on attachment 332132 [details]
Patch

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

> Source/WebCore/animation/AnimationEffect.cpp:36
> +const auto timeEpsilon = Seconds::fromMilliseconds(0.001); // 1 microsecond.

No need for the comment.

> Source/WebCore/animation/AnimationEffect.cpp:73
> +    if ((localTimeValue + timeEpsilon) < beforeActiveBoundaryTime || (animationIsBackwards && std::abs(localTimeValue.microseconds() - beforeActiveBoundaryTime.microseconds()) < timeEpsilon.microseconds()))

I wonder if you should have a helper for abs(a - b) < e

Also, do you need the epsilon in local + e < before? Why not just local < before?

> Source/WebCore/animation/AnimationEffect.cpp:156
> +    return std::abs(overallProgress);

Why abs? Could iterationStart be negative?

> Source/WebCore/animation/AnimationEffect.cpp:265
> +std::optional<double> AnimationEffect::iterationProgress() const
> +{
> +    return directedProgress();
> +}

What's the point of having these separate?

[Antoine Quint]

(In reply to Dean Jackson from comment #3)
> Comment on attachment 332132 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332132&action=review
> 
> > Source/WebCore/animation/AnimationEffect.cpp:36
> > +const auto timeEpsilon = Seconds::fromMilliseconds(0.001); // 1 microsecond.
> 
> No need for the comment.

Will remove.

> > Source/WebCore/animation/AnimationEffect.cpp:73
> > +    if ((localTimeValue + timeEpsilon) < beforeActiveBoundaryTime || (animationIsBackwards && std::abs(localTimeValue.microseconds() - beforeActiveBoundaryTime.microseconds()) < timeEpsilon.microseconds()))
> 
> I wonder if you should have a helper for abs(a - b) < e

It's not very pervasive for now.

> Also, do you need the epsilon in local + e < before? Why not just local <
> before?

Testing shows that I do, it adds the required tolerance to check that we are truly under.

> > Source/WebCore/animation/AnimationEffect.cpp:156
> > +    return std::abs(overallProgress);
> 
> Why abs? Could iterationStart be negative?

I didn't get to the root of why, but I saw some cases where we got -0, so I did this to ensure we would get 0, since -0 could trip WPT tests.

> > Source/WebCore/animation/AnimationEffect.cpp:265
> > +std::optional<double> AnimationEffect::iterationProgress() const
> > +{
> > +    return directedProgress();
> > +}
> 
> What's the point of having these separate?

So, iterationProgress() is the only public method, as used by getComputedTiming() and soon from other methods to calculate effect timing. All the other methods are procedures spelled out in the spec, and I wanted to implement those as they were called. Also, iterationProgress() will return something else once we account for timing functions.

[Antoine Quint]

Committed r227534: <https://trac.webkit.org/changeset/227534>