Closed Bug 1249212 Opened 9 years ago Closed 9 years ago

Make ComputedTiming.endTime handling match updated spec

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

()

Details

Attachments

(7 files)

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.
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: nobody → bbirtles
Status: NEW → ASSIGNED
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)
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)
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)
Attachment #8720679 - Flags: review?(boris.chiou)
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)
Attachment #8720674 - Flags: review?(boris.chiou) → review+
Attachment #8720676 - Flags: review?(boris.chiou) → review+
Attachment #8720677 - Flags: review?(boris.chiou) → review+
Attachment #8720678 - Flags: review?(boris.chiou) → review+
Attachment #8720679 - Flags: review?(boris.chiou) → review+
Attachment #8720680 - Flags: review?(boris.chiou) → review+
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+
Summary: Make ComputedTiming.endTime handling match (soon-to-be) updated spec → Make ComputedTiming.endTime handling match updated spec
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: