Splitting this off from the work in bug 1245748 because that bug is getting too big and MozReview can't cope with landing in stages.

Earlier in this patch series we divided keyframe processing into two stages: (1) Turning javascript objects into an array of Keyframe objects (2) Calculating AnimationProperty arrays from the Keyframe objects This patch creates a SetFrames method so that CSS animations and CSS transitions can skip (1) and pass the frames constructed from CSS syntax into (2). It also adds the following additional processing: a. Notifying animation mutation observers when the set of frames has changed. This is currently performed by nsAnimationManager but ultimately we should encapsulate this logic inside the effect itself. Furthermore, it will be needed when we implement effect.setFrames() (i.e. the Javascript-facing wrapper for this method). b. Preserving the mWinsInCascade and mIsRunningOnCompositor state on properties when updating them. This is currently performed by: bool KeyframeEffectReadOnly::UpdateProperties( const InfallibleTArray<AnimationProperty>& aProperties) which is what nsAnimationManager currently uses. We will ultimately remove the above method and here we are just moving this code to the new version of UpdateProperties. c. Requesting a restyle when the set of AnimationProperty objects has changed. Again, this is currently performed by the existing UpdateProperties method so we are just moving it here. This behavior will also be required when we implement effect.setFrames() and when we call UpdateProperties from elsewhere in restyling code. This is bug 1235002 but we fix it here and leave that bug to just do further cleanup work (e.g. re-instating the check for an empty property set before requesting a restyle in NotifyAnimationTimingUpdated). d. Marking the cascade as needing an update when the set of AnimationProperty objects has changed. This is in preparation for calling UpdateProperties from elsewhere in restyling (e.g. when the nsStyleContext is updated). Review commit: https://reviewboard.mozilla.org/r/43161/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43161/

Review commit: https://reviewboard.mozilla.org/r/43163/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43163/

These types have been removed from the normative part of the Web Animations spec. Review commit: https://reviewboard.mozilla.org/r/43165/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43165/

Before switching CSS animations over to using KeyframeEffectReadOnly::SetFrames we update the getFrames() API to return the set frame objects (when available) so that we can test that we are setting the correct frames. Review commit: https://reviewboard.mozilla.org/r/43167/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43167/

When we go to switch CSS Animations over to using KeyframeEffectReadOnly::SetFrames we will need a way to represent any filled-in from/to values as nsCSSValue objects. These objects are built from the current computed style. We currently use StyleAnimationValue::ExtractComputedValue for this which returns a StyleAnimationValue. In order to convert this to an nsCSSValue we can use StyleAnimationValue::UncomputeValue. However, in some cases, the nsCSSValue objects returned by that method are dependent on the passed-in StyleAnimationValue object. This patch adds an overload to UncomputeValue that takes an rvalue StyleAnimationValue reference and produces an nsCSSValue that is independent of the StyleAnimationValue through a combination of copying data and transferring ownership of data. This patch also adjusts the return value for the case of filter and shadow lists when the list is empty so that we return a none value in this case. These are the only list types which are allowed to have a null list value. Not only does this produce the correct result when these values are serialized (the initial value for 'filter', 'text-shadow', and 'box-shadow' is 'none') it also means that UncomputeValue should never return an nsCSSValue whose unit is null which is important because when we later pass that value to BuildStyleRule it will treat a null nsCSSValue as an error case (specifically, "longhand failed to parse"). Review commit: https://reviewboard.mozilla.org/r/43169/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43169/

This is needed in order to use std::stable_sort with this type since some implementations of std::stable_sort require this (as opposed to simply a move constructor). Review commit: https://reviewboard.mozilla.org/r/43171/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43171/

We will call this method in the next patch in this series. Review commit: https://reviewboard.mozilla.org/r/43173/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43173/

Review commit: https://reviewboard.mozilla.org/r/43175/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43175/

Review commit: https://reviewboard.mozilla.org/r/43177/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43177/

I wrote this test function because I was worried about what would happen if we got bad values from CSS (e.g. of ExtractComputedValue failed). Having written this patch, however, I'm not sure I want to land it. We either have to disable a bunch of assertions (which, to be fair, we will eventually drop anyway) or write some macro that conditionally turns them off depending on some debug flag we pass around. And, having confirmed that the test passes and nothing blows up I'm less worried about horrible things happenning if we get bad values from CSS.

