Closed Bug 1245748 Opened 4 years ago Closed 4 years ago

Store specified values in KeyframeEffectReadOnly

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox47 --- affected

People

(Reporter: birtles, Assigned: birtles)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 27 obsolete files)

1.11 KB, text/html
Details
155.82 KB, patch
Details | Diff | Splinter Review
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.
See Also: → 1239889
Attached file Context test
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.
Blocks: 1245000
Blocks: 1247004
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?
Flags: needinfo?(cam)
(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?
Flags: needinfo?(cam) → needinfo?(bbirtles)
(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.
Flags: needinfo?(bbirtles)
Blocks: 1252599
No longer blocks: 1252599
Attached patch WIP patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Blocks: 1253470
Blocks: 1244591
Depends on: 1254419
Depends on: 1246320
Blocks: 1254424
Here's an updated exploratory patch. I'm going to start pulling pieces out of this to stick into dependent bugs.
Attachment #8726105 - Attachment is obsolete: true
Comment on attachment 8730179 [details] [diff] [review]
Merge inbound to m-c. a=merge

Wrong bug, I think.
Attachment #8730179 - Attachment is obsolete: true
Flags: needinfo?(jeremychen)
(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. =(
Flags: needinfo?(jeremychen)
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/
Attachment #8733250 - Flags: review?(cam)
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/
Attachment #8733251 - Flags: review?(cam)
Attachment #8733251 - Flags: review?(bzbarsky)
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/
Attachment #8733252 - Flags: review?(cam)
Attachment #8733252 - Flags: review?(bzbarsky)
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/
Attachment #8733254 - Flags: review?(cam)
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/
Attachment #8733257 - Flags: review?(cam)
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.
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.  :(
Attachment #8733251 - Flags: review?(bzbarsky)
Attachment #8733252 - Flags: review?(bzbarsky)
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
Attachment #8733252 - Flags: review+
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
Attachment #8733251 - Flags: review+
(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/
Attachment #8733252 - Attachment description: MozReview Request: Bug 1245748 - Update handling of 'composite' dictionary members to match changes to the spec → MozReview Request: Bug 1245748 - Update handling of 'composite' dictionary members to match changes to the spec; r?heycam, r=bz
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.
Attachment #8733250 - Flags: review?(cam) → review+
Attachment #8733251 - Flags: review?(cam) → review+
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?
Attachment #8733252 - Flags: review?(cam) → review+
Attachment #8733253 - Flags: review?(cam) → review+
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.
Attachment #8733254 - Flags: review?(cam) → review+
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.
Attachment #8733255 - Flags: review?(cam) → review+
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?
Attachment #8733256 - Flags: review?(cam) → review+
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.
Attachment #8733257 - Flags: review?(cam) → review+
Attachment #8733258 - Flags: review?(cam) → review+
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?
Attachment #8733259 - Flags: review?(cam) → review+
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.
Attachment #8733260 - Flags: review?(cam) → review+
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.
Attachment #8733261 - Flags: review?(cam) → review+
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.
Attachment #8733262 - Flags: review?(cam) → review+
(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
Attachment #8733263 - Flags: review?(cam) → review+
Attachment #8733264 - Flags: review?(cam) → review+
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/
Attachment #8733250 - Attachment description: MozReview Request: Bug 1245748 - Move ComputedTiming to a separate file → MozReview Request: Bug 1245748 - Move ComputedTiming to a separate file; r=heycam
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/
Attachment #8733251 - Attachment description: MozReview Request: Bug 1245748 - Rename Keyframe-related IDL types to match changes to Web Animations spec → MozReview Request: Bug 1245748 - Rename Keyframe-related IDL types to match changes to Web Animations spec; r=heycam
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/
Attachment #8733252 - Attachment description: MozReview Request: Bug 1245748 - Update handling of 'composite' dictionary members to match changes to the spec; r?heycam, r=bz → MozReview Request: Bug 1245748 - Update handling of 'composite' dictionary members to match changes to the spec; r=heycam, r=bz
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/
Attachment #8733253 - Attachment description: MozReview Request: Bug 1245748 - Define the Keyframe type for storing specified keyframes → MozReview Request: Bug 1245748 - Define the Keyframe type for storing specified keyframes; r=heycam
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/
Attachment #8733254 - Attachment description: MozReview Request: Bug 1245748 - Add missing includes to TimingParams.{cpp,h} → MozReview Request: Bug 1245748 - Add missing includes to TimingParams.{cpp,h}; r=heycam
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/
Attachment #8733255 - Attachment description: MozReview Request: Bug 1245748 - Move keyframe handling code to a separate KeyframeUtils class → MozReview Request: Bug 1245748 - Move keyframe handling code to a separate KeyframeUtils class; r=heycam
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/
Attachment #8733256 - Attachment description: MozReview Request: Bug 1245748 - Add KeyframeUtils::GetKeyframesFromObject → MozReview Request: Bug 1245748 - Add KeyframeUtils::GetKeyframesFromObject; r=heycam
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/
Attachment #8733257 - Attachment description: MozReview Request: Bug 1245748 - Add nsStyleContext parameter to StyleAnimationValue::ComputeValue(s) → MozReview Request: Bug 1245748 - Add nsStyleContext parameter to StyleAnimationValue::ComputeValue(s); r=heycam
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/
Attachment #8733258 - Attachment description: MozReview Request: Bug 1245748 - Add a variant of StyleAnimationValue::ComputeValues that takes an nsCSSValue → MozReview Request: Bug 1245748 - Add a variant of StyleAnimationValue::ComputeValues that takes an nsCSSValue; r=heycam
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/
Attachment #8733259 - Attachment description: MozReview Request: Bug 1245748 - Split PropertyPriorityComparator into a separate (reusable) class → MozReview Request: Bug 1245748 - Split PropertyPriorityComparator into a separate (reusable) class; r=heycam
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/
Attachment #8733260 - Attachment description: MozReview Request: Bug 1245748 - Add PropertyPriorityIterator → MozReview Request: Bug 1245748 - Add PropertyPriorityIterator; r=heycam
Attachment #8733261 - Attachment description: MozReview Request: Bug 1245748 - Add GetAnimationPropertiesFromKeyframes → MozReview Request: Bug 1245748 - Add GetAnimationPropertiesFromKeyframes; r=heycam
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/
Attachment #8733262 - Attachment description: MozReview Request: Bug 1245748 - Add ApplyDistributeSpacing for Keyframe objects → MozReview Request: Bug 1245748 - Add ApplyDistributeSpacing for Keyframe objects; r=heycam
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/
Attachment #8733263 - Attachment description: MozReview Request: Bug 1245748 - Use Keyframe-based utility functions when constructing KeyframeEffect(ReadOnly) → MozReview Request: Bug 1245748 - Use Keyframe-based utility functions when constructing KeyframeEffect(ReadOnly); r=heycam
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/
Attachment #8733264 - Attachment description: MozReview Request: Bug 1245748 - Remove no-longer-needed code for directly setting up properties in KeyframeEffect(ReadOnly) constructor → MozReview Request: Bug 1245748 - Remove no-longer-needed code for directly setting up properties in KeyframeEffect(ReadOnly) constructor; r=heycam
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/
Attachment #8734226 - Flags: review?(cam)
Attachment #8734226 - Flags: review?(cam) → review+
There are still more patches needed before this bug can be closed.
Keywords: leave-open
Duplicate of this bug: 1217256
Duplicate of this bug: 1234476
Duplicate of this bug: 1217252
Depends on: 1260572
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/
Attachment #8733250 - Attachment description: MozReview Request: Bug 1245748 - Move ComputedTiming to a separate file; r=heycam → MozReview Request: Bug 1245748 - Add KeyframeEffectReadOnly::SetFrames; r?heycam
Attachment #8733251 - Attachment description: MozReview Request: Bug 1245748 - Rename Keyframe-related IDL types to match changes to Web Animations spec; r=heycam → MozReview Request: Bug 1245748 - Wrap lines in keyframe-effect/constructor.html to 80 chars; r=whitespace-only
Attachment #8733252 - Attachment description: MozReview Request: Bug 1245748 - Update handling of 'composite' dictionary members to match changes to the spec; r=heycam, r=bz → MozReview Request: Bug 1245748 - Update keyframe-effect/constructor.html to no longer refer to PropertyIndexedKeyframes or Keyframe; r?heycam
Attachment #8733253 - Attachment description: MozReview Request: Bug 1245748 - Define the Keyframe type for storing specified keyframes; r=heycam → MozReview Request: Bug 1245748 - Return the stored Keyframe objects from GetFrames, when available ; r?heycam
Attachment #8733254 - Attachment description: MozReview Request: Bug 1245748 - Add missing includes to TimingParams.{cpp,h}; r=heycam → MozReview Request: Bug 1245748 - Allow StyleAnimationValue::UncomputeValue to produce values whose storage is independent of the passed-in computed value; r?heycam
Attachment #8733255 - Attachment description: MozReview Request: Bug 1245748 - Move keyframe handling code to a separate KeyframeUtils class; r=heycam → MozReview Request: Bug 1245748 - Add an assignment operator to Keyframe that takes an rvalue reference; r?heycam
Attachment #8733256 - Attachment description: MozReview Request: Bug 1245748 - Add KeyframeUtils::GetKeyframesFromObject; r=heycam → MozReview Request: Bug 1245748 - Add methods to CSSAnimationBuilder to build a set of Keyframe objects; r?heycam
Attachment #8733257 - Attachment description: MozReview Request: Bug 1245748 - Add nsStyleContext parameter to StyleAnimationValue::ComputeValue(s); r=heycam → MozReview Request: Bug 1245748 - Use CSSAnimationBuilder::BuildAnimationFrames to set up CSS animations using Keyframe objects; r?heycam
Attachment #8733258 - Attachment description: MozReview Request: Bug 1245748 - Add a variant of StyleAnimationValue::ComputeValues that takes an nsCSSValue; r=heycam → MozReview Request: Bug 1245748 - Drop some no-longer-needed code for setting up CSS animations using AnimationProperty objects; r?heycam
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/
Attachment #8733259 - Attachment is obsolete: true
Attachment #8733260 - Attachment is obsolete: true
Attachment #8733261 - Attachment is obsolete: true
Attachment #8733262 - Attachment is obsolete: true
Attachment #8733263 - Attachment is obsolete: true
Attachment #8733264 - Attachment is obsolete: true
Attachment #8734226 - Attachment is obsolete: true
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
Attachment #8736147 - Flags: review?(cam)
MozReview-Commit-ID: FMg5uhxuZ6L
Attachment #8736148 - Flags: review?(cam)
These types have been removed from the normative part of the Web Animations
spec.

MozReview-Commit-ID: LJkWvuDCT55
Attachment #8736149 - Flags: review?(cam)
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
Attachment #8736150 - Flags: review?(cam)
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
Attachment #8736151 - Flags: review?(cam)
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
Attachment #8736152 - Flags: review?(cam)
We will call this method in the next patch in this series.

MozReview-Commit-ID: E8QnGOIt91
Attachment #8736153 - Flags: review?(cam)
MozReview-Commit-ID: BMLvYP8iIIa
Attachment #8736154 - Flags: review?(cam)
MozReview-Commit-ID: JDzvQIxlsX6
Attachment #8736155 - Flags: review?(cam)
Attachment #8733250 - Attachment is obsolete: true
Attachment #8733251 - Attachment is obsolete: true
Attachment #8733252 - Attachment is obsolete: true
Attachment #8733253 - Attachment is obsolete: true
Attachment #8733254 - Attachment is obsolete: true
Attachment #8733255 - Attachment is obsolete: true
Attachment #8733256 - Attachment is obsolete: true
Attachment #8733257 - Attachment is obsolete: true
Attachment #8733258 - Attachment is obsolete: true
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.
Depends on: 1260655
Attachment #8736147 - Attachment is obsolete: true
Attachment #8736147 - Flags: review?(cam)
Attachment #8736148 - Attachment is obsolete: true
Attachment #8736148 - Flags: review?(cam)
Attachment #8736149 - Attachment is obsolete: true
Attachment #8736149 - Flags: review?(cam)
Attachment #8736150 - Attachment is obsolete: true
Attachment #8736150 - Flags: review?(cam)
Attachment #8736151 - Attachment is obsolete: true
Attachment #8736151 - Flags: review?(cam)
Attachment #8736152 - Attachment is obsolete: true
Attachment #8736152 - Flags: review?(cam)
Attachment #8736153 - Attachment is obsolete: true
Attachment #8736153 - Flags: review?(cam)
Attachment #8736154 - Attachment is obsolete: true
Attachment #8736154 - Flags: review?(cam)
Attachment #8736155 - Attachment is obsolete: true
Attachment #8736155 - Flags: review?(cam)
Depends on: 1260976
Depends on: 1260983
All dependencies are now fixed.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Depends on: 1276688
You need to log in before you can comment on or make changes to this bug.