Closed Bug 1304922 Opened 3 years ago Closed 3 years ago

Replace mWinsInCascade with another flag that EffectSet has

Categories

(Core :: DOM: Animation, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Depends on 1 open bug)

Details

Attachments

(12 files)

58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
Currently each AnimationProperty in each KeyframeEffectReadOnly has the mWinsInCascade, that represents only one property wins.  But in case of effect composition many of effects has to be composed even if the effect is lower animation priority.

So, I'd propose to use a hashtable, holding by EffectSet, which has the information which cascade level wins for each CSS property like this;

  enum class WinnerCascadeLevel {                                              
    None,            // There are animations but overridden by important rules.
                     // NOTE: This is meaningful only for opacity and transform
                     // property.                                              
    TransitionsOnly, // There are only Transition level animations.            
    AnimationsOnly,  // There are only Animations level animations.            
    Both,            // Both of Transitions and Animations. This is used for   
                     // effect composition, e.g. additive animation effect onto
                     // transition effect.                                     
  };                                                                           

  nsDataHashtable<nsUint32HashKey, WinnerCascadeLevel> mWinnerInCascade;

The nsDataHashTable is not so handy for nsCSSPropertyID so a struct inheriting the ns
DataHashTable would be better (I did it already locally) and should have some methods.
I should note that, as a result of this change, lower priority animations are also sent to the compositor.
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Priority: -- → P3
(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
>   enum class WinnerCascadeLevel {                                           
> 
>     None,            // There are animations but overridden by important
> rules.
>                      // NOTE: This is meaningful only for opacity and
> transform
>                      // property.                                           
> 
>     TransitionsOnly, // There are only Transition level animations.         
> 
>     AnimationsOnly,  // There are only Animations level animations.         
> 
>     Both,            // Both of Transitions and Animations. This is used for
> 
>                      // effect composition, e.g. additive animation effect
> onto
>                      // transition effect.                                  
> 
>   };                                                                        

I haven't really thought this through, but I'm surprised we'd need a 'Both' value. For a given property, we should never put a value in *both* the transitions level and animations level. I'm probably misunderstanding how this enum is used, however.
(In reply to Brian Birtles (:birtles) from comment #3)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> >   enum class WinnerCascadeLevel {                                           
> > 
> >     None,            // There are animations but overridden by important
> > rules.
> >                      // NOTE: This is meaningful only for opacity and
> > transform
> >                      // property.                                           
> > 
> >     TransitionsOnly, // There are only Transition level animations.         
> > 
> >     AnimationsOnly,  // There are only Animations level animations.         
> > 
> >     Both,            // Both of Transitions and Animations. This is used for
> > 
> >                      // effect composition, e.g. additive animation effect
> > onto
> >                      // transition effect.                                  
> > 
> >   };                                                                        
> 
> I haven't really thought this through, but I'm surprised we'd need a 'Both'
> value. For a given property, we should never put a value in *both* the
> transitions level and animations level. I'm probably misunderstanding how
> this enum is used, however.

I think in the below case we need to add the animation value to the transition value. The value is 200px at 1s. No?

  target.style.transition = "margin-left 1s";
  target.style.setProperty("margin-left", "100px", "");

  target.animate({ marginLeft: [ "0px", "100px" ] }, { duration: 1000, composite: 'add');
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> I think in the below case we need to add the animation value to the
> transition value. The value is 200px at 1s. No?
> 
>   target.style.transition = "margin-left 1s";
>   target.style.setProperty("margin-left", "100px", "");
> 
>   target.animate({ marginLeft: [ "0px", "100px" ] }, { duration: 1000,
> composite: 'add');

Right, but we only put a value in the animations level of the cascade. We don't put anything in the transitions level of the cascade.
(In reply to Brian Birtles (:birtles) from comment #5)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> > I think in the below case we need to add the animation value to the
> > transition value. The value is 200px at 1s. No?
> > 
> >   target.style.transition = "margin-left 1s";
> >   target.style.setProperty("margin-left", "100px", "");
> > 
> >   target.animate({ marginLeft: [ "0px", "100px" ] }, { duration: 1000,
> > composite: 'add');
> 
> Right, but we only put a value in the animations level of the cascade. We
> don't put anything in the transitions level of the cascade.

Ah, I see.  The 'Both' will be used in EffectCompositor::ComposeAnimationRule() to compose transition effect just right before composing animation effect something like this;

 if (aCascadeLevel == Animations && the 'Both') { // We could also check the lowest animation has 'add' or 'accumulate'?
    for (KeyframeEffectReadOnly* transitionEffect : transitionEffects) {
      transitionEffect->GetANimation()->ComposeStyle(animationRule);
    }
 }

So, IIUC, nothing will be put into the transition level cascade.

Or do we call ComposeStyle() for all *animations* regardless of cascade level?  If so, we don't need the 'Both'.  Is this exactly what you have in mind, Brian?
Comment on attachment 8794644 [details]
Bug 1304922 - Part 1: Rename nsLayoutUtils::HasCurrentAnimationOfProperty() to nsLayoutUtils::HasActiveAnimationOfProperty().

https://reviewboard.mozilla.org/r/80976/#review79884

::: layout/base/nsLayoutUtils.h:2236
(Diff revision 1)
> -   * Returns true if the frame has current (i.e. running or scheduled-to-run)
> -   * animations or transitions for the property.
> +   * Returns true if the frame has active (i.e. running) animations or
> +   * transitions for the property.

The term 'active' here is confusing since it could be taken to mean an animation in its 'active phase' but I believe this also includes animations in the before phase that have a backwards fill, right?

So, perhaps we should say:

'Returns true if the frame has animations or transitions that are running or filling forwards for the specified property.'

?

::: layout/base/nsLayoutUtils.cpp:484
(Diff revision 1)
> -      return aEffect.IsCurrent() && aEffect.HasAnimationOfProperty(aProperty);
> +      return aEffect.IsCurrent() && aEffect.IsInEffect() &&
> +        aEffect.HasAnimationOfProperty(aProperty);

I think preserving the existing behavior is fine, but it seems like we should be layerizing animations in their delay phase even if they don't have a backwards fill.

For example, if I load this jsbin:

  http://jsbin.com/niborugubo/edit?html,css,output

And set layers.draw-borders to true, I notice we don't layerize the div until it starts animating.

That seems very undesirable. I suspect we used to layerize such content but somewhere during our refactoring (perhaps when we introduced mWinsInCascade?) we broke that.

We should file a bug to fix that which will possibly mean changing this function back to HasCurrentAnimationOfPrroperty or having two versions of this function (HasCurrent... and HasActive...) if it turns out there are call sites that need the HasActive... behavior.
Attachment #8794644 - Flags: review?(bbirtles) → review+
Comment on attachment 8794645 [details]
Bug 1304922 - Part 2: Sort animation array before sending animations to the compositor.

https://reviewboard.mozilla.org/r/80978/#review79890
Attachment #8794645 - Flags: review?(bbirtles) → review+
Comment on attachment 8794646 [details]
Bug 1304922 - Part 3: Request restyle for layer when CSS animation's index is changed.

https://reviewboard.mozilla.org/r/80980/#review79894
Attachment #8794646 - Flags: review?(bbirtles) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> (In reply to Brian Birtles (:birtles) from comment #5)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> > > I think in the below case we need to add the animation value to the
> > > transition value. The value is 200px at 1s. No?
> > > 
> > >   target.style.transition = "margin-left 1s";
> > >   target.style.setProperty("margin-left", "100px", "");
> > > 
> > >   target.animate({ marginLeft: [ "0px", "100px" ] }, { duration: 1000,
> > > composite: 'add');
> > 
> > Right, but we only put a value in the animations level of the cascade. We
> > don't put anything in the transitions level of the cascade.
> 
> Ah, I see.  The 'Both' will be used in
> EffectCompositor::ComposeAnimationRule() to compose transition effect just
> right before composing animation effect something like this;
> 
>  if (aCascadeLevel == Animations && the 'Both') { // We could also check the
> lowest animation has 'add' or 'accumulate'?
>     for (KeyframeEffectReadOnly* transitionEffect : transitionEffects) {
>       transitionEffect->GetANimation()->ComposeStyle(animationRule);
>     }
>  }
> 
> So, IIUC, nothing will be put into the transition level cascade.
> 
> Or do we call ComposeStyle() for all *animations* regardless of cascade
> level?  If so, we don't need the 'Both'.  Is this exactly what you have in
> mind, Brian?

I guess I was confused because we talk about the 'winner' but there is only ever one winner so 'both' doesn't make sense.

I don't think we need to treat transitions/animations differently when composing. It's just one stack.

I think we just need an enum that says for each property which level it is applied to: animations / transitions / none. Should we just extend EffectCompositor::CascadeLevel with a None value? Then call the member mOutputLevel? mResultLevel?

Then on a call to ComposeAnimationRule we do:

  aCascadeLevel = transitions:
    mResultLevel = animations/none -> skip
                   transitions -> compose all
  aCascadeLevel = animations:
    mResultLevel = transitions/none -> skip
                   animations -> compose all

Alternatively, instead of setting up mOutputLevel/mWinningLevel/mResultLevel, could we just set up two nsCSSPropertyIDSet objects when we calculate the cascade and pass them in as |aSetProperties| ?

e.g. nsCSSPropertyIDSet mTransitionsPropertyMask -- contains all the properties for which we have an animation that applies at the animations level. Then when we go to compose the transitions rule we will skip adding values for any of these properties, regardless of whether the animation targets the animations level or transitions level. However, we will still compose transitions when we compose the animation rule, since in that case we will use mAnimationsPropertyMask which only has the properties set that are overridden by important rules.

Does that work? I have to think about this some more, but if it does it seems simpler. We wouldn't need WinnerCascadeLevels at all since we'd track the same information using two nsCSSPropertyIDSet objects instead. Furthermore, we might not need to touch Animation::ComposeStyle since it already has the |aSetProperties| parameter.
(In reply to Brian Birtles (:birtles) from comment #17)

> Alternatively, instead of setting up
> mOutputLevel/mWinningLevel/mResultLevel, could we just set up two
> nsCSSPropertyIDSet objects when we calculate the cascade and pass them in as
> |aSetProperties| ?
> 
> e.g. nsCSSPropertyIDSet mTransitionsPropertyMask -- contains all the
> properties for which we have an animation that applies at the animations
> level. Then when we go to compose the transitions rule we will skip adding
> values for any of these properties, regardless of whether the animation
> targets the animations level or transitions level. However, we will still
> compose transitions when we compose the animation rule, since in that case
> we will use mAnimationsPropertyMask which only has the properties set that
> are overridden by important rules.
> 
> Does that work? I have to think about this some more, but if it does it
> seems simpler. We wouldn't need WinnerCascadeLevels at all since we'd track
> the same information using two nsCSSPropertyIDSet objects instead.
> Furthermore, we might not need to touch Animation::ComposeStyle since it
> already has the |aSetProperties| parameter.

That will work.  Actually I am trying it now, the most cumbersome point is that we have to drop mWinsInCascade at the same time because we need to reverse the iteration order in UpdateCascadeResult() to maintain the property set correctly.  (We also need to reverse the iteration order in ComposeAnimationRule()).  I am still in the middle way, but I guess the patch will be bigger, if you are OK with it, (I guess you are OK because it's simplified!), I am really happy to do it. Thanks!  Anyway I will post the patch later.
Thank you!

I was thinking about the names for the two nsCSSPropertyIDSet objects and I think maybe something like:

* mPropertiesWithImportantRules -- i.e. the properties we should skip when composing the animations rule
* mPropertiesForAnimationsLevel -- i.e. the properties we should skip when composing the transitions rule

Conversely, when we compose the animations rule, perhaps we should skip any properties *not* in mPropertiesForAnimationsLevel so that we skip transitions that we're not adding on to?

(I guess when we come to find animations for the compositor we'd need to be a bit more clever, e.g. if the property is in both sets, we'd skip sending anything for that property; and until we send the whole stack of effects to the compositor, we'd skip sending transitions if the property is in mPropertiesForAnimationsLevel.)
Comment on attachment 8794647 [details]
Bug 1304922 - Part 4: Add EffectSet::Count().

https://reviewboard.mozilla.org/r/80982/#review80134

I'm curious about why we want to do this. The original design was that EffectSet is basically just a dumb data structure (with convenience accessors) and all the logic for compositing goes in EffectCompositor. In this patch we're moving some bits of that logic to EffectSet. That's ok if we decided the original design doesn't work any more, but I'd like to know why this is better.
(In reply to Brian Birtles (:birtles) from comment #20)
> Comment on attachment 8794647 [details]
> Bug 1304922 - Part 4: Move GetOverriddenProperties() and
> UpdateCascadeResults() from EffectCompositor to EffectSet class.
> 
> https://reviewboard.mozilla.org/r/80982/#review80134
> 
> I'm curious about why we want to do this. The original design was that
> EffectSet is basically just a dumb data structure (with convenience
> accessors) and all the logic for compositing goes in EffectCompositor. In
> this patch we're moving some bits of that logic to EffectSet. That's ok if
> we decided the original design doesn't work any more, but I'd like to know
> why this is better.

Because I'd want to hide the logic specific for each (pseudo-)element.  
I think this intention is related to your comment in comment 19.

I think both of mPropertiesWithImportantRules and mPropertiesForAnimationsLevel are really nice and easy to understand.  I guess you are going to expose both of them, but I did hide them inside EffectSet class locally so they are currently EnumeratedArray.  

(In reply to Brian Birtles (:birtles) from comment #19)
> Conversely, when we compose the animations rule, perhaps we should skip any
> properties *not* in mPropertiesForAnimationsLevel so that we skip
> transitions that we're not adding on to?

I am now thinking about this part. It will be easy to understand if we expose this name in ComposeStyle() or not.  I will write both ways anyway, and then I will see which one looks better.

> (I guess when we come to find animations for the compositor we'd need to be
> a bit more clever, e.g. if the property is in both sets, we'd skip sending
> anything for that property; and until we send the whole stack of effects to
> the compositor, we'd skip sending transitions if the property is in
> mPropertiesForAnimationsLevel.)

The first part sounds really nice to me, but the second one is not clear to me that it's efficient or not. I guess you are supposed to sort all of animations before iterating it in FindAnimationsForCompositor()?  It will work but we need some tweaks in the case when FindAnimationsForCompositor() is called from HasAnimationForCompositor().

Anyway, thanks for really nice suggestion!
(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> I think both of mPropertiesWithImportantRules and
> mPropertiesForAnimationsLevel are really nice and easy to understand.  I
> guess you are going to expose both of them, but I did hide them inside
> EffectSet class locally so they are currently EnumeratedArray.  
> 
> (In reply to Brian Birtles (:birtles) from comment #19)
> > Conversely, when we compose the animations rule, perhaps we should skip any
> > properties *not* in mPropertiesForAnimationsLevel so that we skip
> > transitions that we're not adding on to?
> 
> I am now thinking about this part. It will be easy to understand if we
> expose this name in ComposeStyle() or not.

Oops. Sorry, I meant COmposeAnimationRule().
Here is a try with the patch using EnumeratedArray.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=322b0a8fecae

I am now writing another patch using mPropertiesWithImportantRules and mPropertiesForAnimationsLevel suggested in comment 19.  But unfortunately there is one point which will be confusable.  As you can see the patch in the try, we are setting the  property mask for transition level only if there are transitions and animations [1].

[1] https://hg.mozilla.org/try/rev/ed1a0c2956fc4724d6cc39620630830947744ce2#l4.77 

The check is really important to detect we need to request restyle or not.  On the other hand in case of mPropertiesForAnimationsLevel, it looks that it's set regardless of whether there is transition or not.  Actually I did drop the check inadvertently while rewriting the code.  As a result, what happened?  A test case[2] in test_restyle.html failed.

[2] http://hg.mozilla.org/mozilla-central/file/66a77b9bfe5d/dom/animation/test/chrome/test_restyles.html#l587

It took me half a day to know what exactly caused the failure.  Without the check we requested restyle for transition level because I used mPropertiesForAnimationsLevel to check whether we need to request restyle for transition level.  Apparently it was wrong.  To avoid this failure we need one more variable to store there is transitions or not.  Or mPropertiesForAnimationsLevel should be renamed to a proper name representing it is properties for *transitions* but suppressed.

Now I am inclined to use EnumeratedArray and name it mMaskedProperties (actually I named mPropertyMask in the patch though) because we don't need to name them respectively. ;-)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> It took me half a day to know what exactly caused the failure.  Without the
> check we requested restyle for transition level because I used
> mPropertiesForAnimationsLevel to check whether we need to request restyle
> for transition level.

So I guess the code was checking that if a property is in mPropertiesForAnimationLevel that we *don't* need to update the transitions rule for that property. Is that correct?
(In reply to Brian Birtles (:birtles) from comment #24)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> > It took me half a day to know what exactly caused the failure.  Without the
> > check we requested restyle for transition level because I used
> > mPropertiesForAnimationsLevel to check whether we need to request restyle
> > for transition level.
> 
> So I guess the code was checking that if a property is in
> mPropertiesForAnimationLevel that we *don't* need to update the transitions
> rule for that property. Is that correct?

Mostly correct.  I did not check the transitions rule was updated, maybe it did not, i guess it's bailed out somewhere. We just requested restyle for transitions level.  In the failure test case in test_restyles.html, we got eRestyle_CSSTransitions, there is no transitions in the test case.
Duplicate of this bug: 1303233
Blocks: 1305325
(In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> (In reply to Brian Birtles (:birtles) from comment #24)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> > > It took me half a day to know what exactly caused the failure.  Without the
> > > check we requested restyle for transition level because I used
> > > mPropertiesForAnimationsLevel to check whether we need to request restyle
> > > for transition level.
> > 
> > So I guess the code was checking that if a property is in
> > mPropertiesForAnimationLevel that we *don't* need to update the transitions
> > rule for that property. Is that correct?
> 
> Mostly correct.  I did not check the transitions rule was updated, maybe it
> did not, i guess it's bailed out somewhere. We just requested restyle for
> transitions level.  In the failure test case in test_restyles.html, we got
> eRestyle_CSSTransitions, there is no transitions in the test case.

I don't understand why making mPropertiesForAnimationLevel only include properties where we have a transition (that is a *more* restricted set) would cause us to trigger *less* restyles if we're testing for the *absence* of a property in that set.

I hope we can avoid using enumerated arrays and masked properties and have some more easily understandable concepts.
(In reply to Brian Birtles (:birtles) from comment #27)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> > (In reply to Brian Birtles (:birtles) from comment #24)
> > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> > > > It took me half a day to know what exactly caused the failure.  Without the
> > > > check we requested restyle for transition level because I used
> > > > mPropertiesForAnimationsLevel to check whether we need to request restyle
> > > > for transition level.
> > > 
> > > So I guess the code was checking that if a property is in
> > > mPropertiesForAnimationLevel that we *don't* need to update the transitions
> > > rule for that property. Is that correct?
> > 
> > Mostly correct.  I did not check the transitions rule was updated, maybe it
> > did not, i guess it's bailed out somewhere. We just requested restyle for
> > transitions level.  In the failure test case in test_restyles.html, we got
> > eRestyle_CSSTransitions, there is no transitions in the test case.
> 
> I don't understand why making mPropertiesForAnimationLevel only include
> properties where we have a transition (that is a *more* restricted set)
> would cause us to trigger *less* restyles if we're testing for the *absence*
> of a property in that set.

Yes, right.  An error-prone point here is the name, mPropertiesForAnimationLevel.  If we use the name, then I am proposing we need another variable (nsCSSPropertyIDSet I guess).

Here is an example case we need the additional variable.

1) start a transition on an element
2) start an animation on the same element while running the transition

In the above case, we need to request style for both of cascade levels.  Especially it's important to request for transitions level, otherwise, we will get a wrong style by getComputedStyle().  There is no important rule, so mPropertiesForAnimationLevel is not changed at all.  So we miss the request restyle for the transitions level.

For now, I have no other idea to solve this situation if mPropertiesForAnimationLevel includes properties in the case when there is no transition.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> (In reply to Brian Birtles (:birtles) from comment #27)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> > > (In reply to Brian Birtles (:birtles) from comment #24)
> > > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> > > > > It took me half a day to know what exactly caused the failure.  Without the
> > > > > check we requested restyle for transition level because I used
> > > > > mPropertiesForAnimationsLevel to check whether we need to request restyle
> > > > > for transition level.
> > > > 
> > > > So I guess the code was checking that if a property is in
> > > > mPropertiesForAnimationLevel that we *don't* need to update the transitions
> > > > rule for that property. Is that correct?
> > > 
> > > Mostly correct.  I did not check the transitions rule was updated, maybe it
> > > did not, i guess it's bailed out somewhere. We just requested restyle for
> > > transitions level.  In the failure test case in test_restyles.html, we got
> > > eRestyle_CSSTransitions, there is no transitions in the test case.
> > 
> > I don't understand why making mPropertiesForAnimationLevel only include
> > properties where we have a transition (that is a *more* restricted set)
> > would cause us to trigger *less* restyles if we're testing for the *absence*
> > of a property in that set.
> 
> Yes, right.  An error-prone point here is the name,
> mPropertiesForAnimationLevel.  If we use the name, then I am proposing we
> need another variable (nsCSSPropertyIDSet I guess).
> 
> Here is an example case we need the additional variable.
> 
> 1) start a transition on an element
> 2) start an animation on the same element while running the transition
> 
> In the above case, we need to request style for both of cascade levels. 
> Especially it's important to request for transitions level, otherwise, we
> will get a wrong style by getComputedStyle().  There is no important rule,
> so mPropertiesForAnimationLevel is not changed at all.

Oops, sorry. the mPropertiesForAnimationLevel is changed. This is not the case.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> Yes, right.  An error-prone point here is the name,
> mPropertiesForAnimationLevel.  If we use the name, then I am proposing we
> need another variable (nsCSSPropertyIDSet I guess).
> 
> Here is an example case we need the additional variable.
> 
> 1) start a transition on an element
> 2) start an animation on the same element while running the transition
> 
> In the above case, we need to request style for both of cascade levels. 
> Especially it's important to request for transitions level, otherwise, we
> will get a wrong style by getComputedStyle().  There is no important rule,
> so mPropertiesForAnimationLevel is not changed at all.  So we miss the
> request restyle for the transitions level.

So the problem is we fail to remove the transitions from the transitions rule in this case? So we need to know that we had a transition so that when we update the cascade we request the restyle on the transitions rule? Is that right?

If that's right, can't we detect that as (a) the first animation in the stack is a transition, and (b) the property is not already in mPropertiesForAnimationLevel?
(In reply to Brian Birtles (:birtles) from comment #30)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> > Yes, right.  An error-prone point here is the name,
> > mPropertiesForAnimationLevel.  If we use the name, then I am proposing we
> > need another variable (nsCSSPropertyIDSet I guess).
> > 
> > Here is an example case we need the additional variable.
> > 
> > 1) start a transition on an element
> > 2) start an animation on the same element while running the transition
> > 
> > In the above case, we need to request style for both of cascade levels. 
> > Especially it's important to request for transitions level, otherwise, we
> > will get a wrong style by getComputedStyle().  There is no important rule,
> > so mPropertiesForAnimationLevel is not changed at all.  So we miss the
> > request restyle for the transitions level.
> 
> So the problem is we fail to remove the transitions from the transitions
> rule in this case? So we need to know that we had a transition so that when
> we update the cascade we request the restyle on the transitions rule? Is
> that right?
> 
> If that's right, can't we detect that as (a) the first animation in the
> stack is a transition, and (b) the property is not already in
> mPropertiesForAnimationLevel?

Oh! I am an idiot!  I think we can move the transitions check[1] just right before RequestRestyle for transitions.

[1] https://hg.mozilla.org/try/rev/ed1a0c2956fc4724d6cc39620630830947744ce2#l4.77
(In reply to Hiroyuki Ikezoe (:hiro) from comment #31)
> (In reply to Brian Birtles (:birtles) from comment #30)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> > > Yes, right.  An error-prone point here is the name,
> > > mPropertiesForAnimationLevel.  If we use the name, then I am proposing we
> > > need another variable (nsCSSPropertyIDSet I guess).
> > > 
> > > Here is an example case we need the additional variable.
> > > 
> > > 1) start a transition on an element
> > > 2) start an animation on the same element while running the transition
> > > 
> > > In the above case, we need to request style for both of cascade levels. 
> > > Especially it's important to request for transitions level, otherwise, we
> > > will get a wrong style by getComputedStyle().  There is no important rule,
> > > so mPropertiesForAnimationLevel is not changed at all.  So we miss the
> > > request restyle for the transitions level.
> > 
> > So the problem is we fail to remove the transitions from the transitions
> > rule in this case? So we need to know that we had a transition so that when
> > we update the cascade we request the restyle on the transitions rule? Is
> > that right?
> > 
> > If that's right, can't we detect that as (a) the first animation in the
> > stack is a transition, and (b) the property is not already in
> > mPropertiesForAnimationLevel?
> 
> Oh! I am an idiot!  I think we can move the transitions check[1] just right
> before RequestRestyle for transitions.

Unfortunately this did not work yet. All of test cases that check initial value of transtion failed.
An example is;
http://hg.mozilla.org/mozilla-central/file/b1d60f2f68c7/layout/style/test/test_animations.html#l2033

It seems that transition relies on RequestRestyle in UpdateCascadeResults().  I don't know why but we need to fix it first.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #32)

> > Oh! I am an idiot!  I think we can move the transitions check[1] just right
> > before RequestRestyle for transitions.
> 
> Unfortunately this did not work yet. All of test cases that check initial
> value of transtion failed.
> An example is;
> http://hg.mozilla.org/mozilla-central/file/b1d60f2f68c7/layout/style/test/
> test_animations.html#l2033
> 
> It seems that transition relies on RequestRestyle in UpdateCascadeResults().
> I don't know why but we need to fix it first.

I made a mistaked.It works!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69f46ac09fc4
(In reply to Hiroyuki Ikezoe (:hiro) from comment #33)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=69f46ac09fc4

> +  // If multiple animations affect the same property, animations with higher
> +  // composite order (priority) override or add or animations with lower
> +  // priority except properties in skipProperties.
> +  // NOTE: PropertiesWithImportantRules has only properties which can be
> +  // run on the compositor.  In case of other properties, restyling process
> +  // on the main-thread overrides this composition results.
> +  for (KeyframeEffectReadOnly* effect : sortedEffectList) {
> +    const nsCSSPropertyIDSet& skipProperties =
> +      effect->GetAnimation()->CascadeLevel() == CascadeLevel::Animations
> +        ? effects->PropertiesWithImportantRules()
> +        : effects->PropertiesForAnimationsLevel();
> +    effect->GetAnimation()->ComposeStyle(animationRule, skipProperties);
>    }

I expected we'd use |aCascadeLevel| to determine which set of properties to skip? Not the cascade level of the animation?

E.g. for a transition, when composing the transitions rule, we'd skip it depending on the value of mPropertiesForAnimationsLevel; but when composing the transitions rule, we'd skip it depending it depend on the value of mPropertiesWithImportantRules?
(In reply to Brian Birtles (:birtles) from comment #34)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #33)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=69f46ac09fc4
> 
> > +  // If multiple animations affect the same property, animations with higher
> > +  // composite order (priority) override or add or animations with lower
> > +  // priority except properties in skipProperties.
> > +  // NOTE: PropertiesWithImportantRules has only properties which can be
> > +  // run on the compositor.  In case of other properties, restyling process
> > +  // on the main-thread overrides this composition results.
> > +  for (KeyframeEffectReadOnly* effect : sortedEffectList) {
> > +    const nsCSSPropertyIDSet& skipProperties =
> > +      effect->GetAnimation()->CascadeLevel() == CascadeLevel::Animations
> > +        ? effects->PropertiesWithImportantRules()
> > +        : effects->PropertiesForAnimationsLevel();
> > +    effect->GetAnimation()->ComposeStyle(animationRule, skipProperties);
> >    }
> 
> I expected we'd use |aCascadeLevel| to determine which set of properties to
> skip? Not the cascade level of the animation?

I did also expect it, but missed!
Comment on attachment 8794644 [details]
Bug 1304922 - Part 1: Rename nsLayoutUtils::HasCurrentAnimationOfProperty() to nsLayoutUtils::HasActiveAnimationOfProperty().

https://reviewboard.mozilla.org/r/80976/#review79884

> I think preserving the existing behavior is fine, but it seems like we should be layerizing animations in their delay phase even if they don't have a backwards fill.
> 
> For example, if I load this jsbin:
> 
>   http://jsbin.com/niborugubo/edit?html,css,output
> 
> And set layers.draw-borders to true, I notice we don't layerize the div until it starts animating.
> 
> That seems very undesirable. I suspect we used to layerize such content but somewhere during our refactoring (perhaps when we introduced mWinsInCascade?) we broke that.
> 
> We should file a bug to fix that which will possibly mean changing this function back to HasCurrentAnimationOfPrroperty or having two versions of this function (HasCurrent... and HasActive...) if it turns out there are call sites that need the HasActive... behavior.

Actually the fix has not landed yet.  It will be fixed in bug 1278136 and the bug now depends on this bug (removing mWinsInCascade flag).  mWinsInCascade is a key stone various bugs.  Sorry for the confusion.
Blocks: 1278136
This looks really good. I don't think I'll finish reviewing it all before I go on leave tomorrow but one quick question, do we still need part 4? It seems like we're not adding much logic to EffectSet so those functions could stay on EffectCompositor?
Actually it works without part 4, if you don't like it, please stamp r-.  I have a patch for handling base style value for missing keyframe and the patch adds code to obtain the base style value in UpdateCascadeResults() but I will re-consider it in that bug.

The reason I don't add the code in UpdateProperties() is that, if we do that, we need to obtain the base style even if there is no AnimationProperty change because setting important rules or removing important rules does not change AnimationProperty at all,  and if we do that, obtaining the base value is processed even if there is no style change, e.g. changing animation duration or something.  But anyway it should be considered later bug.
Comment on attachment 8794648 [details]
Bug 1304922 - Part 5: AnimValuesStyleRule::AddValue replaces the existence entry's mValue.

https://reviewboard.mozilla.org/r/80984/#review80540

r=me with the following changes

::: dom/animation/AnimValuesStyleRule.h:43
(Diff revision 2)
> -  void AddValue(nsCSSPropertyID aProperty, const StyleAnimationValue &aStartValue)
> -  {
> +  // NOTE: Both of below functions replace the existence entry's mValue
> +  // if there is already an entry for |aProperty|.

"For the following functions, if there is already a value for |aProperty| it will be replaced with |aValue|."

::: dom/animation/AnimValuesStyleRule.h:51
(Diff revision 2)
> -  }
>  
>  private:
>    ~AnimValuesStyleRule() {}
>  
> -  InfallibleTArray<PropertyStyleAnimationValuePair> mPropertyValuePairs;
> +  nsDataHashtable<nsUint32HashKey, StyleAnimationValue> mAnimationValues;

I think we want nsTHashtable here? Looking at nsDataHashtable, it wraps nsBashHashtable which says, "This class manages simple data types that do not need construction or destruction". I'm pretty sure we want to destroy StyleAnimationValue objects.

::: dom/animation/AnimValuesStyleRule.cpp:37
(Diff revision 2)
> -    if (aRuleData->mSIDs & nsCachedStyleData::GetBitForSID(
> -                             nsCSSProps::kSIDTable[pair.mProperty]))
> +    if (aRuleData->mSIDs &
> +        nsCachedStyleData::GetBitForSID(nsCSSProps::kSIDTable[property])) {

I'm not sure but perhaps the previous indentation was better since it looks less like a logical operator that way?

::: dom/animation/AnimValuesStyleRule.cpp:68
(Diff revision 2)
>  
> +void
> +AnimValuesStyleRule::AddValue(nsCSSPropertyID aProperty,
> +                              const StyleAnimationValue &aValue)
> +{
> +  mAnimationValues.Put(aProperty, aValue);

Should we assert that aProperty is not eCSSProperty_UNKNOWN since it is -1 and will wrap once we cast it to uint32?
Attachment #8794648 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #49)
> ::: dom/animation/AnimValuesStyleRule.h:43
> (Diff revision 2)
> > -  void AddValue(nsCSSPropertyID aProperty, const StyleAnimationValue &aStartValue)
> > -  {
> > +  // NOTE: Both of below functions replace the existence entry's mValue
> > +  // if there is already an entry for |aProperty|.
> 
> "For the following functions, if there is already a value for |aProperty| it
> will be replaced with |aValue|."
> 
> ::: dom/animation/AnimValuesStyleRule.h:51
> (Diff revision 2)
> > -  }
> >  
> >  private:
> >    ~AnimValuesStyleRule() {}
> >  
> > -  InfallibleTArray<PropertyStyleAnimationValuePair> mPropertyValuePairs;
> > +  nsDataHashtable<nsUint32HashKey, StyleAnimationValue> mAnimationValues;
> 
> I think we want nsTHashtable here? Looking at nsDataHashtable, it wraps
> nsBashHashtable which says, "This class manages simple data types that do
> not need construction or destruction". I'm pretty sure we want to destroy
> StyleAnimationValue objects.

Oh, thanks. I did not read the comment of nsBashHashtable.  Actually in this case it seems not a problem.  I did confirm that StyleAnimationValue objects are destroyed when the hashtable is discarded (i.e. when destroying AnimValuesStyleRule).  That being said, using nsDataHashtable is misleading, I will replace it with nsTHashtable<nsBaseHashtableET<nsUint32HashKey, StyleAnimationValue>>.

Apart from that, though I did not notice, StyleAnimationValue does not have move assignment operator.  We should have the operator,  I will add a patch as part 11 here.  I did push a try with the patch, it revealed we are using StyleAnimationValue after moving it.

https://treeherder.mozilla.org/logviewer.html#?job_id=28226869&repo=try#L8180
(In reply to Brian Birtles (:birtles) from comment #49)
> > -  InfallibleTArray<PropertyStyleAnimationValuePair> mPropertyValuePairs;
> > +  nsDataHashtable<nsUint32HashKey, StyleAnimationValue> mAnimationValues;
> 
> I think we want nsTHashtable here? Looking at nsDataHashtable, it wraps
> nsBashHashtable which says, "This class manages simple data types that do
> not need construction or destruction". I'm pretty sure we want to destroy
> StyleAnimationValue objects.

If you want to run destructors, I think that means you want nsClassHashtable, not raw use of nsTHashtable.
(In reply to David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) from comment #51)
> (In reply to Brian Birtles (:birtles) from comment #49)
> > > -  InfallibleTArray<PropertyStyleAnimationValuePair> mPropertyValuePairs;
> > > +  nsDataHashtable<nsUint32HashKey, StyleAnimationValue> mAnimationValues;
> > 
> > I think we want nsTHashtable here? Looking at nsDataHashtable, it wraps
> > nsBashHashtable which says, "This class manages simple data types that do
> > not need construction or destruction". I'm pretty sure we want to destroy
> > StyleAnimationValue objects.
> 
> If you want to run destructors, I think that means you want
> nsClassHashtable, not raw use of nsTHashtable.

Thank you, dbaron.  But I am confused.  Can we add object to nsClassHashtable instead of pointer somehow?  We allocate StyleAnimationValue on stack or class members.
If you want the object to be stored in the hashtable structure (which will mean it will be copied when you put it into the hashtable), you should continue using nsDataHashtable.  I believe it will run destructors, though you should probably double-check.

If you want to allocate the object yourself, on the heap, and then hand ownership to the hashtable, you should use nsClassHashtable.
(In reply to David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) from comment #53)
> If you want the object to be stored in the hashtable structure (which will
> mean it will be copied when you put it into the hashtable), you should
> continue using nsDataHashtable.  I believe it will run destructors, though
> you should probably double-check.

Ah, Thanks!  Then I will revise it to nsDataHashtable. I will, again, check the destructor is surely run just in case.
Comment on attachment 8794647 [details]
Bug 1304922 - Part 4: Add EffectSet::Count().

https://reviewboard.mozilla.org/r/80982/#review81314

As discussed in comment 48, I don't think we need this, or at least not yet.
Attachment #8794647 - Flags: review?(bbirtles) → review-
Comment on attachment 8794649 [details]
Bug 1304922 - Part 6: Introduce mPropertiesWithImportantRules and mPropertiesForAnimationsLevel and use it to detect conditions that we need to update layers.

https://reviewboard.mozilla.org/r/80986/#review80552

Sorry, I haven't finished reviewing this yet, but here is my first round of feedback.

::: dom/animation/Animation.h:313
(Diff revision 2)
> -   * Any properties already contained in |aSetProperties| are not changed. Any
> -   * properties that are changed are added to |aSetProperties|.
> +   * If |aSkipProperties| has the properties that we don't compose any effects
> +   * for the property because of CSS cascading rules.

Any properties contained in |aPropertiesToSkip| will not be added to or updated in |aStyleRule|.

::: dom/animation/Animation.h:317
(Diff revision 2)
>     * if any.
> -   * Any properties already contained in |aSetProperties| are not changed. Any
> -   * properties that are changed are added to |aSetProperties|.
> +   * If |aSkipProperties| has the properties that we don't compose any effects
> +   * for the property because of CSS cascading rules.
>     */
>    void ComposeStyle(RefPtr<AnimValuesStyleRule>& aStyleRule,
> -                    nsCSSPropertyIDSet& aSetProperties);
> +                    const nsCSSPropertyIDSet& aSkipProperties);

Let's call this aPropertiesToSkip (and likewise later in this file).

::: dom/animation/Animation.cpp:839
(Diff revision 2)
>    return mAnimationIndex < aOther.mAnimationIndex;
>  }
>  
>  void
>  Animation::ComposeStyle(RefPtr<AnimValuesStyleRule>& aStyleRule,
> -                        nsCSSPropertyIDSet& aSetProperties)
> +                        const nsCSSPropertyIDSet& aSkipProperties)

aPropertiesToSkip

::: dom/animation/EffectCompositor.cpp:575
(Diff revision 2)
>    // Get a list of effects for the current level sorted by composite order.
>    nsTArray<KeyframeEffectReadOnly*> sortedEffectList;
>    for (KeyframeEffectReadOnly* effect : *effects) {
>      MOZ_ASSERT(effect->GetAnimation());
> -    if (effect->GetAnimation()->CascadeLevel() == aCascadeLevel) {
> +    // We don't filter out animations which don't match |aCascadeLevel|
> +    // because those animations' properties has been set in
> +    // EffectSet::mPropertiesWithImportantRules or
> +    // EffectSet::mPropertiesForAnimationsLevel and will be filter out in
> +    // ComposeStyle() so that we can compose additive or accumulate animations
> +    // onto transitions level effects.
> -      sortedEffectList.AppendElement(effect);
> +    sortedEffectList.AppendElement(effect);
> -    }
> +  }

This needs a few updates:

a) The comment should just be, "Get a list of effects sorted by composite order"
b) We no longer need to assert that effect->GetAnimation() is non-null; or at least not here
c) We could possibly expose a Count()/Length() member on EffectSet and call SetCapacity on sortedEffectList to save unnecessary allocations (since we know, now, how long the array needs to be).

::: dom/animation/EffectCompositor.cpp:596
(Diff revision 2)
> -  // property has already been set, we don't change it.
> -  nsCSSPropertyIDSet properties;
> -
> -  for (KeyframeEffectReadOnly* effect : Reversed(sortedEffectList)) {
> -    effect->GetAnimation()->ComposeStyle(animationRule, properties);
> +  // NOTE: PropertiesWithImportantRules has only properties which can be
> +  // run on the compositor.  In case of other properties, restyling process
> +  // on the main-thread overrides this composition results.
> +  const nsCSSPropertyIDSet& skipProperties =
> +    aCascadeLevel == CascadeLevel::Animations
> +      ? effects->PropertiesWithImportantRules()
> +      : effects->PropertiesForAnimationsLevel();
> +  for (KeyframeEffectReadOnly* effect : sortedEffectList) {
> +    effect->GetAnimation()->ComposeStyle(animationRule, skipProperties);
>    }

Do we even need to skip properties with important rules when we are composing on the main thread? Could we just use an empty set when aCascadeLevel is CascadeLevel::Animations?

::: dom/animation/EffectSet.h:256
(Diff revision 2)
>    // corresponding layer so we can check that the layer is up to date with
>    // the animation manager.
>    uint64_t mAnimationGeneration;
>  
> +  // Contains the properties which can be run on the compositor but overridden
> +  // by important rules.

Nit: s/important/!important/ would make this comment more clear

Perhaps, "Specifies the compositor-animatable properties that are overridden by !important rules" ?

::: dom/animation/EffectSet.h:258
(Diff revision 2)
>    uint64_t mAnimationGeneration;
>  
> +  // Contains the properties which can be run on the compositor but overridden
> +  // by important rules.
> +  nsCSSPropertyIDSet mPropertiesWithImportantRules;
> +  // Contains the properties for animation level effect.

Maybe, "Specifies the properties for which the result will be added to the animations level of the cascade and hence should be skipped when we are composing the animation style for the transitions level of the cascade" ?

::: dom/animation/EffectSet.cpp:242
(Diff revision 2)
> +  auto hasCompositorRunnableProperties =
> +    [](nsCSSPropertyIDSet& propertySet) -> bool {
> +      return propertySet.HasProperty(eCSSProperty_opacity) ||
> +             propertySet.HasProperty(eCSSProperty_transform);
> +    };

Is there any other way we can do without hard-coding the set of compositor-animatable properties in yet another place? Perhaps we can use LayerAnimationInfo.h here?

::: dom/animation/EffectSet.cpp:271
(Diff revision 2)
> +      if (overriddenProperties.HasProperty(prop.mProperty)) {
> +        mPropertiesWithImportantRules.AddProperty(prop.mProperty);
> +      }

I wonder if we need this or if we can just assign overridenProperties to mPropertiesWithImportantRules?
Comment on attachment 8794649 [details]
Bug 1304922 - Part 6: Introduce mPropertiesWithImportantRules and mPropertiesForAnimationsLevel and use it to detect conditions that we need to update layers.

https://reviewboard.mozilla.org/r/80986/#review80552

> Do we even need to skip properties with important rules when we are composing on the main thread? Could we just use an empty set when aCascadeLevel is CascadeLevel::Animations?

Indeed!

> I wonder if we need this or if we can just assign overridenProperties to mPropertiesWithImportantRules?

Though I've never check the result, I think it will cause unnecessary request for restyles.  For example, there is a transform animation and opacity style with important, in this case removing the important style causes a request.
Comment on attachment 8794647 [details]
Bug 1304922 - Part 4: Add EffectSet::Count().

https://reviewboard.mozilla.org/r/80982/#review81560

::: dom/animation/EffectSet.h:191
(Diff revision 3)
>    void UpdateAnimationGeneration(nsPresContext* aPresContext);
>    uint64_t GetAnimationGeneration() const { return mAnimationGeneration; }
>  
>    static nsIAtom** GetEffectSetPropertyAtoms();
>  
> +  size_t Count() const { return mEffects.Count(); }

Move this up next to IsEmpty()
Attachment #8794647 - Flags: review?(bbirtles) → review+
Comment on attachment 8794649 [details]
Bug 1304922 - Part 6: Introduce mPropertiesWithImportantRules and mPropertiesForAnimationsLevel and use it to detect conditions that we need to update layers.

https://reviewboard.mozilla.org/r/80986/#review81562

This is looking good but I want to have another look at it. I haven't gone through the test changes since the MozReview diff was quite hard to read and I figured the results might change depending on some of the comments below. But generally looking good!

::: dom/animation/EffectCompositor.h:188
(Diff revision 3)
> -  // Update the mWinsInCascade member for each property in effects targetting
> +  // Update the mWinsInCascade member for each property and
> +  // EffectSet::mPropertiesWithImportantRules and
> +  // EffectSet::mPropertiesForAnimationsLevel in effects targetting

"Update the mWinsInCascade member of each in effects targeting the specified (pseudo-)element. Also updates the mPropertiesWithImportantRules and mPropertiesForAnimationsLevel members of the corresponding EffectSet."

::: dom/animation/EffectCompositor.cpp:608
(Diff revision 3)
> -  // property has already been set, we don't change it.
> -  nsCSSPropertyIDSet properties;
> -
> -  for (KeyframeEffectReadOnly* effect : Reversed(sortedEffectList)) {
> +  const nsCSSPropertyIDSet& propertiesToSkip =
> +    aCascadeLevel == CascadeLevel::Animations
> +      ? nsCSSPropertyIDSet()
> +      : effects->PropertiesForAnimationsLevel();

(Wow, clever. So this only works because a local const ref to a temporary prolongs the life of the temporary. That seems to be supported by all compilers too--I just hope no-one tries to drop the 'const'!)

::: dom/animation/EffectCompositor.cpp:609
(Diff revision 3)
> -  nsCSSPropertyIDSet properties;
> -
> -  for (KeyframeEffectReadOnly* effect : Reversed(sortedEffectList)) {
> +    aCascadeLevel == CascadeLevel::Animations
> +      ? nsCSSPropertyIDSet()
> +      : effects->PropertiesForAnimationsLevel();

Nit: According to our coding style the '?' should be under the 'a' in 'aCascadeLevel'.

::: dom/animation/EffectCompositor.cpp:692
(Diff revision 3)
> +  // Preserve the state whether we have opacity or transform properties
> +  // in propertiesWithImportantRules or propertiesForAnimationsLevel to
> +  // check whether it's changed later.
> +  auto hasCompositorRunnableProperties =
> +    [](nsCSSPropertyIDSet& propertySet) -> bool {
> +      for (const LayerAnimationInfo::Record& record :
> +             LayerAnimationInfo::sRecords) {
> +        if (propertySet.HasProperty(record.mProperty)) {
> +          return true;
> +        }
> +      }
> +      return false;
> +    };
> +  bool hadCompositorRunnablePropertiesWithImportantRules =
> +    hasCompositorRunnableProperties(propertiesWithImportantRules);
> +  bool hadCompositorRunnablePropertiesForAnimationsLevel =
> +    hasCompositorRunnableProperties(propertiesForAnimationsLevel);

Is a bool sufficient here? Suppose we animations of 'opacity' and 'transform' and initially there is an !important rule overriding the opacity animation. Then, when we update, we notice that we now how an !important rule on the 'transform' property but not the 'opacity' property.

I think hadCompositorRunnablePropertiesWithImportantRules will be == hasCompositorRunnableProperties(propertiesWithImportantRules), right?

Should we just build up a bitfield to represent the properties that are set and then compare that?

::: dom/animation/EffectCompositor.cpp:773
(Diff revision 3)
> +  // If properties for compositor are newly overridden by important rules, or
> +  // released from being overridden by important rules, we need to update
> +  // layers for animations level because it's a trigger to send animations to
> +  // the compositor or pull animations back from the compositor.
> +  if (hadCompositorRunnablePropertiesWithImportantRules !=
> +        hasCompositorRunnableProperties(propertiesWithImportantRules)) {
> +    presContext->EffectCompositor()->
> +      RequestRestyle(aElement, aPseudoType,
> +                     EffectCompositor::RestyleType::Layer,
> +                     EffectCompositor::CascadeLevel::Animations);
> +  }
> +  // If we have transition properties for compositor and if the same propery
> +  // for animations level is newly added or removed, we need to update layers
> +  // for transitions level because composite order has been changed now.
> +  if (hasCompositorRunnablePropertiesForTransition &&
> +      hadCompositorRunnablePropertiesForAnimationsLevel !=
> +        hasCompositorRunnableProperties(propertiesForAnimationsLevel)) {
> +    presContext->EffectCompositor()->
> +      RequestRestyle(aElement, aPseudoType,
> +                     EffectCompositor::RestyleType::Layer,
> +                     EffectCompositor::CascadeLevel::Transitions);
>    }

I wonder if we really need to be this precise. It's not common that we have changes in important rules or animations taking over transitions--would it be simpler to just restyle both levels if either of those things change?

::: dom/animation/EffectSet.h:245
(Diff revision 3)
>  
> +  // Specifies the compositor-animatable properties that are overridden by
> +  // !important rules.
> +  nsCSSPropertyIDSet mPropertiesWithImportantRules;
> +  // Specifies the properties for which the result will be added to the
> +  // animations level of the cascade and hence should be skipped where we are

Nit: s/where we/when we/

::: dom/animation/KeyframeEffectReadOnly.cpp:206
(Diff revision 3)
> -      if (!result->mWinsInCascade) {
> +      if (effectSet &&
> +          effectSet->PropertiesWithImportantRules()
> +            .HasProperty(result->mProperty)) {
> +        // Ignore if the property is overridden by important rules.

Nit: I think this would be more clear if the comment went before the condition and said something like:

// Skip the property if it is overridden by !important rules

That said, is this right? If we have a transition and an !important rule, I believe the existing code would have mWinsInCascade == true and hence would return an AnimationProperty*. Do we only want to check PropertiesWithImportantRules() if PropertiesForAnimationsLevel() is also true?

(Also, on a very minor note, I notice this gets called for background_position_x etc. too in which case PropertiesWithImportantRules() will not be set. Not sure that matters?)

::: dom/animation/KeyframeEffectReadOnly.cpp:319
(Diff revision 3)
> +  // Basically this function is called only while the effect is in-effect,
> +  // but there is a race condition that animation's current time gets back to
> +  // before phase or forward to after phase due to temporary hold time tweaks in
> +  // Animation::ComposeStyle().
> +  if (!IsInEffect()) {
> +    return;
> +  }

I don't understand this comment.

::: dom/animation/KeyframeEffectReadOnly.cpp:1136
(Diff revision 3)
> +  // Should not block other animations if this effect is not in-effect.
> +  if (!IsInEffect()) {
> +    return false;
> +  }

Is this right? If we have an animation of transform and margin-left, but there is a delay on the margin-left animation, shouldn't we still block the transform animation?

::: dom/animation/KeyframeEffectReadOnly.cpp:1144
(Diff revision 3)
> -    // If a property is overridden in the CSS cascade, it should not block other
> -    // animations from running on the compositor.
> -    if (!property.mWinsInCascade) {
> +    // If a property is overridden by important rules, it should not block other
> +    // other animations from running on the compositor.
> +    if (effectSet &&
> +        effectSet->PropertiesWithImportantRules()
> +          .HasProperty(property.mProperty)) {
>        continue;
>      }

We should probably mention that this only works for compositor-animatable properties. (e.g. if a margin-left animation is overridden by an !important rule, we'll still let it block transform animations since we don't add margin-left to PropertiesWithImportantRules.)

Also, don't we want to check PropertiesForAnimationsLevel first? i.e. if the property is in *both* sets skip it?

::: layout/base/nsDisplayList.cpp:495
(Diff revision 3)
>        keyframeEffect->GetAnimationOfProperty(aProperty);
>      if (!property) {
>        continue;
>      }
>  
> -    // Note that if mWinsInCascade on property was  false,
> +    // Note that if the property is overridden by important rules,

Nit: I think !important is more clear here and two places below
Attachment #8794649 - Flags: review?(bbirtles)
Comment on attachment 8794650 [details]
Bug 1304922 - Part 7: Drop mWinsInCascade.

https://reviewboard.mozilla.org/r/80988/#review81574

::: dom/animation/EffectSet.h:226
(Diff revision 3)
>    EnumeratedArray<EffectCompositor::CascadeLevel,
>                    EffectCompositor::CascadeLevel(
>                      EffectCompositor::kCascadeLevelCount),
>                    TimeStamp> mAnimationRuleRefreshTime;
>  
> -  // Dirty flag to represent when the mWinsInCascade flag on effects in
> +  // Dirty flag to represent when the mPropertyMask on effects in

This comment seems out of date?

::: dom/animation/KeyframeEffectReadOnly.h:156
(Diff revision 3)
> -  // mIsRunningOnCompositor member.
>    // This is because AnimationProperty objects are compared when recreating
>    // CSS animations to determine if mutation observer change records need to
>    // be created or not. However, at the point when these objects are compared
> -  // neither the mWinsInCascade nor the mIsRunningOnCompositor will have been
> -  // set on the new objects so we ignore these members to avoid generating
> +  // the mIsRunningOnCompositor will not have been set on the new objects so
> +  // we ignore these members to avoid generating spurious change records.

Nit: s/these members/this member/

::: dom/animation/KeyframeEffectReadOnly.cpp:285
(Diff revision 3)
>  
>    if (mProperties == properties) {
>      return;
>    }
>  
> -  // Preserve the state of mWinsInCascade and mIsRunningOnCompositor flags.
> +  // Preserve the state of mIsRunningOnCompositor flags.

Nit: Preserve the state of the mIsRunningOnCompositor flag.
Attachment #8794650 - Flags: review?(bbirtles) → review+
Comment on attachment 8796001 [details]
Bug 1304922 - Part 8: Add a test case to check we don't request restyle for layer when cascading result for the property not running on the compositor is changed

https://reviewboard.mozilla.org/r/81976/#review81576

I don't really understand this test. Are we testing that adding an !important rule to a non-compositor-animatable property does not create a restyle? But the test fails since we currently do restyles for such animations anyway due to bug 1300982? But even if we fix bug 1300982, don't we still want at least one restyle to remove the effect from background-color setting from the animation rule?
Comment on attachment 8796002 [details]
Bug 1304922 - Part 9: Early return from FindAnimationsForCompositor() if we have neither transitions and animations level effect for a given property.

https://reviewboard.mozilla.org/r/81978/#review81578

::: dom/animation/EffectCompositor.cpp:76
(Diff revision 2)
> +  // If the property is overridden by important rules and there is a property
> +  // for animations level, it means that there are animations overridden by
> +  // important rules and maybe be transitions suppressed by the animations,
> +  // that means we have no animations for the compositor.

"If the property will be added to the animations level of the cascade but there is an !important rule for that property in the cascade then the animation will not be applied since the !important rule overrides it."

::: dom/animation/EffectCompositor.cpp:80
(Diff revision 2)
> +  if (effects->PropertiesWithImportantRules()
> +        .HasProperty(aProperty) &&
> +      effects->PropertiesForAnimationsLevel()
> +        .HasProperty(aProperty)) {

I think this fits within 80 chars?

  if (effects->PropertiesWithImportantRules().HasProperty(aProperty) &&
      effects->PropertiesForAnimationsLevel().HasProperty(aProperty)) {
    return false;
  }

I've noticed a few places within these patches we seem to be wrapping more than we need to. (I know some reviewers suggest wrapping even if the line is 78~79 characters but in this case the line is only 71 characters so I don't think we need to wrap.)
Attachment #8796002 - Flags: review?(bbirtles) → review+
Comment on attachment 8796003 [details]
Bug 1304922 - Part 10: Drop non-const version of KeyframeEffectReadOnly::Properties().

https://reviewboard.mozilla.org/r/81980/#review81580

::: layout/base/nsLayoutUtils.cpp:556
(Diff revision 2)
>      for (size_t propIdx = effect->Properties().Length(); propIdx-- != 0; ) {
> -      AnimationProperty& prop = effect->Properties()[propIdx];
> +      const AnimationProperty& prop = effect->Properties()[propIdx];
>        if (prop.mProperty == eCSSProperty_transform) {
>          for (uint32_t segIdx = prop.mSegments.Length(); segIdx-- != 0; ) {
> -          AnimationPropertySegment& segment = prop.mSegments[segIdx];
> +          const AnimationPropertySegment& segment = prop.mSegments[segIdx];

(Might be worth switching this over to a range-based loop while we're at it? Not sure we need the reverse behavior either--I think that was just to simplify dealing with the unsigned size_t values.)
Attachment #8796003 - Flags: review?(bbirtles) → review+
Comment on attachment 8797117 [details]
Bug 1304922 - Part 11: Don't use StyleAnimationValue after moving it.

https://reviewboard.mozilla.org/r/82738/#review81582

(I think you misspelled my nick in the commit message so this review request wasn't assigned to me but maybe autoland will fix that when it lands?)
Attachment #8797117 - Flags: review+
(In reply to Brian Birtles (:birtles) from comment #73)
> Comment on attachment 8796001 [details]
> Bug 1304922 - Part 8: Add a test case to check we don't request restyle for
> layer when cascading result for the property not running on the compositor
> is changed
> 
> https://reviewboard.mozilla.org/r/81976/#review81576
> 
> I don't really understand this test. Are we testing that adding an
> !important rule to a non-compositor-animatable property does not create a
> restyle? But the test fails since we currently do restyles for such
> animations anyway due to bug 1300982?

Right, exactly.

> But even if we fix bug 1300982, don't
> we still want at least one restyle to remove the effect from
> background-color setting from the animation rule?

I believe we don't.  Color layer is independent from opacity or transform layer, we don't need to synchronize opacity and transform layers.  I've confirmed that below test works fine without restyles.

 div.style.backgroundColor = "blue";
 div.animate({ opacity: [0, 1] }, 1000);
 setTimeout(() => {
   div.style.setProperty("background-color", "red", "important");
 }, 500);
Hmm, it won't let me assign myself as the reviewer of part 11 because it says the patch author has drafts...
(In reply to Hiroyuki Ikezoe (:hiro) from comment #77)
> (In reply to Brian Birtles (:birtles) from comment #73)
> > But even if we fix bug 1300982, don't
> > we still want at least one restyle to remove the effect from
> > background-color setting from the animation rule?
> 
> I believe we don't.  Color layer is independent from opacity or transform
> layer, we don't need to synchronize opacity and transform layers.

Oh, we are only counting layer restyles? We still need to update the animation rule but is that restyle not counted?
(In reply to Brian Birtles (:birtles) from comment #79)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #77)
> > (In reply to Brian Birtles (:birtles) from comment #73)
> > > But even if we fix bug 1300982, don't
> > > we still want at least one restyle to remove the effect from
> > > background-color setting from the animation rule?
> > 
> > I believe we don't.  Color layer is independent from opacity or transform
> > layer, we don't need to synchronize opacity and transform layers.
> 
> Oh, we are only counting layer restyles? We still need to update the
> animation rule but is that restyle not counted?

We are only counting restyles caused by animations.  In the test cases, the background color style is not changed by animations, it's changed by setting a new style.  I don't know what we call it inside Gecko.
Comment on attachment 8797118 [details]
Bug 1304922 - Part 12: Add move assignment operator for StyleAnimationValue.

https://reviewboard.mozilla.org/r/82740/#review81586

::: layout/style/StyleAnimationValue.h:523
(Diff revision 1)
> +  {
> +    MOZ_ASSERT(this != &aOther, "Do not move itself");
> +    if (this != &aOther) {
> +      FreeValue();
> +      mUnit = aOther.mUnit;
> +      mValue = aOther.mValue;

Perhaps we should add a comment to mention that the implicitly-defined copy assignment operator for a union type uses memmove.
Attachment #8797118 - Flags: review?(bbirtles) → review+
Comment on attachment 8796001 [details]
Bug 1304922 - Part 8: Add a test case to check we don't request restyle for layer when cascading result for the property not running on the compositor is changed

https://reviewboard.mozilla.org/r/81976/#review81588

::: dom/animation/test/chrome/test_restyles.html:732
(Diff revision 2)
> +      // Make the background-color style as important to cause cascading
> +      // result change for the background-color.

Perhaps the first sentence could be: "Mark the background-color style as !important to cause an update to the cascade."
Attachment #8796001 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #71)
> ::: dom/animation/EffectCompositor.cpp:692
> (Diff revision 3)
> > +  // Preserve the state whether we have opacity or transform properties
> > +  // in propertiesWithImportantRules or propertiesForAnimationsLevel to
> > +  // check whether it's changed later.
> > +  auto hasCompositorRunnableProperties =
> > +    [](nsCSSPropertyIDSet& propertySet) -> bool {
> > +      for (const LayerAnimationInfo::Record& record :
> > +             LayerAnimationInfo::sRecords) {
> > +        if (propertySet.HasProperty(record.mProperty)) {
> > +          return true;
> > +        }
> > +      }
> > +      return false;
> > +    };
> > +  bool hadCompositorRunnablePropertiesWithImportantRules =
> > +    hasCompositorRunnableProperties(propertiesWithImportantRules);
> > +  bool hadCompositorRunnablePropertiesForAnimationsLevel =
> > +    hasCompositorRunnableProperties(propertiesForAnimationsLevel);
> 
> Is a bool sufficient here? Suppose we animations of 'opacity' and
> 'transform' and initially there is an !important rule overriding the opacity
> animation. Then, when we update, we notice that we now how an !important
> rule on the 'transform' property but not the 'opacity' property.
> 
> I think hadCompositorRunnablePropertiesWithImportantRules will be ==
> hasCompositorRunnableProperties(propertiesWithImportantRules), right?

Good catch!  Fixed it locally.  I also wrote a test case to check this case, but unfortunately it fails.  I am still investgating the reason but anyway a restyle comes from somewhere else.  Filed bug 1307341.
Comment on attachment 8794649 [details]
Bug 1304922 - Part 6: Introduce mPropertiesWithImportantRules and mPropertiesForAnimationsLevel and use it to detect conditions that we need to update layers.

https://reviewboard.mozilla.org/r/80986/#review81562

> Nit: I think this would be more clear if the comment went before the condition and said something like:
> 
> // Skip the property if it is overridden by !important rules
> 
> That said, is this right? If we have a transition and an !important rule, I believe the existing code would have mWinsInCascade == true and hence would return an AnimationProperty*. Do we only want to check PropertiesWithImportantRules() if PropertiesForAnimationsLevel() is also true?
> 
> (Also, on a very minor note, I notice this gets called for background_position_x etc. too in which case PropertiesWithImportantRules() will not be set. Not sure that matters?)

Wow thanks!  I did initially such checks but I did drop it at some point. It was totally wrong.  Unfortunately we didn't have such test cases.  I added two test cases in test_running_on_compositor.html.  A transition with an important rule, and a transition and an animation with an important rule.  The formar should run on the compositor, the latter should not.  

Regarding background_position animations, I did ask to Markus before, and he said it's OK, see bug 1292441 comment2.

> I don't understand this comment.

Initially I did use MOZ_ASSERT(IsInEffect()) there because we are checking IsInEffect() at the top of Animation::ComposeStyle().  But I saw sometimes that the assertion was hit on try, and then I noticed that the assertion was caused by the tweak for hold time in Animation::ComposeStyle().

> Is this right? If we have an animation of transform and margin-left, but there is a delay on the margin-left animation, shouldn't we still block the transform animation?

This change just aims to preserve current behavior.  I have a patch to drop the IsInEffect check in bug 1223658.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #96)
> > I don't understand this comment.
> 
> Initially I did use MOZ_ASSERT(IsInEffect()) there because we are checking
> IsInEffect() at the top of Animation::ComposeStyle().  But I saw sometimes
> that the assertion was hit on try, and then I noticed that the assertion was
> caused by the tweak for hold time in Animation::ComposeStyle().

If we're not in effect, won't we return early when we check computedTiming.mProgress.IsNull() a few lines later?
(In reply to Brian Birtles (:birtles) from comment #97)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #96)
> > > I don't understand this comment.
> > 
> > Initially I did use MOZ_ASSERT(IsInEffect()) there because we are checking
> > IsInEffect() at the top of Animation::ComposeStyle().  But I saw sometimes
> > that the assertion was hit on try, and then I noticed that the assertion was
> > caused by the tweak for hold time in Animation::ComposeStyle().
> 
> If we're not in effect, won't we return early when we check
> computedTiming.mProgress.IsNull() a few lines later?

Ah, right. I will drop the check.
Comment on attachment 8794649 [details]
Bug 1304922 - Part 6: Introduce mPropertiesWithImportantRules and mPropertiesForAnimationsLevel and use it to detect conditions that we need to update layers.

https://reviewboard.mozilla.org/r/80986/#review82054

Thank you for doing this!

Nit: s/layres/layers/ in the commit message

::: dom/animation/EffectCompositor.cpp:592
(Diff revision 4)
> -    MOZ_ASSERT(effect->GetAnimation());
> -    if (effect->GetAnimation()->CascadeLevel() == aCascadeLevel) {
> +    // We don't filter out animations which don't match |aCascadeLevel|
> +    // because those animations' properties has been set in
> +    // EffectSet::mPropertiesWithImportantRules or
> +    // EffectSet::mPropertiesForAnimationsLevel and will be filter out in
> +    // ComposeStyle() so that we can compose additive or accumulate animations
> +    // onto transitions level effects.

Do we need this comment?

If so,

s/has been set/have been set/
s/will be filter out/will be filtered out/

::: dom/animation/EffectCompositor.cpp:688
(Diff revision 4)
> -  bool changed = false;
> +  // Returns a bitset contains information on each bit that a CSS property
> +  // runnable on the compositor is included in a given nsCSSPropertyIDSet or
> +  // not.
> +  // The order of each bit matches to LayerAnimationInfo records' CSS property.

How about: "Returns a bitset the represents which properties from LayerAnimationInfo::sRecords are present in |aPropertySet|."

::: dom/animation/EffectCompositor.cpp:692
(Diff revision 4)
> +  auto hasCompositorRunnableProperties =
> +    [](nsCSSPropertyIDSet& propertySet) ->

Since this no longer returns a bool the name should probably be compositorRunnablePropertiesInSet (or just compositorPropertiesInSet?)

Also s/propertySet/aPropertySet/

::: dom/animation/EffectCompositor.cpp:705
(Diff revision 4)
> +  // Preserve the state whether we have opacity or transform properties
> +  // in propertiesWithImportantRules or propertiesForAnimationsLevel to
> +  // check whether it's changed later.

"Record which compositor-animatable properties were originally set so we can compare for changes later."

Also, we should move this comment down to just before we call compositorPropertiesInSet.

::: dom/animation/EffectCompositor.cpp:713
(Diff revision 4)
> +  std::bitset<LayerAnimationInfo::kRecords>
> +    hadCompositorRunnablePropertiesWithImportantRules =
> +      hasCompositorRunnableProperties(propertiesWithImportantRules);
> +  std::bitset<LayerAnimationInfo::kRecords>
> +    hadCompositorRunnablePropertiesForAnimationsLevel =
> +      hasCompositorRunnableProperties(propertiesForAnimationsLevel);

These names should probably be something like:

* prevCompositorPropertiesWithImportantRules
* prevCompositorPropertiesForAnimationsLevel

(assuming we name the function above to compositorPropertiesInSet)

::: dom/animation/EffectCompositor.cpp:726
(Diff revision 4)
> +  propertiesForAnimationsLevel.Empty();
> +
>    nsCSSPropertyIDSet animatedProperties;
> +  bool hasCompositorRunnablePropertiesForTransition = false;
>  
> -  // Iterate from highest to lowest composite order.
> +  // Iterate from lowest to highest composite order.

(I wonder if we even need this comment now)

::: dom/animation/EffectCompositor.cpp:782
(Diff revision 4)
> +
> +  // If properties for compositor are newly overridden by !important rules, or
> +  // released from being overridden by !important rules, we need to update
> +  // layers for animations level because it's a trigger to send animations to
> +  // the compositor or pull animations back from the compositor.
> +  if (hadCompositorRunnablePropertiesWithImportantRules !=
> +        hasCompositorRunnableProperties(propertiesWithImportantRules)) {
> +    presContext->EffectCompositor()->
> +      RequestRestyle(aElement, aPseudoType,
> +                     EffectCompositor::RestyleType::Layer,
> +                     EffectCompositor::CascadeLevel::Animations);
> +  }
> +  // If we have transition properties for compositor and if the same propery
> +  // for animations level is newly added or removed, we need to update layers
> +  // for transitions level because composite order has been changed now.
> +  if (hasCompositorRunnablePropertiesForTransition &&
> +      hadCompositorRunnablePropertiesForAnimationsLevel !=
> +        hasCompositorRunnableProperties(propertiesForAnimationsLevel)) {
> +    presContext->EffectCompositor()->
> +      RequestRestyle(aElement, aPseudoType,
> +                     EffectCompositor::RestyleType::Layer,
> +                     EffectCompositor::CascadeLevel::Transitions);

(I'm still a little unsure if this complexity is needed--it seems like we're more likely to have bugs by failing to restyle in some cases and that the benefit of, for example, only recreating the transitions level and not the animations level, is minimal. But I guess it's ok.)

::: dom/animation/KeyframeEffectReadOnly.cpp:212
(Diff revision 4)
> +        // Skip if there is a property of animation level that is overridden
> +        // by !important rules.

I think this comment makes more sense before the condition? (i.e. before the if)

::: dom/animation/KeyframeEffectReadOnly.cpp:323
(Diff revision 4)
> +  // Basically this function is called only while the effect is in-effect,
> +  // but there is a race condition that animation's current time gets back to
> +  // before phase or forward to after phase due to temporary hold time tweaks in
> +  // Animation::ComposeStyle().
> +  if (!IsInEffect()) {
> +    return;
> +  }
> +

As discussed, we should drop this comment and early return.

::: dom/animation/KeyframeEffectReadOnly.cpp:1153
(Diff revision 4)
> -    // If a property is overridden in the CSS cascade, it should not block other
> -    // animations from running on the compositor.
> -    if (!property.mWinsInCascade) {
> +    if (effectSet &&
> +        effectSet->PropertiesWithImportantRules()
> +          .HasProperty(property.mProperty) &&
> +        effectSet->PropertiesForAnimationsLevel()
> +          .HasProperty(property.mProperty)) {
> +      // If there is a property for aniamtions level that is overridden by

s/aniamtions/animations/

::: dom/animation/KeyframeEffectReadOnly.cpp:1153
(Diff revision 4)
> +      // If there is a property for aniamtions level that is overridden by
> +      // !important rules, it should not block other animations from running
> +      // on the compositor.
> +      // NOTE: We don't currently check the important rules for properties
> +      // other than opacity or transform so such properties (e.g. margin-left)
> +      // still blocks even if the properties are overridden by !important rules.

Again, I think this comment makes more sense before the 'if'.

::: dom/animation/KeyframeEffectReadOnly.cpp:1156
(Diff revision 4)
> +      // NOTE: We don't currently check the important rules for properties
> +      // other than opacity or transform so such properties (e.g. margin-left)
> +      // still blocks even if the properties are overridden by !important rules.

"NOTE: We don't currently check for !important rules for properties that don't run on the compositor. As result such properties (e.g. margin-left) can still block async animations even if they are overridden by !important rules."

::: dom/animation/test/chrome/test_running_on_compositor.html:674
(Diff revision 4)
>                    'compositor even after a lower-priority animation is ' +
>                    'applied to the same element');
>    });
> -}, 'Animation continues not running on the compositor after being applied ' +
> +}, 'Animation begins running on the compositor after being applied ' +
>     'to an element which has a higher-priority animation and after ' +
>     'being remporarily associated with no target element');

While we're at it, can we fix s/remporarily/temporarily/

::: dom/animation/test/chrome/test_running_on_compositor.html:749
(Diff revision 4)
> +}, 'Neither transition nor animation does not run on the compositor if the ' +
> +   'property is overridden by an !important rule');

'Neither transition nor animation runs on the compositor...'

('Neither' + 'does not run' = double negative, i.e. *does* run)
Attachment #8794649 - Flags: review?(bbirtles) → review+
Comment on attachment 8794649 [details]
Bug 1304922 - Part 6: Introduce mPropertiesWithImportantRules and mPropertiesForAnimationsLevel and use it to detect conditions that we need to update layers.

https://reviewboard.mozilla.org/r/80986/#review82054

> Do we need this comment?
> 
> If so,
> 
> s/has been set/have been set/
> s/will be filter out/will be filtered out/

I did surely drop the comment but it was revived somehow.  I guess I did undo something..

> (I'm still a little unsure if this complexity is needed--it seems like we're more likely to have bugs by failing to restyle in some cases and that the benefit of, for example, only recreating the transitions level and not the animations level, is minimal. But I guess it's ok.)

I forgot to leave a comment here yesterday.  I understand your concern but I'd like to keep our test cases for restyles with precision.  As far as I can tell we have sufficient test cases, not only just in test_restyles.html, that are covering restyles caused by changing cascades, so once we fall into the failure some test cases will catch it, I hope.
Pushed by hiikezoe@mozilla-japan.org:
https://hg.mozilla.org/integration/autoland/rev/8b74bede29c3
Part 1: Rename nsLayoutUtils::HasCurrentAnimationOfProperty() to nsLayoutUtils::HasActiveAnimationOfProperty(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/df062c00654d
Part 2: Sort animation array before sending animations to the compositor. r=birtles
https://hg.mozilla.org/integration/autoland/rev/0c0cf80887d3
Part 3: Request restyle for layer when CSS animation's index is changed. r=birtles
https://hg.mozilla.org/integration/autoland/rev/37ffd2ddd1ec
Part 4: Add EffectSet::Count(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/04b7daffe438
Part 5: AnimValuesStyleRule::AddValue replaces the existence entry's mValue. r=birtles
https://hg.mozilla.org/integration/autoland/rev/01bdca9df2c7
Part 6: Introduce mPropertiesWithImportantRules and mPropertiesForAnimationsLevel and use it to detect conditions that we need to update layers. r=birtles
https://hg.mozilla.org/integration/autoland/rev/fa352a014eb4
Part 7: Drop mWinsInCascade. r=birtles
https://hg.mozilla.org/integration/autoland/rev/a4503a863c0c
Part 8: Add a test case to check we don't request restyle for layer when cascading result for the property not running on the compositor is changed r=birtles
https://hg.mozilla.org/integration/autoland/rev/4d72270b22c0
Part 9: Early return from FindAnimationsForCompositor() if we have neither transitions and animations level effect for a given property. r=birtles
https://hg.mozilla.org/integration/autoland/rev/d16bd0cb1511
Part 10: Drop non-const version of KeyframeEffectReadOnly::Properties(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/679f93a3e5db
Part 11: Don't use StyleAnimationValue after moving it. r=birtles
https://hg.mozilla.org/integration/autoland/rev/4372532be221
Part 12: Add move assignment operator for StyleAnimationValue. r=birtles
I thought I have to wait for landing bug 1302949, but actually I did not have to.  All patches here could be applied to inbound.  Wow!
Depends on: 1309837
Depends on: 1336772
You need to log in before you can comment on or make changes to this bug.