Closed Bug 1228229 Opened 9 years ago Closed 8 years ago

Rewrite wins-in-cascade setting using the EffectSet

Categories

(Core :: DOM: Animation, defect)

Product:
Core
Component:
DOM: Animation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox46 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(10 files, 17 obsolete files)

part 2 - Add a helper to get the appropriate (pseudo-)element for a frame
6.89 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 3 - Factor out a method to get compositor-animatable overridden properties
7.09 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 4 - Add a flag to EffectSet to mark when the cascade needs to be updated
4.22 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 7 - Add a method to Animation to indicate if it applies to the transitions level of the cascade
2.78 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 11 - Avoid calling nsRuleNode::ComputePropertiesOverridingAnimation when there are no compositor-animatable properties
5.45 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 8 - Add EffectCompositor::(Maybe)UpdateCascadeResults
11.70 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 10 - Remove no-longer-used cascade functions
14.42 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 5 - Separate target element registration in NotifyAnimationTimingUpdated
3.79 KB, patch
Details | Diff | Splinter Review
part 6 - Mark the animation cascade results as dirty when an effect goes in or out of being "in effect"
3.12 KB, patch
Details | Diff | Splinter Review
part 9 - Use EffectCompositor::UpdateCascadeResults
6.65 KB, patch
dbaron
: review+
Details | Diff | Splinter Review 
      No description provided.
In debugging the patches for this bug I noticed we were often doing
unnecessary work since we never discard of empty EffectSet objects attached
to Elements.

This patch makes us lazily discard such objects in GetEffectSet. This
means that we do need to be a bit careful that if we call GetEffectSet,
remove something from the set, then call something else that, in turn, calls
GetEffectSet, the original pointer to the EffectSet will dangle. Having
to be a bit careful (i.e. having to re-fecth the Effectset after calling
anything that might modify it *and* re-fetch it) still seems preferable to
the complexity of making it ref-counted.

Once we introduce a step where we traverse all EffectSets to composite them
(part of bug 1190235) we can probably replace this with an explicit step
to clean up empty effect sets which would be more robust.
Attachment #8693999 - Flags: review?(dbaron)
We will use similar logic later in this patch series so we separate it out into
a separate helper function here.
Attachment #8694000 - Flags: review?(dbaron)
This patch also simplifies this logic by simply always looking for overrides of
'transform' and 'opacity'.

REVIEW: It's not clear to me how much we save by narrowing down the set of
properties to track passed to ComputePropertiesOverridingAnimation. Given that
we are careful not to call UpdateCascadeResults when it is unnecessary (we only
do it when the style context has changed, not on every tick) and that
ComputePropertiesOverridingAnimation doesn't contain any early returns for
the case where aProperties is empty, it seems like we don't save much by doing
the extra work of examining all animations to see what properties they're
actually using.
Attachment #8694001 - Flags: review?(dbaron)
There are three situations when the cascade results of effects needs to be
updated.

1. The sets of effects (animations) has changed.

2. One or more effects have changed their "in effect" status.

3. Other style properties affecting the element have changing meaning that
   animations applied at the animations-level of the cascade may now be
   overridden or become active again.

We want to detect these situations so we can avoid updating the cascade when
none of these possibilities exist.

Currently we handle case 1 by calling UpdateCascadeResults at the appropriate
point in nsAnimationManager and nsTransitionManager when we build
animations/transtiions.

