As discussed in bug 1239889 comment 3, we currently compute animation values in the KeyframeEffect(ReadOnly) constructor. That's not quite right since it should be possible to construct a KeyframeEffect(ReadOnly) without a target element or with a target element that doesn't have a current document. Furthermore, if the target element is rebound to the different part of the tree, or replaced with a different target element, or if font-size on an ancestor element changed, etc. those computed values could change so we need to recalculate the computed values. Re-computing values every frame is probably too expensive but Cameron suggests there's something in nsIFrame we could hook into to update when the style context changes.

I checked what Chrome does here and it appears to update based on context (you need to open the console to see the output).

Ideally we should fix this before shipping in order to match Chrome.

If this could work correctly, I think bug 1245260 will be also fixed.

This is turning out to be quite involved. Here's what I have in mind so far: ★ Stage A: Store frames with their specified values in parallel with computed segments ★ This serves two purposes: i. So we can return the frames in mostly the same format as they were set (bug 1217252) ii. So we can preserve the specified style values (this bug) So, adding something like the following to KeyframeEffectReadOnly: struct SpecifiedKeyframe { Maybe<float> mOffset; Maybe<ComputedTimingFunction> mTimingFunction; nsTArray<Pair<nsCSSProperty, nsCSSValue>> mPropertyValues; // Later we can add extra members for storing unsupported properties // (and possibly unsupported values if we don't store them above) // e.g. nsTArray<Pair<nsString, nsString>> mUnsupportedValues; }; nsTArray<SpecifiedKeyframe> mSpecifiedFrames; Storing an nsCSSValue, however, means we'd be expanding shorthands and getFrames() would always return only longhand properties. That might be necessary anyway but seems pretty difficult for the author given our current scheme where longhands override shorthands. e.g. var anim = elem.animate({ borderWidth: [ '0px', '100px' ] }, 1000); // Actually, make it go from 50px to 100px var frames = anim.effect.getFrames(); frames[0].borderWidth = '50px'; anim.effect.setFrames(frames); // Nothing changes because frames now looks like: // // frames = [ // { borderLeftWidth: '0px', // borderTopWidth: '0px', // borderRightWidth: '0px', // borderBottomWidth: '0px', // borderWidth: '50px' // }, // { borderLeftWidth: '100px', // borderTopWidth: '100px', // borderRightWidth: '100px', // borderBottomWidth: '100px' // } ] // // And borderWidth will be overridden by the longhands. We could *try* to preserve the original frames intact but I suspect that might be hard? Anyway, assuming we can solve that I'd be looking to do something like: 1. Make KeyframeEffectReadOnly::Constructor build up mSpecifiedKeyframes in parallel with the list of AnimationProperty objects (and their segments). Ideally, we probably want to add some sort of SetFrames() method that does most of the grunt work--something we can re-use in later steps and also re-use when we later implement the content-facing SetFrames() method. 2. Make KeyframeEffectReadOnly::GetFrames() use mSpecifiedKeyframes as its return value *when available* and get existing tests to pass. 3. Add further tests for things like 'em' units not being converted to 'px'. 4. Fix bug 1217252 at this point and add tests for it -- i.e. don't merge the passed-in keyframes too soon. 5. Make nsAnimationManager::BuildAnimations build up mSpecifiedKeyframes (e.g. by calling SetFrames() with the appropriate objects) and get existing tests to pass. Add tests that units are preserved, shorthands are not expanded or whatever other behavioral changes we expect to introduce. 6. Make nsTransitionManager::BuildAnimations build up mSpecifiedKeyframes and get existing tests to pass. Add tests as in step 5. 7. Remove check from GetFrames() that mSpecifiedKeyframes is available since it should now always be available. Likewise, remove any code in GetFrames that works off the segment list. ★ Stage B: Use stored frames to calculate segments ★ 1. Add necessary machinery to StyleAnimationValue to convert from an nsCSSValue (or whatever representation we use in A, above) to a StyleAnimationValue, rather than from an nsAString. 2. Move piece-by-piece all the code for building up animation property segments in KeyframeEffectReadOnly::Constructor / nsAnimationManager / nsTransitionManager to some common function in KeyframeEffectReadOnly. We'll need to be a bit careful about detecting whether the keyframes have changed or not. For the purpose of dispatching mutation observer records we only really want to compare based on mSpecifiedKeyframes (and this would allow us to remove some of the odd behavior in AnimationProperty::operator==). However, whenever the computed value of any of the keyframe values changes, we need to trigger a layer update. That might be an orthogonal step, however triggered by the frame we're targetting. 3. Make SetFrames() automatically build up the computed segments (by calling the function from 2) whilst preserving the flags on each AnimationProperty like KeyframeEffectReadOnly::CopyPropertiesFrom currently does. 4. Remove any remaining old segment building code outside of KeyframeEffectReadOnly. 5. Hook up to nsIFrame such that whenever the style context changes we blow away the computed segments and recompute them on-demand. 6. Make SetFrames *not* compute segments immediately (this seems to be important to avoid some of the cases of bugs where we get in knots updating animations and recomputing style at the same time). 7. Add tests that when the context changes, animation values are updated (like attachment 8715646 [details] but preferably as a web-platform-tests style reftest) and get it to pass. 8. Add tests for creating an animation on an element without a doc and revert the code that makes us throw in that case: https://hg.mozilla.org/mozilla-central/rev/3bb3381dc5f8 9. (Possibly separate bug) Optimize the representation of segments so we don't repeat all the offsets. 10. (Separate bug) Store unsupported properties and values and add tests. Cameron, what do you think about storing the specified values using nsCSSValue and the implication that getFrames() will then always return shorthands? Are there any other parts of this plan that seem problematic?