The other thing we might want to consider here is whether we should really animate 'display' or not. With these patches it will work, but setting to display:none will cancel animations. It seems like that could generate some bad recursive behavior but I haven't yet succeeded in producing any (e.g. http://codepen.io/anon/pen/yOooOg). Maybe it's ok? CSS Transitions and CSS Animations don't say you can't animate display, but I believe Chrome doesn't allow it.

Forgive the oncoming MozReview spam (I've been pushing to get that bug fixed, but apparently it's going to be difficult). I need to add another patch into this series since apparently Mac/Android want a copy constructor for Keyframe.

It turns out that std::stable_sort on Mac and Android use this. Review commit: https://reviewboard.mozilla.org/r/43445/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43445/

Comment on attachment 8736189 [details] MozReview Request: Bug 1260655 - Add methods to CSSAnimationBuilder to build a set of Keyframe objects; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43173/diff/1-2/

Comment on attachment 8736190 [details] MozReview Request: Bug 1260655 - Use CSSAnimationBuilder::BuildAnimationFrames to set up CSS animations using Keyframe objects; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43175/diff/1-2/

Comment on attachment 8736191 [details] MozReview Request: Bug 1260655 - Drop some no-longer-needed code for setting up CSS animations using AnimationProperty objects; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43177/diff/1-2/

Sorry, more spam coming. Looks like Mac/Android what a copy assignment operator too. I'll use a follow-up bug to add a move constructor for nsCSSValue and PropertyValuePair but if Mac/Android are copying these data structures with std::stable_sort, maybe I need to introduce another wrapper type to store the initial index and use our non-stable sort with a comparator that references the index.

Comment on attachment 8736602 [details] MozReview Request: Bug 1260655 - Add a copy constructor and copy assignment operator to Keyframe; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43445/diff/1-2/

Comment on attachment 8736189 [details] MozReview Request: Bug 1260655 - Add methods to CSSAnimationBuilder to build a set of Keyframe objects; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43173/diff/2-3/

Comment on attachment 8736190 [details] MozReview Request: Bug 1260655 - Use CSSAnimationBuilder::BuildAnimationFrames to set up CSS animations using Keyframe objects; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43175/diff/2-3/

Comment on attachment 8736191 [details] MozReview Request: Bug 1260655 - Drop some no-longer-needed code for setting up CSS animations using AnimationProperty objects; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43177/diff/2-3/

Comment on attachment 8736183 [details] MozReview Request: Bug 1260655 - Add KeyframeEffectReadOnly::SetFrames; r?heycam https://reviewboard.mozilla.org/r/43161/#review41899 ::: dom/animation/KeyframeEffect.cpp:483 (Diff revision 1) > + if (mAnimation) { > + nsNodeUtils::AnimationChanged(mAnimation); > + } Do we need to check mAnimation->IsRelevant() before calling AnimationChanged here? Or can we never get in here if IsRelevant() would return false?

Comment on attachment 8736185 [details] MozReview Request: Bug 1260655 - Update keyframe-effect/constructor.html to no longer refer to PropertyIndexedKeyframes or Keyframe; r?heycam https://reviewboard.mozilla.org/r/43165/#review41903

Comment on attachment 8736186 [details] MozReview Request: Bug 1260655 - Return the stored Keyframe objects from GetFrames, when available ; r?heycam https://reviewboard.mozilla.org/r/43167/#review41905

Comment on attachment 8736187 [details] MozReview Request: Bug 1260655 - Allow StyleAnimationValue::UncomputeValue to produce values whose storage is independent of the passed-in computed value; r?heycam https://reviewboard.mozilla.org/r/43169/#review41907

Comment on attachment 8736188 [details] MozReview Request: Bug 1260655 - Add an assignment operator to Keyframe that takes an rvalue reference; r?heycam https://reviewboard.mozilla.org/r/43171/#review41909

Comment on attachment 8736602 [details] MozReview Request: Bug 1260655 - Add a copy constructor and copy assignment operator to Keyframe; r?heycam https://reviewboard.mozilla.org/r/43445/#review41911 r=me but is this an indication that stable_sort will do a bunch of copying that we would prefer to avoid, on those platforms?

