Closed Bug 1249212 Opened 4 years ago Closed 4 years ago

Make ComputedTiming.endTime handling match updated spec

Categories

(Core :: DOM: Animation, defect)

defect
Not set

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+
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
You need to log in before you can comment on or make changes to this bug.