Bug 182039 - [Web Animations] Compute the progress and currentIteration properties on getComputedTiming()
Summary: [Web Animations] Compute the progress and currentIteration properties on getC...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-23 23:43 PST by Antoine Quint
Modified: 2018-01-24 11:30 PST (History)
2 users (show)

See Also:


Attachments
Patch (71.04 KB, patch)
2018-01-24 00:59 PST, Antoine Quint
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2018-01-23 23:43:11 PST
We need to account for various timing properties (delay, iterations, iterationStart, direction, fill modes) when computing the "progress" and "currentIteration" properties returned by getComputedTiming().
Comment 1 Radar WebKit Bug Importer 2018-01-23 23:43:31 PST
<rdar://problem/36813568>
Comment 2 Antoine Quint 2018-01-24 00:59:53 PST
Created attachment 332132 [details]
Patch
Comment 3 Dean Jackson 2018-01-24 10:51:47 PST
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?
Comment 4 Antoine Quint 2018-01-24 11:27:27 PST
(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.
Comment 5 Antoine Quint 2018-01-24 11:30:00 PST
Committed r227534: <https://trac.webkit.org/changeset/227534>