Comment on attachment 8736189 [details] MozReview Request: Bug 1260655 - Add methods to CSSAnimationBuilder to build a set of Keyframe objects; r?heycam https://reviewboard.mozilla.org/r/43173/#review41913 ::: layout/style/nsAnimationManager.cpp:917 (Diff revision 3) > + const nsCSSKeyframesRule* aRule) > +{ > + // Ideally we'd like to build up a set of Keyframe objects that more-or-less > + // reflects the keyframes as-specified in the @keyframes rule(s). However, > + // that proves to be difficult because the way CSS declarations are processed > + // differs from how we are able to represent keyframes as Javascript objects. Nit: "JavaScript" (and below). ::: layout/style/nsAnimationManager.cpp:1055 (Diff revision 3) > + existingKeyframe-> > + mPropertyValues.AppendElements(Move(uniquePropertyValues)); Note, if you haven't realised (since it's not obvious), but the only time this will move elements and not copy them is if mPropertyValues is empty (in which case it swaps the arrays). ::: layout/style/nsAnimationManager.cpp:1087 (Diff revision 3) > + RefPtr<nsStyleContext> keyframeRuleContext = > + mResolvedStyles.Get(aPresContext, mStyleContext, > + aKeyframeRule->Declaration()); Maybe move this into the if statement? ::: layout/style/nsAnimationManager.cpp:1104 (Diff revision 3) > + > + return result; > +} > + > +static Maybe<ComputedTimingFunction> > +ConvertTimingFunction(const nsTimingFunction& aTimingFunction) { Nit: brace on new line.

Comment on attachment 8736190 [details] MozReview Request: Bug 1260655 - Use CSSAnimationBuilder::BuildAnimationFrames to set up CSS animations using Keyframe objects; r?heycam https://reviewboard.mozilla.org/r/43175/#review41915 ::: layout/style/nsAnimationManager.cpp:333 (Diff revision 3) > > return nullptr; > } > > static void > UpdateOldAnimationPropertiesWithNew( Should we rename this method now that it's working on Keyframes? ::: layout/style/nsAnimationManager.cpp:349 (Diff revision 3) > if (aOld.GetEffect()) { > KeyframeEffectReadOnly* oldEffect = aOld.GetEffect(); > animationChanged = > oldEffect->SpecifiedTiming() != aNewTiming; > oldEffect->SetSpecifiedTiming(aNewTiming); > - animationChanged |= > + oldEffect->SetFrames(Move(aNewFrames), aStyleContext); Why do we no longer have to update animationChanged?

Comment on attachment 8736191 [details] MozReview Request: Bug 1260655 - Drop some no-longer-needed code for setting up CSS animations using AnimationProperty objects; r?heycam https://reviewboard.mozilla.org/r/43177/#review41917

