Closed
Bug 1266257
Opened 8 years ago
Closed 8 years ago
Revise GetComputedTimingAt to use simpler fraction-based approach
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
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
Assignee | ||
Comment 1•8 years ago
|
||
As per updates to Web Animations spec: https://github.com/w3c/web-animations/compare/10fc97366041%5E...b9a721e6536ae27c37df1e3948b013c6a52d5f4c Review commit: https://reviewboard.mozilla.org/r/47973/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47973/
Attachment #8743637 -
Flags: review?(hiikezoe)
Attachment #8743638 -
Flags: review?(hiikezoe)
Attachment #8743639 -
Flags: review?(hiikezoe)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47975/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47975/
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47977/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47977/
Comment 4•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8743638 -
Flags: review?(hiikezoe) → review+
Comment 5•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8743639 -
Flags: review?(hiikezoe) → review+
Comment 6•8 years ago
|
||
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8984af0dc14 https://hg.mozilla.org/integration/mozilla-inbound/rev/b38998a7ae5b https://hg.mozilla.org/integration/mozilla-inbound/rev/b575775f423b
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b8984af0dc14 https://hg.mozilla.org/mozilla-central/rev/b38998a7ae5b https://hg.mozilla.org/mozilla-central/rev/b575775f423b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•