KeyframeEffectReadOnly::ComposeStyle should check the result of StyleAnimationValue::Interpolate

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM: Animation
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: birtles, Unassigned)

Tracking

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Splitting this off from my work in bug 1245748.
(Reporter)

Comment 1

2 years ago
Created attachment 8736143 [details]
MozReview Request: Bug 1260572 - Replace AnimValuesStyleRule::AddEmptyValue with an overload of AddValue that takes an rvalue reference; r?heycam

In the next patch in this series, we would like to update the error handling of
the call to StyleAnimationValue::Interpolate in
KeyframeEffectReadOnly::ComposeStyle. Using AnimValuesStyleRule::AddEmptyValue
there, however, makes handling the error case difficult because we need a means
of clearing the allocated StyleAnimationValue.

However, simply using AnimationValuesStyleRule::AddValue means we will end up
doing needless allocations for StyleAnimationValue objects (the copy
constructor for which can result in performing potentially expensive heap
allocations, such as when lists are deep-copied).

Instead, we add a Move constructor to StyleAnimationValue and add an overload
of AnimValuesStyleRule::AddValue that takes an rvalue reference. This
provides a more consistent interface to AnimValuesStyleRule and avoids the
unnecessary allocations from copying StyleAnimationValue objects.

Review commit: https://reviewboard.mozilla.org/r/43133/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43133/
Attachment #8736143 - Flags: review?(cam)
Attachment #8736144 - Flags: review?(cam)
(Reporter)

Comment 2

2 years ago
Created attachment 8736144 [details]
MozReview Request: Bug 1260572 - Use 50% switch behavior if StyleAnimationValue::Interpolate fails; r?heycam

In KeyframeEffectReadOnly::ComposeStyle we call StyleAnimationValue::Interpolate
but assume that it always passes. That was true when that code was only used for
CSS animations and CSS transitions since they check that their animation values
can be interpolated before setting up segments.

However, when we set up animations using the Web Animations API we don't perform
that check so it is possible for this call to fail.

In that case, we could just bail, but, according to CSS Transitions we should
apply a 50% switch in this case:

  https://drafts.csswg.org/css-transitions/#step-types

(In Web Animations, specifying this is an open issue. See:
https://w3c.github.io/web-animations/#specific-animation-behaviors).

Bug 1064937 tracks doing this in general (we'll likely need to mark various
properties as being no longer unanimatable but instead as supporting discrete
animation) but we can start to introduce it now.

Later in bug 1245748, CSS animations and transitions will likely start using
the same code path as the Web Animations API for setting up keyframes.
As a result, unless we take care to add checks that the values we set are
interpolable, the 50% switch behavior will begin to apply to CSS animations and
transitions too at that point.

Some concerns have been raised about possible web compatibility issues around
the 50% switch behavior (see [1] and [2]). For CSS animations, Chrome already
supports this behavior so it should be ok at least for CSS animations.
When we switch CSS transitions over to the same code path, however, we will need
to be careful to add checks that the transition endpoints are interpolable
(we can investigate introducing this behavior to transitions as a separate bug
that can be easily backed out / preffed off).

Regarding the naming of the test added here, going forward we would like to
restructure the tests under web-platform-tests to better match the structure of
the Web Animations since that seems to be the convention there.

However, this doesn't *quite* match the structure of the spec since there are
upcoming changes to the spec in this area (e.g. renaming animation behaviors to
animation types). However, it should be close enough that we don't have to move
it around too much in future.

[1] https://drafts.csswg.org/css-transitions/#step-types
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1064937#c0

Review commit: https://reviewboard.mozilla.org/r/43135/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43135/
(Reporter)

Updated

2 years ago
Blocks: 1260655
Comment on attachment 8736143 [details]
MozReview Request: Bug 1260572 - Replace AnimValuesStyleRule::AddEmptyValue with an overload of AddValue that takes an rvalue reference; r?heycam

https://reviewboard.mozilla.org/r/43133/#review41895

::: dom/animation/AnimValuesStyleRule.h:41
(Diff revision 1)
>      PropertyValuePair v = { aProperty, aStartValue };
>      mPropertyValuePairs.AppendElement(v);

I'm curious whether this ends up calling the StyleAnimationValue copy constructor twice (once to copy in to the local variable, and second in the AppendElement call).
Attachment #8736143 - Flags: review?(cam) → review+
Comment on attachment 8736144 [details]
MozReview Request: Bug 1260572 - Use 50% switch behavior if StyleAnimationValue::Interpolate fails; r?heycam

https://reviewboard.mozilla.org/r/43135/#review41897
Attachment #8736144 - Flags: review?(cam) → review+
(Reporter)

Comment 5

2 years ago
(In reply to Cameron McCormack (:heycam) from comment #3)
> Comment on attachment 8736143 [details]
> MozReview Request: Bug 1260572 - Replace AnimValuesStyleRule::AddEmptyValue
> with an overload of AddValue that takes an rvalue reference; r?heycam
> 
> https://reviewboard.mozilla.org/r/43133/#review41895
> 
> ::: dom/animation/AnimValuesStyleRule.h:41
> (Diff revision 1)
> >      PropertyValuePair v = { aProperty, aStartValue };
> >      mPropertyValuePairs.AppendElement(v);
> 
> I'm curious whether this ends up calling the StyleAnimationValue copy
> constructor twice (once to copy in to the local variable, and second in the
> AppendElement call).

Yes, it does, at least in a debug build.

We could fix that by using the form of AppendElement that allocates a PropertyValuePair and then filling it in (1 copy). However, this variant only gets called for zero-length segments so I don't think it's particularly critical.

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/debc00d94145
https://hg.mozilla.org/mozilla-central/rev/3baed5c339e3
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.