(In reply to Cameron McCormack (:heycam) from comment #29) > Comment on attachment 8736190 [details] > MozReview Request: Bug 1260655 - Use > CSSAnimationBuilder::BuildAnimationFrames to set up CSS animations using > Keyframe objects; r?heycam > > https://reviewboard.mozilla.org/r/43175/#review41915 > > ::: layout/style/nsAnimationManager.cpp:333 > (Diff revision 3) > > > > return nullptr; > > } > > > > static void > > UpdateOldAnimationPropertiesWithNew( > > Should we rename this method now that it's working on Keyframes? > > ::: layout/style/nsAnimationManager.cpp:349 > (Diff revision 3) > > if (aOld.GetEffect()) { > > KeyframeEffectReadOnly* oldEffect = aOld.GetEffect(); > > animationChanged = > > oldEffect->SpecifiedTiming() != aNewTiming; > > oldEffect->SetSpecifiedTiming(aNewTiming); > > - animationChanged |= > > + oldEffect->SetFrames(Move(aNewFrames), aStyleContext); > > Why do we no longer have to update animationChanged? Because SetFrames does the work in the case of a change to the frames.

(In reply to Brian Birtles (:birtles, away 13 April) from comment #31) > Because SetFrames does the work in the case of a change to the frames. Does that mean we can end up calling nsNodeUitls::AnimationChanged twice (and is that OK due to coalescing)?

(In reply to Cameron McCormack (:heycam) from comment #32) > (In reply to Brian Birtles (:birtles, away 13 April) from comment #31) > > Because SetFrames does the work in the case of a change to the frames. > > Does that mean we can end up calling nsNodeUitls::AnimationChanged twice > (and is that OK due to coalescing)? Right. I plan to file a follow-up bug to make SetSpecifiedTiming etc. also do observer notifications so we can remove animationChanged altogether.

Comment on attachment 8736190 [details] MozReview Request: Bug 1260655 - Use CSSAnimationBuilder::BuildAnimationFrames to set up CSS animations using Keyframe objects; r?heycam https://reviewboard.mozilla.org/r/43175/#review41929 OK, thanks for the clarification.

(In reply to Cameron McCormack (:heycam) from comment #22) > > + if (mAnimation) { > > + nsNodeUtils::AnimationChanged(mAnimation); > > + } > > Do we need to check mAnimation->IsRelevant() before calling AnimationChanged > here? Or can we never get in here if IsRelevant() would return false? No, I think you're right, I'll add the IsRelevant() check here.

(In reply to Cameron McCormack (:heycam) from comment #27) > Comment on attachment 8736602 [details] > MozReview Request: Bug 1260655 - Add a copy constructor and copy assignment > operator to Keyframe; r?heycam > > https://reviewboard.mozilla.org/r/43445/#review41911 > > r=me but is this an indication that stable_sort will do a bunch of copying > that we would prefer to avoid, on those platforms? Yes. I've filed bug 1263500 for this.

(In reply to Cameron McCormack (:heycam) from comment #28) > ::: layout/style/nsAnimationManager.cpp:1055 > (Diff revision 3) > > + existingKeyframe-> > > + mPropertyValues.AppendElements(Move(uniquePropertyValues)); > > Note, if you haven't realised (since it's not obvious), but the only time > this will move elements and not copy them is if mPropertyValues is empty (in > which case it swaps the arrays). Oh, I hadn't. That's a shame.

(In reply to Cameron McCormack (:heycam) from comment #29) > ::: layout/style/nsAnimationManager.cpp:333 > (Diff revision 3) > > > > return nullptr; > > } > > > > static void > > UpdateOldAnimationPropertiesWithNew( > > Should we rename this method now that it's working on Keyframes? I left this as it was because this method updates both keyframes and timing properties and I figured "animation properties" was a reasonable generic term to cover both?

(In reply to Brian Birtles (:birtles, away 13 April) from comment #38) > I left this as it was because this method updates both keyframes and timing > properties and I figured "animation properties" was a reasonable generic > term to cover both? OK, no problem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5db982657c9b https://hg.mozilla.org/integration/mozilla-inbound/rev/8272d22371b7 https://hg.mozilla.org/integration/mozilla-inbound/rev/852340c877fb https://hg.mozilla.org/integration/mozilla-inbound/rev/33b9f509fab7 https://hg.mozilla.org/integration/mozilla-inbound/rev/51a410cc49c8 https://hg.mozilla.org/integration/mozilla-inbound/rev/bd357fbe9688 https://hg.mozilla.org/integration/mozilla-inbound/rev/77d2df1822a9 https://hg.mozilla.org/integration/mozilla-inbound/rev/d846bcdce3e9 https://hg.mozilla.org/integration/mozilla-inbound/rev/063370aa01ab https://hg.mozilla.org/integration/mozilla-inbound/rev/52c0128aff10

https://hg.mozilla.org/mozilla-central/rev/5db982657c9b https://hg.mozilla.org/mozilla-central/rev/8272d22371b7 https://hg.mozilla.org/mozilla-central/rev/852340c877fb https://hg.mozilla.org/mozilla-central/rev/33b9f509fab7 https://hg.mozilla.org/mozilla-central/rev/51a410cc49c8 https://hg.mozilla.org/mozilla-central/rev/bd357fbe9688 https://hg.mozilla.org/mozilla-central/rev/77d2df1822a9 https://hg.mozilla.org/mozilla-central/rev/d846bcdce3e9 https://hg.mozilla.org/mozilla-central/rev/063370aa01ab https://hg.mozilla.org/mozilla-central/rev/52c0128aff10

I'm not sure if this actually needs documentation or not, but, as a result of this bug, CSS animations on various enum-type properties will now exhibit the 50% switch behavior described here: https://drafts.csswg.org/css-transitions/#step-types Note that, like Chrome, we don't apply this to CSS transitions (yet, anyway). The set of properties affected is, I think, all those in [1] with eStyleAnimType_EnumU8, although there may be other cases of properties where certain combinations of values where not interpolable but now exhibit the 50% switch behavior. Also, there are probably other properties marked as eStyleAnimType_None which should also exhibit this behavior. We'll fix that in bug 1260572 after checking what makes sense and what Chrome/etc. do. [1] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSPropList.h