We need to account for various timing properties (delay, iterations, iterationStart, direction, fill modes) when computing the "progress" and "currentIteration" properties returned by getComputedTiming().
<rdar://problem/36813568>
Created attachment 332132 [details] Patch
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?
(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.
Committed r227534: <https://trac.webkit.org/changeset/227534>