Closed
Bug 1249212
Opened 9 years ago
Closed 9 years ago
Make ComputedTiming.endTime handling match updated spec
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: birtles, Assigned: birtles)
References
()
Details
Attachments
(7 files)
15.64 KB,
patch
|
boris
:
review+
|
Details | Diff | Splinter Review |
2.08 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
2.15 KB,
patch
|
boris
:
review+
|
Details | Diff | Splinter Review |
3.99 KB,
patch
|
boris
:
review+
|
Details | Diff | Splinter Review |
2.81 KB,
patch
|
boris
:
review+
|
Details | Diff | Splinter Review |
3.00 KB,
patch
|
boris
:
review+
|
Details | Diff | Splinter Review |
4.34 KB,
patch
|
boris
:
review+
|
Details | Diff | Splinter Review |
I spoke with Google about how end-time should be defined and we agreed it should be possible to have an end time before the start time. When I went to implement this and check it (before speccing), however, I found a number of other issues in our code we need to fix. This bug covers all those other issues too.
Assignee | ||
Comment 1•9 years ago
|
||
This patch just simplifies the keyframe-effect tests so that we don't have to
repeat default values. This makes the tests shorter, easier to scan, and
easier to understand what is being tested.
In some cases we still repeat the default values in order to indicate that
we're testing that we get a particular default value.
Attachment #8720674 -
Flags: review?(boris.chiou)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
This patch brings the behavior of this method into line with the comment that
describes it and other methods in this class that have a similar test.
I discovered this bug while exercising this code from animation code and was
getting test failures due to returning -Infinity instead of the expected
Infinity.
Attachment #8720675 -
Flags: review?(nfroyd)
Assignee | ||
Comment 3•9 years ago
|
||
With the added tests in part 4 we crash without this change because we end up
trying to multiply an infinite iteration duration by a zero iteration count
which trips an assertion in StickyTimeDuration. Hence we fix this behavior
before adding the tests.
Attachment #8720676 -
Flags: review?(boris.chiou)
Assignee | ||
Comment 4•9 years ago
|
||
Before we go fixing endTime, we should add tests that activeDuration (which
endTime builds on) is being calculated correctly. (Spoiler: it wasn't, hence
parts 2 and 3 in this patch series.)
Attachment #8720677 -
Flags: review?(boris.chiou)
Assignee | ||
Comment 5•9 years ago
|
||
Based on discussion at: https://github.com/w3c/web-animations/issues/86
Attachment #8720678 -
Flags: review?(boris.chiou)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8720679 -
Flags: review?(boris.chiou)
Assignee | ||
Comment 7•9 years ago
|
||
Current endTime is calculated when getComputedTiming() is called. As a result,
the value returned there doesn't necessarily reflect what we are using in
the model. It would be more simple, consistent and useful if we simply
calculate this as part of GetComputedTimingAt and use it both internally
and in the result to getComputedTiming().
Attachment #8720680 -
Flags: review?(boris.chiou)
Updated•9 years ago
|
Attachment #8720674 -
Flags: review?(boris.chiou) → review+
Updated•9 years ago
|
Attachment #8720676 -
Flags: review?(boris.chiou) → review+
Updated•9 years ago
|
Attachment #8720677 -
Flags: review?(boris.chiou) → review+
Updated•9 years ago
|
Attachment #8720678 -
Flags: review?(boris.chiou) → review+
Updated•9 years ago
|
Attachment #8720679 -
Flags: review?(boris.chiou) → review+
Updated•9 years ago
|
Attachment #8720680 -
Flags: review?(boris.chiou) → review+
![]() |
||
Comment 8•9 years ago
|
||
Comment on attachment 8720675 [details] [diff] [review]
part 2 - Fix infinity handling in StickyTimeDurationValueCalculator::Multiply
Review of attachment 8720675 [details] [diff] [review]:
-----------------------------------------------------------------
Nice catch, thanks for the patch!
Attachment #8720675 -
Flags: review?(nfroyd) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e17bca1849ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed71e0a2f711
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e4a17c84ae3
https://hg.mozilla.org/integration/mozilla-inbound/rev/85e66410c01b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9130dcb9d01
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f7b038c23f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/99f8242bbd92
Assignee | ||
Comment 10•9 years ago
|
||
Spec has now been updated.
https://github.com/w3c/web-animations/commit/082696c1db5fad92226b6d8daf05242fa665df09
Summary: Make ComputedTiming.endTime handling match (soon-to-be) updated spec → Make ComputedTiming.endTime handling match updated spec
Comment 11•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e17bca1849ad
https://hg.mozilla.org/mozilla-central/rev/ed71e0a2f711
https://hg.mozilla.org/mozilla-central/rev/2e4a17c84ae3
https://hg.mozilla.org/mozilla-central/rev/85e66410c01b
https://hg.mozilla.org/mozilla-central/rev/e9130dcb9d01
https://hg.mozilla.org/mozilla-central/rev/2f7b038c23f1
https://hg.mozilla.org/mozilla-central/rev/99f8242bbd92
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•