(In reply to Brian Birtles (:birtles) from comment #4) > Storing an nsCSSValue, however, means we'd be expanding shorthands and > getFrames() would always return only longhand properties. That might be > necessary anyway but seems pretty difficult for the author given our current > scheme where longhands override shorthands. > > e.g. > > var anim = elem.animate({ borderWidth: [ '0px', '100px' ] }, 1000); > // Actually, make it go from 50px to 100px > var frames = anim.effect.getFrames(); > frames[0].borderWidth = '50px'; > anim.effect.setFrames(frames); > > // Nothing changes because frames now looks like: > // > // frames = [ > // { borderLeftWidth: '0px', > // borderTopWidth: '0px', > // borderRightWidth: '0px', > // borderBottomWidth: '0px', > // borderWidth: '50px' > // }, > // { borderLeftWidth: '100px', > // borderTopWidth: '100px', > // borderRightWidth: '100px', > // borderBottomWidth: '100px' > // } ] > // > // And borderWidth will be overridden by the longhands. Actually, I notice we already do this so we already have this problem.

The overall plan sounds OK, and a solution that doesn't involve expanding out shorthands on the keyframe objects would be good. What about storing shorthand values as strings? (You could use eCSSUnit_TokenStream as the nsCSSValue type.) Then when we parse and expand those shorthands at the time we build the computed value keyframes. Also, for animations that don't have a target element, how will we produce computed values, given we won't have anything to resolve relative units against? Will we use the document element? Will we want the Web Animations API available in workers (or worklets) in the future, and so potentially not have any element to resolve values against? These problems all sound like open spec issues. Have they been raised with other implementors, and what do they think?

(In reply to Cameron McCormack (:heycam) (away Feb 27 – Mar 13) from comment #6) > The overall plan sounds OK, and a solution that doesn't involve expanding > out shorthands on the keyframe objects would be good. What about storing > shorthand values as strings? (You could use eCSSUnit_TokenStream as the > nsCSSValue type.) Then when we parse and expand those shorthands at the > time we build the computed value keyframes. Yeah, I'll have a look at doing that. > Also, for animations that don't have a target element, how will we produce > computed values, given we won't have anything to resolve relative units > against? Will we use the document element? Will we want the Web Animations > API available in workers (or worklets) in the future, and so potentially not > have any element to resolve values against? I might be missing something but I don't think we need to produce computed values if we don't have a target element, right? That's the motivation behind this bug: store the specified values so you can create an animation without a target and then resolve the computed values when you attach a target. As for the workers question, I don't think we will be doing that in the short-term so I think we can address that when we come to it. > These problems all sound like open spec issues. Have they been raised with > other implementors, and what do they think? No-one else implements getFrames() yet so they haven't come across it. I think we need to work out what we want the behavior to be and what's achievable then we can work on speccing that. I'll probably send a mail in the next few days about some possible alternatives to this API.

I've made some progress on this. This WIP patch makes us store values as specified values and re-resolve the computed values whenever the style context changes. Still to do: * It still doesn't work if you create an animation on an element not attached to the document tree and then attach it. We update the properties correctly and re-request a style but I suspect we're failing somewhere else (e.g. to get the timeline to observe the refresh driver, or to create an effect set, or something of the sort) * It doesn't respond to changes in the parent style. E.g. if you use em units and update the fontSize on the parent we don't seem to get a call to GetContext(). We might need to hook in somewhere different. * I haven't switched CSS Animations or CSS Transitions over to the new SetFrames() API so they're possibly completely broken. Once this works, however, we should be able to remove a lot of code.

Here's an updated exploratory patch. I'm going to start pulling pieces out of this to stick into dependent bugs.

Comment on attachment 8730179 [details] [diff] [review] Merge inbound to m-c. a=merge Wrong bug, I think.

(In reply to Brian Birtles (:birtles) from comment #11) > Comment on attachment 8730179 [details] [diff] [review] > Merge inbound to m-c. a=merge > > Wrong bug, I think. Sooooo sorry. Something must be wrong w/ my git-bz tool. =(

Before we begin re-arranging KeyframeEffect.h we move ComputedTiming aside since putting it in a separate file should make navigating the source easier. Review commit: https://reviewboard.mozilla.org/r/41679/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41679/

In particular, the spec no longer has Keyframe and PropertyIndexedKeyframes types but rather deals with objects. The algorithms for processing these objects, however, define various Base* types: https://w3c.github.io/web-animations/#dictdef-basepropertyindexedkeyframe https://w3c.github.io/web-animations/#dictdef-basekeyframe https://w3c.github.io/web-animations/#dictdef-basecomputedkeyframe Review commit: https://reviewboard.mozilla.org/r/41681/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41681/

Specifically, for the 'composite' member on keyframes, we now indicate "use the composite value specified on the effect" using a missing/undefined 'composite' member as opposed to a null value. Review commit: https://reviewboard.mozilla.org/r/41683/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41683/

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

Once we tweak moz.build in the next patch, the grouping in the unified build will change and expose these missing includes so we fix them here, first. Review commit: https://reviewboard.mozilla.org/r/41687/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41687/

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

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

StyleAnimationValue::ComputeValue(s) will automatically look up the style context of the supplied element. This is mostly fine, but when we start using this method in scenarios where we are building the initial style context (as happens later in this patch series) it can easily land us in a situation where we iterate indefinitely. It would be better, instead, to just explicitly pass in the style context we want to use, as we already do for StyleAnimationValue::ExtractComputedValue. Review commit: https://reviewboard.mozilla.org/r/41693/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41693/

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

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

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

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

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

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

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

Cameron, really sorry about the review bomb here. This is only the first stage too. It's been quite busy over here so I haven't had time to add the amount of detailed comments I normally try to, but I think you know this code well so hopefully it's ok.

Looks like I missed an assertion. https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a466128afef

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

Comment on attachment 8733258 [details] MozReview Request: Bug 1245748 - 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/41695/diff/1-2/

Comment on attachment 8733259 [details] MozReview Request: Bug 1245748 - Split PropertyPriorityComparator into a separate (reusable) class; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41697/diff/1-2/

Comment on attachment 8733260 [details] MozReview Request: Bug 1245748 - Add PropertyPriorityIterator; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41699/diff/1-2/

Comment on attachment 8733261 [details] MozReview Request: Bug 1245748 - Add GetAnimationPropertiesFromKeyframes; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41701/diff/1-2/

Comment on attachment 8733262 [details] MozReview Request: Bug 1245748 - Add ApplyDistributeSpacing for Keyframe objects; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41703/diff/1-2/

Comment on attachment 8733263 [details] MozReview Request: Bug 1245748 - Use Keyframe-based utility functions when constructing KeyframeEffect(ReadOnly); r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41705/diff/1-2/

Comment on attachment 8733264 [details] MozReview Request: Bug 1245748 - Remove no-longer-needed code for directly setting up properties in KeyframeEffect(ReadOnly) constructor; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41707/diff/1-2/

Comment on attachment 8733251 [details] MozReview Request: Bug 1245748 - Wrap lines in keyframe-effect/constructor.html to 80 chars; r=whitespace-only https://reviewboard.mozilla.org/r/41681/#review38195 r=me, though I wish mozreview showed deleted files without having to jump through hoops. :(

Comment on attachment 8733252 [details] MozReview Request: Bug 1245748 - Update keyframe-effect/constructor.html to no longer refer to PropertyIndexedKeyframes or Keyframe; r?heycam https://reviewboard.mozilla.org/r/41683/#review38199 ::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html:130 (Diff revision 1) > var gBadCompositeValueTests = [ > - "unrecognised", "replace ", "Replace" > + "unrecognised", "replace ", "Replace", null > ]; > > test(function(t) { > - gGoodCompositeValueTests.forEach(function(composite) { > + var getFrame = function(composite) { You don't need this, do you? In particular this part: if (typeof composite !== "undefined") { frame.composite = composite; } is equivalent to: frame.composite = composite; if you plan to do dictionary processing on `frame`. ::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html:156 (Diff revision 1) > - gGoodCompositeValueTests.forEach(function(composite) { > - var effect = new KeyframeEffectReadOnly(target, [ > - { offset: 0, left: "10px", composite: composite }, > + var getFrames = function(composite) { > + var frames = [ > + { offset: 0, left: "10px" }, > { offset: 1, left: "20px" } > - ]); > + ]; > + if (typeof composite !== "undefined") { Again, you don't need this complexity here. r=me modulo those, I guess, though I don't see why we don't need to change other mComposite consumers. Do we not have any?

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

Comment on attachment 8733251 [details] MozReview Request: Bug 1245748 - Wrap lines in keyframe-effect/constructor.html to 80 chars; r=whitespace-only https://reviewboard.mozilla.org/r/41681/#review38205

(In reply to Boris Zbarsky [:bz] from comment #40) > You don't need this, do you? In particular this part: > > if (typeof composite !== "undefined") { > frame.composite = composite; > } > > is equivalent to: > > frame.composite = composite; Right. Sorry, I had meant to fix that! > r=me modulo those, I guess, though I don't see why we don't need to change > other mComposite consumers. Do we not have any? Right, we don't actually implement composite modes yet, so the only place affected at the moment are these tests. (And it turns out I missed a couple of tests.)

Comment on attachment 8733252 [details] MozReview Request: Bug 1245748 - Update keyframe-effect/constructor.html to no longer refer to PropertyIndexedKeyframes or Keyframe; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41683/diff/1-2/

Comment on attachment 8733253 [details] MozReview Request: Bug 1245748 - Return the stored Keyframe objects from GetFrames, when available ; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41685/diff/1-2/

Comment on attachment 8733254 [details] MozReview Request: Bug 1245748 - Allow StyleAnimationValue::UncomputeValue to produce values whose storage is independent of the passed-in computed value; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41687/diff/1-2/

Comment on attachment 8733255 [details] MozReview Request: Bug 1245748 - Add an assignment operator to Keyframe that takes an rvalue reference; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41689/diff/1-2/

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

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

Comment on attachment 8733258 [details] MozReview Request: Bug 1245748 - 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/41695/diff/2-3/

Comment on attachment 8733259 [details] MozReview Request: Bug 1245748 - Split PropertyPriorityComparator into a separate (reusable) class; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41697/diff/2-3/

Comment on attachment 8733260 [details] MozReview Request: Bug 1245748 - Add PropertyPriorityIterator; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41699/diff/2-3/

Comment on attachment 8733261 [details] MozReview Request: Bug 1245748 - Add GetAnimationPropertiesFromKeyframes; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41701/diff/2-3/

Comment on attachment 8733262 [details] MozReview Request: Bug 1245748 - Add ApplyDistributeSpacing for Keyframe objects; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41703/diff/2-3/

Comment on attachment 8733263 [details] MozReview Request: Bug 1245748 - Use Keyframe-based utility functions when constructing KeyframeEffect(ReadOnly); r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41705/diff/2-3/

Comment on attachment 8733264 [details] MozReview Request: Bug 1245748 - Remove no-longer-needed code for directly setting up properties in KeyframeEffect(ReadOnly) constructor; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41707/diff/2-3/

Comment on attachment 8733250 [details] MozReview Request: Bug 1245748 - Add KeyframeEffectReadOnly::SetFrames; r?heycam https://reviewboard.mozilla.org/r/41679/#review38339 ::: dom/animation/KeyframeEffect.h:71 (Diff revision 1) > + > + Nit: Not sure these blank lines are really needed.

Comment on attachment 8733251 [details] MozReview Request: Bug 1245748 - Wrap lines in keyframe-effect/constructor.html to 80 chars; r=whitespace-only https://reviewboard.mozilla.org/r/41681/#review38341 ::: dom/animation/KeyframeEffect.cpp:1725 (Diff revision 1) > - // A frame list specification in the IDL is: > + // A frame list specification in the spec is essentially: > // > - // (PropertyIndexedKeyframes or sequence<Keyframe> or SharedKeyframeList) > + // (PropertyIndexedKeyframe or sequence<Keyframe> or SharedKeyframeList) > // > // We don't support SharedKeyframeList yet, but we do the other two. We > // manually implement the parts of JS-to-IDL union conversion algorithm > // from the Web IDL spec, since we have to represent this an object? so > // we can look at the open-ended set of properties on a > - // PropertyIndexedKeyframes or Keyframe. > + // PropertyIndexedKeyframe or Keyframe. Since PropertyIndexedKeyframe doesn't exist in the codebase now, it might be slightly confusing to reference it here. Maybe link to https://w3c.github.io/web-animations/#processing-a-frames-argument? Also, the spec still talks about "PropertyIndexedKeyframes" with the trailing "s". ::: dom/webidl/BaseKeyframeTypes.webidl:22 (Diff revision 1) > +dictionary BasePropertyIndexedKeyframe > +{ Nit: brace on previous line to be idiomatic IDL style. ::: dom/webidl/BaseKeyframeTypes.webidl:34 (Diff revision 1) > -dictionary ComputedKeyframe : Keyframe { > +dictionary BaseComputedKeyframe : BaseKeyframe > +{ Nit: Here too.

Comment on attachment 8733252 [details] MozReview Request: Bug 1245748 - Update keyframe-effect/constructor.html to no longer refer to PropertyIndexedKeyframes or Keyframe; r?heycam https://reviewboard.mozilla.org/r/41683/#review38347 ::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html:383 (Diff revision 2) > { offset: 0.0, easing: "ease", top: "20px"}, > { offset: 0.5, easing: "linear", left: "30px" }, > { offset: 0.5, easing: "linear", top: "40px" }, > { offset: 1.0, easing: "step-end", left: "50px" }, > { offset: 1.0, easing: "step-end", top: "60px" }], > - output: [{ offset: 0.0, computedOffset: 0.0, easing: "ease", composite: "replace", left: "10px", top: "20px" }, > + output: [{ offset: 0.0, computedOffset: 0.0, easing: "ease", left: "10px", top: "20px" }, Nit: something went wrong with the alignment here. Can you re-align the "left:" with the one below it?

Comment on attachment 8733253 [details] MozReview Request: Bug 1245748 - Return the stored Keyframe objects from GetFrames, when available ; r?heycam https://reviewboard.mozilla.org/r/41685/#review38357 ::: dom/animation/KeyframeEffect.h:58 (Diff revision 2) > } > > +/** > + * A property-value pair specified on a keyframe. > + */ > +struct PropertyValuePair { Nit: brace on new line. ::: dom/animation/KeyframeEffect.h:81 (Diff revision 2) > + * > + * When the target element or style context changes, however, we rebuild these > + * per-property arrays from the original list of keyframes objects. As a result, > + * these objects represent the master definition of the effect's values. > + */ > +struct Keyframe { Nit: brace on new line.

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

Comment on attachment 8733255 [details] MozReview Request: Bug 1245748 - Add an assignment operator to Keyframe that takes an rvalue reference; r?heycam https://reviewboard.mozilla.org/r/41689/#review38361 I didn't check that this is only a code move and that other changes were introduced. ::: dom/animation/KeyframeUtils.h:16 (Diff revision 2) > +namespace mozilla { > +struct AnimationProperty; > +enum class CSSPseudoElementType : uint8_t; > +class ErrorResult; > + > +namespace dom { > +class Element; > +} // namespace dom > +} // namespace mozilla Nit: I'm not sure I've seen people align forward declared types like this. It feels a bit odd to me so I probably wouldn't bother.

Comment on attachment 8733256 [details] MozReview Request: Bug 1245748 - Add methods to CSSAnimationBuilder to build a set of Keyframe objects; r?heycam https://reviewboard.mozilla.org/r/41691/#review38371 ::: dom/animation/KeyframeUtils.cpp:376 (Diff revision 2) > + MOZ_ASSERT(!aRv.Failed()); > + > + nsTArray<Keyframe> keyframes; > + > + if (!aFrames) { > + // The argument was explicitly null meaning no kyeframes. keyframes ::: dom/animation/KeyframeUtils.cpp:377 (Diff revision 2) > + > + nsTArray<Keyframe> keyframes; > + > + if (!aFrames) { > + // The argument was explicitly null meaning no kyeframes. > + return Move(keyframes); Is the Move() necessary here? I think we can just rely on return value optimization and |return keyframes;| here and at the rest of the return statements in this method. (I'm not sure what effect using Move() here has on RVO.) ::: dom/animation/KeyframeUtils.cpp:623 (Diff revision 2) > + if (!keyframeDict.mOffset.IsNull()) { > + keyframe->mOffset.emplace(keyframeDict.mOffset.Value()); > + } These are the same type? I think you can just do: keyframe->mOffset = keyframeDict.mOffset; ::: dom/animation/KeyframeUtils.cpp:809 (Diff revision 2) > + } > + > + if (value.GetUnit() == eCSSUnit_Null) { > + // Either we have a shorthand, or we failed to parse a longhand. > + // In either case, store the string value as a token stream. > + nsCSSValueTokenStream* tokenStream = new nsCSSValueTokenStream; I just noticed mLineNumber/mLineOffset aren't initialized, since they're not initialized in the nsCSSValueTokenStream constructor. Can you file a followup to fix that? ::: dom/animation/KeyframeUtils.cpp:811 (Diff revision 2) > + // By setting mShorthandPropertyID to unknown, we ensure that when > + // we call nsCSSValue::AppendToString we get back the string stored > + // in mTokenStream. > + tokenStream->mShorthandPropertyID = eCSSProperty_UNKNOWN; Just assert that mShorthandPropertyID is eCSSProperty_UNKNOWN, as that's what it's initialized to, and update the comment s/setting ... to/leaving ... as/. ::: dom/animation/KeyframeUtils.cpp:818 (Diff revision 2) > + // in mTokenStream. > + tokenStream->mShorthandPropertyID = eCSSProperty_UNKNOWN; > + value.SetTokenStreamValue(tokenStream); > + } > + > + return { aProperty, value }; Nit: only one space after "return". ::: dom/animation/KeyframeUtils.cpp:1363 (Diff revision 2) > + } > + } > + > + aResult.SetCapacity(processedKeyframes.Count()); > + for (auto iter = processedKeyframes.Iter(); !iter.Done(); iter.Next()) { > + aResult.AppendElement(*iter.UserData()); Can we avoid making a copy of the Keyframe here? ::: dom/animation/KeyframeUtils.cpp:1426 (Diff revision 2) > + ? frame.mOffset.value() > + : computedOffset; > + > + for (const PropertyValuePair& pair : frame.mPropertyValues) { > + if (nsCSSProps::IsShorthand(pair.mProperty)) { > + MOZ_ASSERT(pair.mValue.GetUnit() == eCSSUnit_TokenStream); This MOZ_ASSERT isn't needed since the GetTokenStreamValue() call will do it for you. ::: dom/animation/KeyframeUtils.cpp:1437 (Diff revision 2) > + CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES( > + prop, pair.mProperty, nsCSSProps::eEnabledForAllContent) { > + addToPropertySets(*prop, offsetToUse); > + } > + } else { > + if (pair.mValue.GetUnit() == eCSSUnit_Null || In what cases will we get eCSSUnit_Null? Won't we have an eCSSUnit_TokenStream for any invalid or shorthand value? ::: dom/animation/KeyframeUtils.cpp:1457 (Diff revision 2) > +static bool > +ParseShorthand(nsCSSProperty aProperty, > + const nsAString& aString, > + nsIDocument* aDocument) > +{ Instead of this method can you just call nsCSSParser::IsValueValidForProperty?

Comment on attachment 8733257 [details] MozReview Request: Bug 1245748 - Use CSSAnimationBuilder::BuildAnimationFrames to set up CSS animations using Keyframe objects; r?heycam https://reviewboard.mozilla.org/r/41693/#review38379 ::: dom/animation/KeyframeUtils.cpp:940 (Diff revision 3) > ErrorResult& aRv) > { > + RefPtr<nsStyleContext> styleContext = > + LookupStyleContext(aTarget, aPseudoType); > + if (!styleContext) { > + aRv.Throw(NS_ERROR_DOM_ANIM_MISSING_PROPS_ERR); I don't think this is the right exception to throw, since it doesn't have anything to do with lack of support for additive animaiton. ::: dom/animation/KeyframeUtils.cpp:1151 (Diff revision 3) > MOZ_ASSERT(aValue.isObject()); > > + RefPtr<nsStyleContext> styleContext = > + LookupStyleContext(aTarget, aPseudoType); > + if (!styleContext) { > + aRv.Throw(NS_ERROR_DOM_ANIM_MISSING_PROPS_ERR); Here too.

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

Comment on attachment 8733259 [details] MozReview Request: Bug 1245748 - Split PropertyPriorityComparator into a separate (reusable) class; r=heycam https://reviewboard.mozilla.org/r/41697/#review38383 ::: dom/animation/KeyframeUtils.cpp (Diff revision 3) > - // Example orderings that result from this: > - // > - // margin-left, margin > - // > - // and: > - // > - // border-top-color, border-color, border-top, border > - // > - // This allows us to prioritize values specified by longhands (or smaller > - // shorthand subsets) when longhands and shorthands are both specified > - // on the one keyframe. Why remove this comment?

Comment on attachment 8733260 [details] MozReview Request: Bug 1245748 - Add PropertyPriorityIterator; r=heycam https://reviewboard.mozilla.org/r/41699/#review38385 ::: dom/animation/KeyframeUtils.cpp:183 (Diff revision 3) > + return mIndex != aOther.mIndex; > + } > + > + Iter& operator++() > + { > + MOZ_ASSERT(mIndex+1 <= mParent.mSortedPropertyIndices.Length(), Nit: spaces around "+". ::: dom/animation/KeyframeUtils.cpp:189 (Diff revision 3) > + "Should not seek past end iterator"); > + mIndex++; > + return *this; > + } > + > + const PropertyValuePair& operator* () Nit: no space before "()". ::: dom/animation/KeyframeUtils.cpp:192 (Diff revision 3) > + } > + > + const PropertyValuePair& operator* () > + { > + MOZ_ASSERT(mIndex < mParent.mSortedPropertyIndices.Length(), > + "Should not try to deference an end iterator"); dereference ::: dom/animation/KeyframeUtils.cpp:193 (Diff revision 3) > + MOZ_ASSERT(mParent.mSortedPropertyIndices[mIndex].mIndex > + < mParent.mProperties.Length(), > + "Should not try to deference outside bounds of properties" > + " array"); No need for this MOZ_ASSERT, as the nsTArray::operator[] will do it for you.

Comment on attachment 8733261 [details] MozReview Request: Bug 1245748 - Add GetAnimationPropertiesFromKeyframes; r=heycam https://reviewboard.mozilla.org/r/41701/#review38387 ::: dom/animation/KeyframeUtils.cpp:574 (Diff revision 3) > + nsTArray<PropertyStyleAnimationValuePair> values; > + > + // For shorthands, we store the string as a token stream so we need to > + // extract that first. > + if (nsCSSProps::IsShorthand(pair.mProperty)) { > + MOZ_ASSERT(pair.mValue.GetUnit() == eCSSUnit_TokenStream); As in a previous patch, this MOZ_ASSERT can be dropped. ::: dom/animation/KeyframeUtils.cpp:591 (Diff revision 3) > + // 'visibility' requires special handling that is unique to CSS > + // Transitions/CSS Animations/Web Animations (i.e. not SMIL) so we > + // apply that here. > + if (pair.mProperty == eCSSProperty_visibility) { > + MOZ_ASSERT(values[0].mValue.GetUnit() == > + StyleAnimationValue::eUnit_Enumerated, > + "unexpected unit"); > + values[0].mValue.SetIntValue(values[0].mValue.GetIntValue(), > + StyleAnimationValue::eUnit_Visibility); > + } > + } We should probably have a utility function that does this so that we can avoid duplicating the code from ExtractComputedValueForTransition. ::: dom/animation/KeyframeUtils.cpp:624 (Diff revision 3) > + } > + > + nsTArray<AnimationProperty> result; > + BuildSegmentsFromValueEntries(entries, result); > + > + return Move(result); As before, I think the Move() is not needed here.

Comment on attachment 8733262 [details] MozReview Request: Bug 1245748 - Add ApplyDistributeSpacing for Keyframe objects; r=heycam https://reviewboard.mozilla.org/r/41703/#review38389 r=me if you factor this out or think it it's too much trouble. ::: dom/animation/KeyframeUtils.cpp:547 (Diff revision 3) > +/* static */ void > +KeyframeUtils::ApplyDistributeSpacing(nsTArray<Keyframe>& aKeyframes) There is a fair amount of mostly-similar code duplication in this patch queue, and I wonder if we should make an effort to avoid that. For ApplyDistributeSpacing (and RequiresAdditiveAnimation in an earlier patch) I think we could do this relatively easily, as we're just either accessing mOffset or mKeyframeDict.mOffset (and mComputedOffset or mKeyframeDict.mOffset), depending on the type of element in the nsTArray. So making this a templated method that calls an overloaded method to return a reference to the relevant mOffset variable could work.

(In reply to Cameron McCormack (:heycam) (away Mar 25-28) from comment #69) > There is a fair amount of mostly-similar code duplication in this patch > queue, and I wonder if we should make an effort to avoid that. I see the existing functions disappear in the last patch; please ignore this.

Comment on attachment 8733263 [details] MozReview Request: Bug 1245748 - Use Keyframe-based utility functions when constructing KeyframeEffect(ReadOnly); r=heycam https://reviewboard.mozilla.org/r/41705/#review38391

Comment on attachment 8733264 [details] MozReview Request: Bug 1245748 - Remove no-longer-needed code for directly setting up properties in KeyframeEffect(ReadOnly) constructor; r=heycam https://reviewboard.mozilla.org/r/41707/#review38393

(In reply to Cameron McCormack (:heycam) (away Mar 25-28) from comment #63) > ::: dom/animation/KeyframeUtils.cpp:377 > (Diff revision 2) > > + > > + nsTArray<Keyframe> keyframes; > > + > > + if (!aFrames) { > > + // The argument was explicitly null meaning no kyeframes. > > + return Move(keyframes); > > Is the Move() necessary here? I think we can just rely on return value > optimization and |return keyframes;| here and at the rest of the return > statements in this method. (I'm not sure what effect using Move() here has > on RVO.) Yeah, I stepped through with a debugger and confirmed that Move() prevented a copy but that was on a debug build! So, yes, we don't need it. > ::: dom/animation/KeyframeUtils.cpp:809 > (Diff revision 2) > > + } > > + > > + if (value.GetUnit() == eCSSUnit_Null) { > > + // Either we have a shorthand, or we failed to parse a longhand. > > + // In either case, store the string value as a token stream. > > + nsCSSValueTokenStream* tokenStream = new nsCSSValueTokenStream; > > I just noticed mLineNumber/mLineOffset aren't initialized, since they're not > initialized in the nsCSSValueTokenStream constructor. Can you file a > followup to fix that? Filed bug 1258967. > ::: dom/animation/KeyframeUtils.cpp:1363 > (Diff revision 2) > > + } > > + } > > + > > + aResult.SetCapacity(processedKeyframes.Count()); > > + for (auto iter = processedKeyframes.Iter(); !iter.Done(); iter.Next()) { > > + aResult.AppendElement(*iter.UserData()); > > Can we avoid making a copy of the Keyframe here? We can use Move() but I have yet to confirm it actually saves us a copy here. Or did you have in mind a more radical restructuring of this algorithm to not use a hashtable? > ::: dom/animation/KeyframeUtils.cpp:1437 > (Diff revision 2) > > + CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES( > > + prop, pair.mProperty, nsCSSProps::eEnabledForAllContent) { > > + addToPropertySets(*prop, offsetToUse); > > + } > > + } else { > > + if (pair.mValue.GetUnit() == eCSSUnit_Null || > > In what cases will we get eCSSUnit_Null? Won't we have an > eCSSUnit_TokenStream for any invalid or shorthand value? Yes, that was leftover--I previously implemented this storing invalid values has having null units then went back and stored them as string and forgot to remove this part. > ::: dom/animation/KeyframeUtils.cpp:1457 > (Diff revision 2) > > +static bool > > +ParseShorthand(nsCSSProperty aProperty, > > + const nsAString& aString, > > + nsIDocument* aDocument) > > +{ > > Instead of this method can you just call > nsCSSParser::IsValueValidForProperty? Yes! > ::: dom/animation/KeyframeUtils.cpp:623 > (Diff revision 2) > > + if (!keyframeDict.mOffset.IsNull()) { > > + keyframe->mOffset.emplace(keyframeDict.mOffset.Value()); > > + } > > These are the same type? I think you can just do: > > keyframe->mOffset = keyframeDict.mOffset; |keyframe| is a Maybe<> and |keyframeDict| is a Nullable<> and unfortunately those types don't play together very well (despite one wrapping the other).

(In reply to Brian Birtles (:birtles) from comment #73) > > ::: dom/animation/KeyframeUtils.cpp:1363 > > (Diff revision 2) > > > + } > > > + } > > > + > > > + aResult.SetCapacity(processedKeyframes.Count()); > > > + for (auto iter = processedKeyframes.Iter(); !iter.Done(); iter.Next()) { > > > + aResult.AppendElement(*iter.UserData()); > > > > Can we avoid making a copy of the Keyframe here? > > We can use Move() but I have yet to confirm it actually saves us a copy > here. Or did you have in mind a more radical restructuring of this algorithm > to not use a hashtable? No, the hashtable is fine. Since we're effectively removing entries from processedKeyframes and adding them to aResult, it just seemed like we should be able to avoid duplicating the arrays inside of the Keyframe.

(In reply to Cameron McCormack (:heycam) (away Mar 25-28) from comment #64) > Comment on attachment 8733257 [details] > MozReview Request: Bug 1245748 - Add nsStyleContext parameter to > StyleAnimationValue::ComputeValue(s) > > https://reviewboard.mozilla.org/r/41693/#review38379 > > ::: dom/animation/KeyframeUtils.cpp:940 > (Diff revision 3) > > ErrorResult& aRv) > > { > > + RefPtr<nsStyleContext> styleContext = > > + LookupStyleContext(aTarget, aPseudoType); > > + if (!styleContext) { > > + aRv.Throw(NS_ERROR_DOM_ANIM_MISSING_PROPS_ERR); > > I don't think this is the right exception to throw, since it doesn't have > anything to do with lack of support for additive animaiton. Right. I was just trying to preserve the existing behavior since when I traced through the existing code that's the error we would arrive at in that scenario (since, from memory, we'd get a failure due when checking if we had a valid 0%/100% value). I'll switch it to NS_ERROR_FAILURE (although this code is deleted by the end of the patch series anyway).

(In reply to Cameron McCormack (:heycam) (away Mar 25-28) from comment #66) > Comment on attachment 8733259 [details] > MozReview Request: Bug 1245748 - Split PropertyPriorityComparator into a > separate (reusable) class > > https://reviewboard.mozilla.org/r/41697/#review38383 > > ::: dom/animation/KeyframeUtils.cpp > (Diff revision 3) > > - // Example orderings that result from this: > > - // > > - // margin-left, margin > > - // > > - // and: > > - // > > - // border-top-color, border-color, border-top, border > > - // > > - // This allows us to prioritize values specified by longhands (or smaller > > - // shorthand subsets) when longhands and shorthands are both specified > > - // on the one keyframe. > > Why remove this comment? I moved it to before PropertyPriorityComparator. I'll update the comment to say, "See description of PropertyPriorityComparator".

(In reply to Cameron McCormack (:heycam) (away Mar 25-28) from comment #68) > ::: dom/animation/KeyframeUtils.cpp:591 > (Diff revision 3) > > + // 'visibility' requires special handling that is unique to CSS > > + // Transitions/CSS Animations/Web Animations (i.e. not SMIL) so we > > + // apply that here. > > + if (pair.mProperty == eCSSProperty_visibility) { > > + MOZ_ASSERT(values[0].mValue.GetUnit() == > > + StyleAnimationValue::eUnit_Enumerated, > > + "unexpected unit"); > > + values[0].mValue.SetIntValue(values[0].mValue.GetIntValue(), > > + StyleAnimationValue::eUnit_Visibility); > > + } > > + } > > We should probably have a utility function that does this so that we can > avoid duplicating the code from ExtractComputedValueForTransition. Yeah, Hiro is going to add that in bug 1223658 (see bug 1223658 comment 20) -- I'll add a comment here about that.

Comment on attachment 8733250 [details] MozReview Request: Bug 1245748 - Add KeyframeEffectReadOnly::SetFrames; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41679/diff/1-2/

Comment on attachment 8733251 [details] MozReview Request: Bug 1245748 - Wrap lines in keyframe-effect/constructor.html to 80 chars; r=whitespace-only Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41681/diff/1-2/

Comment on attachment 8733252 [details] MozReview Request: Bug 1245748 - Update keyframe-effect/constructor.html to no longer refer to PropertyIndexedKeyframes or Keyframe; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41683/diff/2-3/

Comment on attachment 8733253 [details] MozReview Request: Bug 1245748 - Return the stored Keyframe objects from GetFrames, when available ; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41685/diff/2-3/

Comment on attachment 8733254 [details] MozReview Request: Bug 1245748 - Allow StyleAnimationValue::UncomputeValue to produce values whose storage is independent of the passed-in computed value; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41687/diff/2-3/

Comment on attachment 8733255 [details] MozReview Request: Bug 1245748 - Add an assignment operator to Keyframe that takes an rvalue reference; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41689/diff/2-3/

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

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

Comment on attachment 8733258 [details] MozReview Request: Bug 1245748 - 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/41695/diff/3-4/

Comment on attachment 8733259 [details] MozReview Request: Bug 1245748 - Split PropertyPriorityComparator into a separate (reusable) class; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41697/diff/3-4/

Comment on attachment 8733260 [details] MozReview Request: Bug 1245748 - Add PropertyPriorityIterator; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41699/diff/3-4/

Comment on attachment 8733261 [details] MozReview Request: Bug 1245748 - Add GetAnimationPropertiesFromKeyframes; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41701/diff/3-4/

Comment on attachment 8733262 [details] MozReview Request: Bug 1245748 - Add ApplyDistributeSpacing for Keyframe objects; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41703/diff/3-4/

Comment on attachment 8733263 [details] MozReview Request: Bug 1245748 - Use Keyframe-based utility functions when constructing KeyframeEffect(ReadOnly); r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41705/diff/3-4/

Comment on attachment 8733264 [details] MozReview Request: Bug 1245748 - Remove no-longer-needed code for directly setting up properties in KeyframeEffect(ReadOnly) constructor; r=heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41707/diff/3-4/

I have confirmed that by adding this, we end up calling SwapElements() on the mPropertyValues member when we build up the nsTArray<Keyframe> result in GetKeyframeListFromPropertyIndexedKeyframe. Without this explicit move constructor (i.e. with only the default move constructor) the copy-constructor for mPropertyValues is called. Review commit: https://reviewboard.mozilla.org/r/42137/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42137/

Comment on attachment 8734226 [details] MozReview Request: Bug 1245748 - Add a Move constructor to Keyframe https://reviewboard.mozilla.org/r/42137/#review38675

Thanks Cameron! https://treeherder.mozilla.org/#/jobs?repo=try&revision=e96f68ce39aa

Missed a spot when rebasing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4144a3ac4b14

https://hg.mozilla.org/integration/mozilla-inbound/rev/68c978a14bbc https://hg.mozilla.org/integration/mozilla-inbound/rev/b66be52a688f https://hg.mozilla.org/integration/mozilla-inbound/rev/b692d554f931 https://hg.mozilla.org/integration/mozilla-inbound/rev/e5b7f120513e https://hg.mozilla.org/integration/mozilla-inbound/rev/a2fc06886cf5 https://hg.mozilla.org/integration/mozilla-inbound/rev/3fd3083eaf4c https://hg.mozilla.org/integration/mozilla-inbound/rev/bb43b0b51db3 https://hg.mozilla.org/integration/mozilla-inbound/rev/20252fc7d044 https://hg.mozilla.org/integration/mozilla-inbound/rev/47f2d102202f https://hg.mozilla.org/integration/mozilla-inbound/rev/a292ea900280 https://hg.mozilla.org/integration/mozilla-inbound/rev/2d6642da5c50 https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea9096a60d2 https://hg.mozilla.org/integration/mozilla-inbound/rev/d2939b503235 https://hg.mozilla.org/integration/mozilla-inbound/rev/88795391fe8b https://hg.mozilla.org/integration/mozilla-inbound/rev/5dbdbe3e4655 https://hg.mozilla.org/integration/mozilla-inbound/rev/5081a75fbe8c

There are still more patches needed before this bug can be closed.

https://hg.mozilla.org/mozilla-central/rev/68c978a14bbc https://hg.mozilla.org/mozilla-central/rev/b66be52a688f https://hg.mozilla.org/mozilla-central/rev/b692d554f931 https://hg.mozilla.org/mozilla-central/rev/e5b7f120513e https://hg.mozilla.org/mozilla-central/rev/a2fc06886cf5 https://hg.mozilla.org/mozilla-central/rev/3fd3083eaf4c https://hg.mozilla.org/mozilla-central/rev/bb43b0b51db3 https://hg.mozilla.org/mozilla-central/rev/20252fc7d044 https://hg.mozilla.org/mozilla-central/rev/47f2d102202f https://hg.mozilla.org/mozilla-central/rev/a292ea900280 https://hg.mozilla.org/mozilla-central/rev/2d6642da5c50 https://hg.mozilla.org/mozilla-central/rev/0ea9096a60d2 https://hg.mozilla.org/mozilla-central/rev/d2939b503235 https://hg.mozilla.org/mozilla-central/rev/88795391fe8b https://hg.mozilla.org/mozilla-central/rev/5dbdbe3e4655 https://hg.mozilla.org/mozilla-central/rev/5081a75fbe8c

Comment on attachment 8733250 [details] MozReview Request: Bug 1245748 - Add KeyframeEffectReadOnly::SetFrames; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41679/diff/2-3/

Comment on attachment 8733251 [details] MozReview Request: Bug 1245748 - Wrap lines in keyframe-effect/constructor.html to 80 chars; r=whitespace-only Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41681/diff/2-3/

Comment on attachment 8733252 [details] MozReview Request: Bug 1245748 - Update keyframe-effect/constructor.html to no longer refer to PropertyIndexedKeyframes or Keyframe; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41683/diff/3-4/

Comment on attachment 8733253 [details] MozReview Request: Bug 1245748 - Return the stored Keyframe objects from GetFrames, when available ; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41685/diff/3-4/

Comment on attachment 8733254 [details] MozReview Request: Bug 1245748 - Allow StyleAnimationValue::UncomputeValue to produce values whose storage is independent of the passed-in computed value; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41687/diff/3-4/

Comment on attachment 8733255 [details] MozReview Request: Bug 1245748 - Add an assignment operator to Keyframe that takes an rvalue reference; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41689/diff/3-4/

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

Comment on attachment 8733257 [details] MozReview Request: Bug 1245748 - Use CSSAnimationBuilder::BuildAnimationFrames to set up CSS animations using Keyframe objects; r?heycam Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41693/diff/4-5/

Comment on attachment 8733258 [details] MozReview Request: Bug 1245748 - 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/41695/diff/4-5/

Oh dear, I guess MozReview can't cope with adding patches after some have already landed. I guess it's back to bzexport. Sorry for the spam.

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). MozReview-Commit-ID: 2ll26lsWZTm

MozReview-Commit-ID: FMg5uhxuZ6L

These types have been removed from the normative part of the Web Animations spec. MozReview-Commit-ID: LJkWvuDCT55

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. MozReview-Commit-ID: 4SpBRM7Ykyv

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"). MozReview-Commit-ID: 4RoCn39ntiJ

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). MozReview-Commit-ID: 5QmcIxkC4aB

We will call this method in the next patch in this series. MozReview-Commit-ID: E8QnGOIt91

MozReview-Commit-ID: BMLvYP8iIIa

MozReview-Commit-ID: JDzvQIxlsX6

Sorry for all the spam but I'm going to split off the CSS animation work into a separate bug--then I should be able to put the patches on MozReview.

All dependencies are now fixed.