We should make the animation and transition replacement codepath in nsStyleSet::RuleNodeWithReplacement math the normal style resolution codepath (which goes through ns{Transition,Animation}Manager::RulesMatching) better. I think when I wrote this code (which landed in bug 996796) what I wrote matched what RulesMatching did, but by the time I landed it no longer matched as well, and I failed to notice. (I'm not sure about that, though, but I don't feel like digging through my patch queue history to check.)

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5ecd2a91e3b8 https://tbpl.mozilla.org/?tree=Try&rev=5ecd2a91e3b8

A try push without confounding patches: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2d0b823b4850 https://tbpl.mozilla.org/?tree=Try&rev=2d0b823b4850

I originally wrote this to see if it would fix bug 1086937, but it didn't. Note that this conflicts a bit with the patch in bug 1085769; whoever lands second will have some merging (though it shouldn't be difficult).

Comment on attachment 8512431 [details] [diff] [review] Make RuleNodeWithReplacement handle animations and transitions like RulesMatching codepath does >diff --git a/layout/style/nsStyleSet.cpp b/layout/style/nsStyleSet.cpp >--- a/layout/style/nsStyleSet.cpp >+++ b/layout/style/nsStyleSet.cpp ... > if (doReplace) { > switch (level->mLevelReplacementHint) { > case eRestyle_CSSAnimations: { >- // FIXME: This should probably be more similar to what >- // FileRules does; this feels like too much poking into the >- // internals of nsAnimationManager. >- nsPresContext* presContext = PresContext(); >- nsAnimationManager* animationManager = >- presContext->AnimationManager(); >- AnimationPlayerCollection* collection = >- animationManager->GetAnimationPlayers(aElement, aPseudoType, false); >- >- if (collection) { >- if (skipAnimationRules) { >- if (postAnimationRestyles) { >- collection->PostRestyleForAnimation(presContext); >- } >- } else { >- animationManager->UpdateStyleAndEvents( >- collection, PresContext()->RefreshDriver()->MostRecentRefresh(), >- EnsureStyleRule_IsNotThrottled); >- if (collection->mStyleRule) { >- ruleWalker.ForwardOnPossiblyCSSRule(collection->mStyleRule); >- } >+ if (aPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement || >+ aPseudoType == nsCSSPseudoElements::ePseudo_before || >+ aPseudoType == nsCSSPseudoElements::ePseudo_after) { >+ nsIStyleRule* rule = PresContext()->AnimationManager()-> >+ GetAnimationRule(aElement, aPseudoType); >+ if (rule) { >+ ruleWalker.ForwardOnPossiblyCSSRule(rule); I'm not sure about this particular change. There is a change in logic here. GetAnimationRule does not call UpdateStyleAndEvents like the code it is replacing does. I'm unclear if this code actually needs to update style or events, however. Likewise for the transitions block. I'll follow-up in person but I wanted to make a note of my question in case I forget or in case you have a chance to respond before then.

The key logic of that change is that this codepath (use of nsStyleSet::ResolveStyleWithReplacement, which calls RuleNodeWithReplacement) is a faster alternative to nsStyleSet::ResolveStyleFor (which calls nsStyleSet::FileRules, which in turn calls nsIStyleRuleProcessor::RulesMatching. nsIStyleRuleProcessor is implemented by nsAnimationManager and nsTransitionManager. So RuleNodeWithReplacement should do the same thing that the RulesMatching implementations of nsAnimationManager and nsTransitionManager. (I suspect at the time I wrote the code, I was matching what they did, but then it changed before I landed. Alternatively, I might have just done something silly.)

Comment on attachment 8512431 [details] [diff] [review] Make RuleNodeWithReplacement handle animations and transitions like RulesMatching codepath does Review of attachment 8512431 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Oct. 24-31) from comment #5) > The key logic of that change is that this codepath (use of > nsStyleSet::ResolveStyleWithReplacement, which calls > RuleNodeWithReplacement) is a faster alternative to > nsStyleSet::ResolveStyleFor (which calls nsStyleSet::FileRules, which in > turn calls nsIStyleRuleProcessor::RulesMatching. nsIStyleRuleProcessor is > implemented by nsAnimationManager and nsTransitionManager. So > RuleNodeWithReplacement should do the same thing that the RulesMatching > implementations of nsAnimationManager and nsTransitionManager. Ok, that makes sense. r=me ::: layout/style/nsTransitionManager.cpp @@ +640,5 @@ > // For print or print preview, ignore animations. > return; > } > > + RestyleManager* restyleManager = mPresContext->RestyleManager(); So now we're using the pres context associated with the transition manager where as before we used the pres context associated with the passed in ElementDependentRuleProcessorData. I assume these will always be the same?

(In reply to Brian Birtles (:birtles, busy 28 Oct~1 Nov) from comment #6) > So now we're using the pres context associated with the transition manager > where as before we used the pres context associated with the passed in > ElementDependentRuleProcessorData. I assume these will always be the same? They should always be the same.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa64f4f16a8 for b2g mochitest-9 permafail: https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3446012&repo=mozilla-inbound

It's a similar set of test failures to the ones I had to tweak a number of times before in the process of landing them in bug 996796, and that I filed bug 1039799 on. Though maybe it's like the ones that were fixed by https://hg.mozilla.org/mozilla-central/rev/5bed5f6da61c -- I'd have to check my history.

The failure is reproducable locally with: ./mach mochitest-plain --setpref layers.acceleration.force-enabled=true --setpref layers.offmainthreadcomposition.async-animations=true layout/style/test/test_animations_omta_start.html (which I realize I should have tried before landing).

This really isn't a very good name anymore, but I'm not quite sure what to call it. Any ideas?

I originally wrote this to see if it would fix bug 1086937, but it didn't. Note that this conflicts a bit with the patch in bug 1085769; whoever lands second will have some merging (though it shouldn't be difficult). The updating of the style rule is needed as part of the animation-only style update, but it shouldn't be in the general restyling code, so it has moved there (using the merged method from patch 1).

I wonder if we really need to update animation events in CommonAnimationManager::AddStyleUpdatesTo. Wouldn't it be enough to just make CommonAnimationManager::AddStyleUpdatesTo in part 2 do: collection->EnsureStyleRuleFor(now, EnsureStyleRule_IsNotThrottled); If so, then we wouldn't need part 1 either. (Long term I want to make transition event queueing more closely match animation event queueing. I will possibly tackle that in bug 1073379 or a dependency of that.)

Clearing and resetting needinfo in case my question got overlooked due to the outstanding needinfo request from comment 9 (since bugzilla doesn't allow parallel needinfo requests I was unable to set the flag in comment 14).

Yeah, I saw, I just need some time to dig in to the event code. (I was kinda hoping you'd know. :-)

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #16) > Yeah, I saw, I just need some time to dig in to the event code. (I was > kinda hoping you'd know. :-) I *think* we'd be ok not dispatching events then. For animations, anyway, we record what events we've dispatched and dispatch catch-up start/end events in the case where we skip an animation interval entirely. So even if this means we dispatch events a little less eagerly we should still be correct with regards to start/end events. Beyond that I'm not sure. For now I made up a version of what I was suggested in comment 14 to see if it triggers any failures: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=521facf14801

Yeah, it looks like we're fine not queueing events. We won't dispatch them anyway until FlushAnimations is called, either from a refresh driver tick or a flush, and we're guaranteed to enqueue them in: * a refresh driver tick, for changes that happen from time advancing * CheckAnimationRule, for changes that happen from style changes

I originally wrote this to see if it would fix bug 1086937, but it didn't. Note that this conflicts a bit with the patch in bug 1085769; whoever lands second will have some merging (though it shouldn't be difficult). The updating of the style rule is needed as part of the animation-only style update, but it shouldn't be in the general restyling code, so it has moved there.

Comment on attachment 8521006 [details] [diff] [review] Make RuleNodeWithReplacement handle animations and transitions like RulesMatching codepath does r=birtles