Closed Bug 1266257 Opened 8 years ago Closed 8 years ago

Revise GetComputedTimingAt to use simpler fraction-based approach

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: birtles, Assigned: birtles)

Details

Attachments

(3 files)

As a result of implementing iterationStart we discovered that it would be a lot simpler if Web Animations timing calculations were expressed primarily in terms of fractional values rather than time values.

The spec has now been updated to reflect this and we should bring our implementation into line.

The changes to the spec are roughly these: https://github.com/w3c/web-animations/compare/10fc97366041%5E...b9a721e6536ae27c37df1e3948b013c6a52d5f4c
Comment on attachment 8743637 [details]
MozReview Request: Bug 1266257 - Revise timing model calculations to use fraction-based approach

https://reviewboard.mozilla.org/r/47973/#review44737

::: dom/animation/KeyframeEffect.cpp:271
(Diff revision 1)
>    if (aLocalTime.IsNull()) {
>      return result;
>    }
>    const TimeDuration& localTime = aLocalTime.Value();
>  
> -  // When we finish exactly at the end of an iteration we need to report
> +  // Calculate the time within the active interval.

How about putting the correponding link as a comment here and there? Especially for me!
// http://w3c.github.io/web-animations/#active-time

::: dom/animation/KeyframeEffect.cpp:320
(Diff revision 1)
> +  // Convert the overall progress to a fraction of a single iteration--the
> +  // simply iteration progress.
> +  double progress = IsFinite(overallProgress)
> +                    ? fmod(overallProgress, 1.0)
> +                    : fmod(result.mIterationStart, 1.0);

Is there any strong reason this calculation for simple iteration progress is after current iteration?  Should be the order matched to the spec?
Also, the spec checks that overall progress is *infinity*, why not here?

::: dom/animation/KeyframeEffect.cpp:332
(Diff revision 1)
> -    } else if (IsInfinite(result.mIterations)) {
> -      progress = fmod(result.mIterationStart, 1.0);
> +    if (result.mCurrentIteration != UINT64_MAX) {
> +      result.mCurrentIteration--;

Ah, now I understand the reason of the reversed order.  Forget about it, please.
Attachment #8743637 - Flags: review?(hiikezoe) → review+
Attachment #8743638 - Flags: review?(hiikezoe) → review+
Comment on attachment 8743638 [details]
MozReview Request: Bug 1266257 - Move getComputedTiming-* tests to timing-model

https://reviewboard.mozilla.org/r/47975/#review44735

Please be aware MANIFEST.json containing changes which will conflict bug 1266251.
Attachment #8743639 - Flags: review?(hiikezoe) → review+
Comment on attachment 8743639 [details]
MozReview Request: Bug 1266257 - Split iteration progress tests into 'simple iteration progress' and 'active time' tests

https://reviewboard.mozilla.org/r/47977/#review44745
You need to log in before you can comment on or make changes to this bug.