Case 2 only affects animations (since whether transitions are in effect or not
makes no difference to the cascade--they have a lower "composite order" than
animations and never overlap with each other so they can't override anything).
As a result, we handle it by adding a flag to CSSAnimation to track when an
animation was in effect last time we checked or not.

For case 3, we take care to call UpdateCascadeResults when the style context
changed in nsAnimationManager::CheckAnimationRule (called from
nsStyleSet::GetContext).

We want to generalize this detection to handle script-generated animations too.
In order to do that this patch introduces a flag to EffectSet that we will use
to mark when the cascade needs to be updated in cases 1 and 2. This patch also
sets the flag when we detect case 1. A subsequent patch sets the flag for
case 2.

Case 3 is more difficult to detect and so we simply maintain the existing
behavior of making nsAnimationManager::CheckAnimationRule unconditionally
update the cascade without checking if the "needs update" flag is set.
Attachment #8694002 - Flags: review?(dbaron)
KeyframeEffectReadOnly::NotifyAnimationTimingUpdated currently just acts as an
alias for UpdateTargetRegistration. However, bug 1226118 added logic to
UpdateTargetRegistration which is not strictly related to updating the target
element registration. This patch tidies this up so that UpdateTargetRegistration
only does what its name suggests. This is in preparation for adding more
logic to NotifyAnimationTimingUpdated.
Attachment #8694003 - Flags: review?(dbaron)
goes in or out of being "in effect"

This patch implements "case 2" describes in the commit message from part 4 of
this patch series.
Attachment #8694004 - Flags: review?(dbaron)
For transitions, this method return false--i.e. the "animation" does *not*
apply to the animations level of the cascade--so long as the transition is
bound to markup. This is based on the below discussion,

  https://github.com/w3c/web-animations/issues/97
  https://github.com/w3c/web-animations/issues/62#issuecomment-117357703
  https://github.com/w3c/web-animations/issues/62#issuecomment-117374689

We will likely reuse this method when we come to implement animation style rule
generation in a more generic manner.
Attachment #8694005 - Flags: review?(dbaron)
Attachment #8693444 - Attachment is obsolete: true
Comment on attachment 8693999 [details] [diff] [review]
part 1 - Remove empty EffectSets in GetEffectSet

This part 1 turns out to be a bad idea (and is the reason for the try failures in comment 12). It's just too fragile to delete effects during GetEffectSet. We should explicitly do clean up at a point where we know it's safe to do so. I'll address that in another bug.
Attachment #8693999 - Flags: review?(dbaron)
(In reply to Brian Birtles (:birtles) from comment #13)
> Comment on attachment 8693999 [details] [diff] [review]
> part 1 - Remove empty EffectSets in GetEffectSet
> 
> This part 1 turns out to be a bad idea (and is the reason for the try
> failures in comment 12). It's just too fragile to delete effects during
> GetEffectSet. We should explicitly do clean up at a point where we know it's
> safe to do so. I'll address that in another bug.

Specifically, for my own reference, the cause of the test failures here was that I made EffectCompositor::GetAnimationsForCompositor call MaybeUpdateCascadeResults *after* getting the effect set but *before* iterating over it. While running MaybeUpdateCascadeResults, we would call GetEffectSet, notice it was empty and delete the property. Then when we unwind the stack to EffectCompositor::GetAnimationsForCompositor, the effect set pointer dangles.

We could just fetch the effect set after calling MaybeUpdateCascadeResults but it seems likely we'll encounter this kind of bug again. We really need to find a suitable place to tidy up these properties. We could even just introduce GetNonEmptyEffectSet (or just a GetEffectSetOptions::DestroyIfEmpty flag?) which would do the automatic deleting.

I think this will be easier to work out once I finish the last part of bug 1190235. For now I might just update these patches to check for an empty effect set.
Attachment #8693999 - Attachment is obsolete: true
Attachment #8694008 - Attachment is obsolete: true
Attachment #8694008 - Flags: review?(dbaron)
Renamed AppliesToAnimationsLevel to AppliesToTransitionsLevel since I think it makes more sense to call out the exception rather than the rule
Attachment #8694503 - Flags: review?(dbaron)
Attachment #8694005 - Attachment is obsolete: true
Attachment #8694005 - Flags: review?(dbaron)
Rebase and add checks for empty effect sets
Attachment #8694504 - Flags: review?(dbaron)
Attachment #8694006 - Attachment is obsolete: true
Attachment #8694006 - Flags: review?(dbaron)
Re-order calls in GetAnimationsForCompositor to make it a little more logical and check for empty sets
Attachment #8694505 - Flags: review?(dbaron)
Attachment #8694007 - Attachment is obsolete: true
Attachment #8694007 - Flags: review?(dbaron)
Comment on attachment 8694000 [details] [diff] [review]
part 2 - Add a helper to get the appropriate (pseudo-)element for a frame

Please make the new function take |const nsIFrame*| instead of |const nsIFrame&|.

That's really just local style ; we pass frames by pointer rather than reference.  (They were once XPCOM objects, and we pass XPCOM objects by pointer rather than reference.)

r=dbaron with that
Attachment #8694000 - Flags: review?(dbaron) → review+
Comment on attachment 8694001 [details] [diff] [review]
part 3 - Factor out a method to get compositor-animatable overridden properties

I guess this is reasonable, although I suspect what I really meant to do, but forgot, was to return early from the function if the animations weren't animating any properties that we can animate on the compositor.  It seems like it might be worth doing that.

But r=dbaron on this, I guess.
Attachment #8694001 - Flags: review?(dbaron) → review+
(er, maybe you do that later in the patch series; I'll see)
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #21)
> I guess this is reasonable, although I suspect what I really meant to do,
> but forgot, was to return early from the function if the animations weren't
> animating any properties that we can animate on the compositor.  It seems
> like it might be worth doing that.

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #22)
> (er, maybe you do that later in the patch series; I'll see)

I don't think that is covered later in the patch series. I take it that nsRuleNode::ComputePropertiesOverridingAnimation is expensive.

I'll work on a part 11 to add that.
Comment on attachment 8694002 [details] [diff] [review]
part 4 - Add a flag to EffectSet to mark when the cascade needs to be updated

If you're going to add a Contains test to AddEffect and RemoveEffect,
it seems better to structure it as (in AddEffect):

  if (mEffects.Contains(&aEffect)) {
    return;
  }

  mCascadeNeedsUpdate = true;
  mEffects.PutEntry(&aEffect);

and similar for RemoveEffect.  It seems like it will make more sense
if you want to add more code in the future.

(Maybe also use MarkCascadeNeedsUpdate() rather than setting to true
directly?)


>+  // Set to false any time the set of effects is changed or when

Set to true, I think.
Attachment #8694002 - Flags: review?(dbaron) → review+
This restores the code removed in part 3 but adjusts it to iterate over
an effect set instead of an AnimationCollection. It also adds an early return
for the case where no compositor-animatable properties are found.


This is to address comment 21. We could also write this somewhat more simply as
something like:


  nsAutoTArray<nsCSSProperty, LayerAnimationInfo::kRecords> propertiesToTrack;
  for (const LayerAnimationInfo::Record& record :
       LayerAnimationInfo::sRecords) {
    propertiesToTrack.AppendElement(record.mProperty);
  }

  bool hasCompositorAnimatableProperties = false;
  for (KeyframeEffectReadOnly* effect : EffectSet) {
    if (effect->HasAnimationOfProperties(propertiesToTrack,
                                         LayerAnimationInfo::kRecords)) {
      hasCompositorAnimatableProperties = true;
      break;
    }
  }
  if (!hasCompositorAnimatableProperties) {
    return;
  }

However, that would involve additional iterations of each effect's property
array and would also mean we end up passing the full set of
compositor-animatable properties to ComputePropertiesOverridingAnimation
in cases where we only need to pass a subset.
Attachment #8695647 - Flags: review?(dbaron)
Drop an unnecessary check for IsEmpty
Attachment #8697122 - Flags: review?(dbaron)
Attachment #8694504 - Attachment is obsolete: true
Attachment #8694504 - Flags: review?(dbaron)
I'd like to update parts 8 and 9 so that we still update the cascade when we don't have a style context.
Attachment #8697122 - Attachment is obsolete: true
Attachment #8697122 - Flags: review?(dbaron)
Attachment #8694505 - Attachment is obsolete: true
Attachment #8694505 - Flags: review?(dbaron)
Comment on attachment 8694503 [details] [diff] [review]
part 7 - Add a method to Animation to indicate if it applies to the transitions level of the cascade

r=dbaron, although I don't really remember if we agreed that we'd stop writing "virtual" next to methods also marked "override"
Attachment #8694503 - Flags: review?(dbaron) → review+
Comment on attachment 8697695 [details] [diff] [review]
part 8 - Add EffectCompositor::(Maybe)UpdateCascadeResults

I find passing dom::Element by reference rather than pointer a bit odd.
We have conventions about which types are passed by reference vs. pointer,
and dom::Element is passed by pointers (like all XPCOM objects).  So I'd
prefer switching dom::Element& to dom::Element*.

Having to call GetEffectSet twice when going through
MaybeUpdateCascadeResults is a little annoying.  Maybe it's worth having
two versions of UpdateCascadeResults, one of which also takes an
EffectSet (and is called by the other)?

The phrases "CompositeOrder" and "CompositorOrder" seem a little odd
to me -- and it also seems odd that you're using one in some places and
the other in some places.  Maybe at least
EffectCompositorOrderComparator should be
EffectCompositeOrderComparator?

It's not clear to me how many of the differences from the current code are
intentional.  You perhaps need to update the large comment in "struct
AnimationProperty" if any of them are.

But since you didn't mention them:

>+    for (AnimationProperty& prop : effect->Properties()) {
>+
>+      bool winsInCascade = !animatedProperties.HasProperty(prop.mProperty) &&
>+                           inEffect;

It seems like this should immediately bail if prop.mProperty isn't
one of the two properties that we animate on the compositor (you can
test nsCSSProps::PropHasFlags(prop.mProperty,
CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR).

>+        // We need to update animatedProperties so that transitions will
>+        // not fire on these properties.

This comment doesn't make sense.  When a transition and a CSS animation
are present, the transition is supposed to win, and the old
mWinsInCascade computation code did that.  In this case, it seems to
produce a different result.

It seems like the merging of the computation for both animations and
transitions into this single loop -- at least the way you've done it --
breaks the computation for transitions, since transitions *do* win in
the cascade when there's an animation on the same property and element,
yet they're lower in composite order.  That's what the wins-in-cascade
computation is intended to do.  (I'm not sure it's particularly well
tested... but I thought we at least had a test or two.  Maybe not,
though.)


>+          !effect->GetAnimation()->AppliesToTransitionsLevel() &&

Likewise, I'm a little puzzled by this test, although it's really just
part of the above concern.



I'm also confused by Animation::HasLowerCompositeOrderThan not being
anti-reflexive, given that the virtual method is overridden on the
this parameter but not based on the aOther parameter.  It seems like
that will produce a non-stable sort.


There are a few things in here I didn't look at carefully, although I
think this is probably the bulk of my concerns.

Maybe I'm missing something above -- but I'd like to either see a revised
patch or understand what it is that you're trying to change here and
what it is that you're trying to keep the same.
Attachment #8697695 - Flags: review?(dbaron) → review-
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #30)
> Comment on attachment 8694503 [details] [diff] [review]
> part 7 - Add a method to Animation to indicate if it applies to the
> transitions level of the cascade
> 
> r=dbaron, although I don't really remember if we agreed that we'd stop
> writing "virtual" next to methods also marked "override"

Someone added it to the coding style so I guess we did.

"Method declarations must use at most one of the following keywords: virtual, override, or final."
(https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style)
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #31)
> Comment on attachment 8697695 [details] [diff] [review]
> part 8 - Add EffectCompositor::(Maybe)UpdateCascadeResults
> 
> I find passing dom::Element by reference rather than pointer a bit odd.
> We have conventions about which types are passed by reference vs. pointer,
> and dom::Element is passed by pointers (like all XPCOM objects).  So I'd
> prefer switching dom::Element& to dom::Element*.

Fixed locally.

> Having to call GetEffectSet twice when going through
> MaybeUpdateCascadeResults is a little annoying.  Maybe it's worth having
> two versions of UpdateCascadeResults, one of which also takes an
> EffectSet (and is called by the other)?

Ok, I'll rework it along those lines.

> The phrases "CompositeOrder" and "CompositorOrder" seem a little odd
> to me -- and it also seems odd that you're using one in some places and
> the other in some places.  Maybe at least
> EffectCompositorOrderComparator should be
> EffectCompositeOrderComparator?

That's a bug. They should both be CompositeOrder. Fixed locally.

> It's not clear to me how many of the differences from the current code are
> intentional.  You perhaps need to update the large comment in "struct
> AnimationProperty" if any of them are.
> 
> But since you didn't mention them:
> 
> >+    for (AnimationProperty& prop : effect->Properties()) {
> >+
> >+      bool winsInCascade = !animatedProperties.HasProperty(prop.mProperty) &&
> >+                           inEffect;
> 
> It seems like this should immediately bail if prop.mProperty isn't
> one of the two properties that we animate on the compositor (you can
> test nsCSSProps::PropHasFlags(prop.mProperty,
> CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR).
> 
> >+        // We need to update animatedProperties so that transitions will
> >+        // not fire on these properties.
> 
> This comment doesn't make sense.  When a transition and a CSS animation
> are present, the transition is supposed to win, and the old
> mWinsInCascade computation code did that.  In this case, it seems to
> produce a different result.

I must be missing something here. When both a transition and a CSS animation are being applied to the same property, the *animation* should win.

The transitions spec says,

"Implementations must add this value to the cascade if and only if that property is not currently undergoing a CSS Animation ([CSS3-ANIMATIONS]) on the same element."
(https://drafts.csswg.org/css-transitions/#application)

The existing code that does that is here:
https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/layout/style/nsTransitionManager.cpp#849

That's the functionality this code is reproducing. This code iterates in reverse composite order so it begins with CSS animations and adds them to the animatedProperties set so that when we get to the transitions we will set winsInCascade to false if there is a CSS animation running on the given property.

That's also why we *don't* "immediately bail if prop.mProperty isn't one of the two properties that we animate on the compositor". mWinsInCascade serves two purposes:

* Determining which animations to set to the compositor,
* Ensuring we *don't* apply transitions when there is a CSS animation on the
  same property since otherwise the transition would clobber the CSS animation
  because it applies to the transitions sheet.

The second part applies to main thread animations too so we can't bail just because the property isn't compositor-animatable.

> It seems like the merging of the computation for both animations and
> transitions into this single loop -- at least the way you've done it --
> breaks the computation for transitions, since transitions *do* win in
> the cascade when there's an animation on the same property and element,
> yet they're lower in composite order.  That's what the wins-in-cascade
> computation is intended to do.  (I'm not sure it's particularly well
> tested... but I thought we at least had a test or two.  Maybe not,
> though.)

Yes we do have tests for this (e.g. [1], [2]) and this patch passes those tests (or at least, this patch plus part 9 which actually exercises this code).

Transitions should *not* win in the cascade in this case despite the fact that they apply to the transitions sheet which has a higher priority.

[1] https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/layout/style/test/test_animations.html#2019
[2] https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/layout/style/test/test_animations_omta.html#2209

> I'm also confused by Animation::HasLowerCompositeOrderThan not being
> anti-reflexive, given that the virtual method is overridden on the
> this parameter but not based on the aOther parameter.  It seems like
> that will produce a non-stable sort.

Yes, we need to rework HasLowerCompositeOrderThan to handle generic animations (as mentioned [1]).

[1] https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/layout/style/nsAnimationManager.cpp#134

I've filed bug 1234095 for that. I believe that as long as we only have CSS animations and transitions the current code is ok.

> Maybe I'm missing something above -- but I'd like to either see a revised
> patch or understand what it is that you're trying to change here and
> what it is that you're trying to keep the same.

The behavior here should be identical. The difference is we are iterating over the whole set of transitions and animations in bulk.

I'm pretty sure this code preserves the existing behavior that animations win over transitions when they apply at the same time. What am I missing?
Flags: needinfo?(dbaron)
Attachment #8697695 - Attachment is obsolete: true
Attachment #8694003 - Attachment is obsolete: true
Attachment #8694004 - Attachment is obsolete: true
Attachment #8694496 - Attachment is obsolete: true
Attachment #8694496 - Flags: review?(dbaron)
Attachment #8701681 - Attachment is obsolete: true
Attachment #8701682 - Attachment is obsolete: true
Comment on attachment 8700446 [details] [diff] [review]
part 8 - Add EffectCompositor::(Maybe)UpdateCascadeResults

This also needs to update comment for struct AnimationProperty, removing:

> // **NOTE**: For CSS animations, we only bother setting mWinsInCascade
> // accurately for properties that we can animate on the compositor.
> // For other properties, we make it always be true.



Could you add an assertion to EffectCompositeOrderComparator::LessThan
that either they're equal or the reverse HasLowerCompositeOrderThan
is the opposite result?



It seems like this patch does change the behavior (but to match the
spec, by not running the transition) in the case of:

@keyframes animate_color {
  from, to { transform: translateX(0) }
}

p {
  transform: translateX(50px) ! important;
  transition: transform 2s;
  animation: animate_transform;
}

p:hover {
  transform: translateX(100px) ! important;
}

I guess it's an edge case, although I feel like our existing behavior
might be better than the spec's.


>+      if (winsInCascade) {
>+        animatedProperties.AddProperty(prop.mProperty);
>+      }

Maybe move this before the if that makes winsInCascade false, so
that you don't need to duplicate it inside that if?

(Then again, if we wanted to not change the above case, you could drop
the duplication without moving it.)

r=dbaron with that
Attachment #8700446 - Flags: review?(dbaron) → review+
Comment on attachment 8697696 [details] [diff] [review]
part 9 - Use EffectCompositor::UpdateCascadeResults

>diff --git a/layout/style/nsAnimationManager.cpp b/layout/style/nsAnimationManager.cpp

>   if (newAnimations.IsEmpty()) {
>     if (collection) {
>       // There might be transitions that run now that animations don't
>       // override them.
>-      mPresContext->TransitionManager()->
>-        UpdateCascadeResultsWithAnimationsToBeDestroyed(collection);
>-
>+      EffectCompositor::UpdateCascadeResults(*aElement,
>+                                             aStyleContext->GetPseudoType(),
>+                                             aStyleContext);
>       collection->Destroy();
>     }
>     return nullptr;
>   }

What makes this keep working?  In particular, what causes us to ignore the animations that we're about to destroy (but haven't yet) here?
(In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #40)
> Comment on attachment 8700446 [details] [diff] [review]
> part 8 - Add EffectCompositor::(Maybe)UpdateCascadeResults
...
> It seems like this patch does change the behavior (but to match the
> spec, by not running the transition) in the case of:
> 
> @keyframes animate_color {
>   from, to { transform: translateX(0) }
> }
> 
> p {
>   transform: translateX(50px) ! important;
>   transition: transform 2s;
>   animation: animate_transform;
> }
> 
> p:hover {
>   transform: translateX(100px) ! important;
> }
> 
> I guess it's an edge case, although I feel like our existing behavior
> might be better than the spec's.
> 
> 
> >+      if (winsInCascade) {
> >+        animatedProperties.AddProperty(prop.mProperty);
> >+      }
> 
> Maybe move this before the if that makes winsInCascade false, so
> that you don't need to duplicate it inside that if?
> 
> (Then again, if we wanted to not change the above case, you could drop
> the duplication without moving it.)

The trouble is that the above would only work for compositor-animatable properties since we only populate overriddenProperties with compositor-animatable properties.

So for now I think I'll keep this simple, removing the duplication you pointed out but matching the spec in terms of behavior. If we want to do the more useful behavior of allowing transitions when the animation is overridden, then I'd rather do it in a separate bug where we do it properly and fix the spec at the same time.
Flags: needinfo?(dbaron)
Attachment #8697696 - Attachment is obsolete: true
Attachment #8697696 - Flags: review?(dbaron)
Comment on attachment 8703980 [details] [diff] [review]
part 9 - Use EffectCompositor::UpdateCascadeResults

Sorry, that review request was supposed to be for dbaron but I kept getting errors with bzexport when I set the review to dbaron due to the non-ASCII character in dbaron's Bugzilla name. Specifically, I was getting:

Traceback (most recent call last):
  File "c:/mozilla-build/python/Scripts/hg", line 43, in <module>
    mercurial.dispatch.run()
  File "c:\mozilla-build\python\Lib\site-packages\mercurial\dispatch.py", line 54, in run
    sys.exit((dispatch(request(sys.argv[1:])) or 0) & 255)
  File "c:\mozilla-build\python\Lib\site-packages\mercurial\dispatch.py", line 116, in dispatch
    ret = _runcatch(req)
  File "c:\mozilla-build\python\Lib\site-packages\mercurial\dispatch.py", line 270, in _runcatch
    ui.warn(_("abort: %s\n") % inst)
UnicodeEncodeError: 'ascii' codec can't encode characters in position 130-131: ordinal not in range(128)

I traced the bug for quite a way but then I couldn't work out where mozhg.auth actually lives so I gave up and just tried using another reviewer without non-ASCII characters in their Bugzilla name.

(In reply to David Baron [:dbaron] UTC-8 from comment #41)
> Comment on attachment 8697696 [details] [diff] [review]
> part 9 - Use EffectCompositor::UpdateCascadeResults
> 
> >diff --git a/layout/style/nsAnimationManager.cpp b/layout/style/nsAnimationManager.cpp
> 
> >   if (newAnimations.IsEmpty()) {
> >     if (collection) {
> >       // There might be transitions that run now that animations don't
> >       // override them.
> >-      mPresContext->TransitionManager()->
> >-        UpdateCascadeResultsWithAnimationsToBeDestroyed(collection);
> >-
> >+      EffectCompositor::UpdateCascadeResults(*aElement,
> >+                                             aStyleContext->GetPseudoType(),
> >+                                             aStyleContext);
> >       collection->Destroy();
> >     }
> >     return nullptr;
> >   }
> 
> What makes this keep working?  In particular, what causes us to ignore the
> animations that we're about to destroy (but haven't yet) here?

We actually don't need this line at all. collection->Destroy() will cause the cascade to be marked out of date (that's why this keeps working). collection->Destroy() will result in Animation::CancelFromStyle() being called which calls KeyframeEffect::NotifyAnimationTimingUpdated().
Flags: needinfo?(dbaron)
Attachment #8703980 - Flags: review?(cam)
Comment on attachment 8703980 [details] [diff] [review]
part 9 - Use EffectCompositor::UpdateCascadeResults

Actually, Cameron would you mind looking at these last three patches?

This bug is blocking a *lot* of other work, I've been waiting a long time for reviews here, and dbaron is not currently accepting review requests.
Attachment #8703980 - Flags: review?(cam)
(In reply to Brian Birtles (:birtles) from comment #44)
> Sorry, that review request was supposed to be for dbaron but I kept getting
> errors with bzexport when I set the review to dbaron due to the non-ASCII
> character in dbaron's Bugzilla name. Specifically, I was getting:

That's bug 1227368.

I'm around through Wednesday; I just set the flag preemptively to avoid getting more review requests.  I might get to this tomorrow.
(In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #46)
> (In reply to Brian Birtles (:birtles) from comment #44)
> > Sorry, that review request was supposed to be for dbaron but I kept getting
> > errors with bzexport when I set the review to dbaron due to the non-ASCII
> > character in dbaron's Bugzilla name. Specifically, I was getting:
> 
> That's bug 1227368.
> 
> I'm around through Wednesday; I just set the flag preemptively to avoid
> getting more review requests.  I might get to this tomorrow.

Great, thanks!
When updating animations, we shouldn't unnecessarily clobber the "wins in
cascade" state of their properties since this can lead to unnecessary restyles
when we then decide we need to update the cascade.
Comment on attachment 8706798 [details] [diff] [review]
part 2 - Preserve "wins in cascade" state when updating animations

Oops, wrong bug
Attachment #8706798 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: