In Gecko, we call UpdateEffectProperties() in nsStyleContext::GetContext() to update KeyframeEffectReadOnly::mProperties when the target element style has changed. We need to call UpdateEffectProperties() for stylo as well. https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f50328d916e3b42271e2841469234243a0de4d6

Comment on attachment 8851420 [details] Bug 1350754 - Introduce UpdateAnimationTasks to perform a bunch of animation's tasks in a SequentialTask. https://reviewboard.mozilla.org/r/123720/#review126140 ::: servo/components/style/context.rs:234 (Diff revision 2) > - UpdateAnimations(el, pseudo) => { > - unsafe { el.update_animations(pseudo.as_ref()) }; > + UpdateAnimations(el, pseudo, tasks) => { > + unsafe { el.update_animations(pseudo.as_ref(), tasks) }; Oops. This does not work for servo. We need to figure out to strip this UpdateAnimations() arm in case of servo.

Comment on attachment 8851416 [details] Bug 1350754 - Add an FFI to check that a given (pseudo-) element has any type of animations or not. https://reviewboard.mozilla.org/r/123712/#review126144 ::: commit-message-520b4:4 (Diff revision 1) > +Bug 1350754 - Add an FFI to check that a given (pseudo-) element has any type of animations or not. r?heycam, birtles > + > +If an element has any type of animations in match_elements(), we need to call > +UpdateEffectPropertis() to update KeyframeEffectReadOnly::mProperties. UpdateEffectProperties ::: servo/components/style/dom.rs:355 (Diff revision 1) > > /// Creates a task to update CSS Animations on a given (pseudo-)element. > /// Note: Gecko only. > fn update_animations(&self, _pseudo: Option<&PseudoElement>); > > + /// Returns true if the element has any kind of animations. Returns true if the element has relevant animations. Relevant animations are those animations that are affecting the element's style or are scheduled to do so in the future.

Comment on attachment 8851417 [details] Bug 1350754 - Templatize EffectCompositor::UpdateEffectProperties. https://reviewboard.mozilla.org/r/123714/#review126146 ::: dom/animation/EffectCompositor.h:119 (Diff revision 1) > // Posts an animation restyle for any elements whose animation style rule > // is out of date but for which an animation restyle has not yet been > // posted because updates on the main thread are throttled. > void PostRestyleForThrottledAnimations(); > > - // Called when the style context on the specified (pseudo-) element might > + // Called when |aStyleType| on the specified (pseudo-) element might This change makes the comment hard to follow. Perhaps we can leave it as "style context" in the abstract sense, or even just make it "computed style"? i.e. "Called then the computed style on the specified..." ::: dom/animation/EffectCompositor.cpp:1168 (Diff revision 1) > +EffectCompositor::UpdateEffectProperties<RefPtr<nsStyleContext>&>( > + RefPtr<nsStyleContext>& aStyleContext, I was a bit concerned that this was introducing additional add-ref traffic because previously we were using a raw pointer here but now we are using a RefPtr<> reference. I think it's ok though because we call this from nsStyleSet::GetContext where |result| is a RefPtr<> so we're not doing an extra implicit conversion to RefPtr here as far as I can see. I guess the template type didn't work with a raw pointer?

Comment on attachment 8851418 [details] Bug 1350754 - Use ServoComputedValuesWithParent for nsAnimationManager::UpdateAnimations(). https://reviewboard.mozilla.org/r/123716/#review126148

(In reply to Brian Birtles (:birtles) from comment #17) > ::: dom/animation/EffectCompositor.cpp:1168 > (Diff revision 1) > > +EffectCompositor::UpdateEffectProperties<RefPtr<nsStyleContext>&>( > > + RefPtr<nsStyleContext>& aStyleContext, > > I was a bit concerned that this was introducing additional add-ref traffic > because previously we were using a raw pointer here but now we are using a > RefPtr<> reference. I think it's ok though because we call this from > nsStyleSet::GetContext where |result| is a RefPtr<> so we're not doing an > extra implicit conversion to RefPtr here as far as I can see. > > I guess the template type didn't work with a raw pointer? Right.

Comment on attachment 8851415 [details] Bug 1350754 - Assert in Gecko_ElementHasCSSAnimations if pseudo element is neither ::before nor ::after. https://reviewboard.mozilla.org/r/123710/#review126182

Comment on attachment 8851416 [details] Bug 1350754 - Add an FFI to check that a given (pseudo-) element has any type of animations or not. https://reviewboard.mozilla.org/r/123712/#review126184

Comment on attachment 8851419 [details] Bug 1350754 - Convert pseudo nsIAtom to CSSPseudoElementType in Gecko_UpdateAnimations(). https://reviewboard.mozilla.org/r/123718/#review126186

Comment on attachment 8851420 [details] Bug 1350754 - Introduce UpdateAnimationTasks to perform a bunch of animation's tasks in a SequentialTask. https://reviewboard.mozilla.org/r/123720/#review126188 ::: layout/style/ServoBindings.h:201 (Diff revision 3) > void Gecko_UpdateAnimations(RawGeckoElementBorrowed aElement, > nsIAtom* aPseudoTagOrNull, > ServoComputedValuesBorrowedOrNull aComputedValues, > - ServoComputedValuesBorrowedOrNull aParentComputedValues); > + ServoComputedValuesBorrowedOrNull aParentComputedValues, > + uint8_t aTaskBits); Maybe make this |UpdateAnimationsTasks aTaskBits|, and then handle UpdateAnimationsTasks as a bitfield type by adding it to servo/components/style/build_gecko.rs using bitfield_enum()?

Comment on attachment 8851421 [details] Bug 1350754 - Call UpdateEffectProperties for stylo. https://reviewboard.mozilla.org/r/123722/#review126194

Comment on attachment 8851424 [details] Bug 1350754 - Update reftest expectation. https://reviewboard.mozilla.org/r/123726/#review126198

Comment on attachment 8851420 [details] Bug 1350754 - Introduce UpdateAnimationTasks to perform a bunch of animation's tasks in a SequentialTask. https://reviewboard.mozilla.org/r/123720/#review126188 > Maybe make this |UpdateAnimationsTasks aTaskBits|, and then handle UpdateAnimationsTasks as a bitfield type by adding it to servo/components/style/build_gecko.rs using bitfield_enum()? Thanks! constified_enum() could be used there.

A final try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7ca62de80a90ea4c62d9e74777410b56fd7c064

Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3179037f8583 Assert in Gecko_ElementHasCSSAnimations if pseudo element is neither ::before nor ::after. r=heycam https://hg.mozilla.org/integration/autoland/rev/0364df22ac8d Add an FFI to check that a given (pseudo-) element has any type of animations or not. r=birtles,heycam https://hg.mozilla.org/integration/autoland/rev/d18eba92bf43 Templatize EffectCompositor::UpdateEffectProperties. r=birtles https://hg.mozilla.org/integration/autoland/rev/8c785e07b91e Use ServoComputedValuesWithParent for nsAnimationManager::UpdateAnimations(). r=birtles https://hg.mozilla.org/integration/autoland/rev/975a6c2e3445 Convert pseudo nsIAtom to CSSPseudoElementType in Gecko_UpdateAnimations(). r=heycam https://hg.mozilla.org/integration/autoland/rev/12d18f57b695 Introduce UpdateAnimationTasks to perform a bunch of animation's tasks in a SequentialTask. r=heycam https://hg.mozilla.org/integration/autoland/rev/e78da982b74c Call UpdateEffectProperties for stylo. r=heycam https://hg.mozilla.org/integration/autoland/rev/53ff79321041 Update reftest expectation. r=heycam

https://hg.mozilla.org/mozilla-central/rev/3179037f8583 https://hg.mozilla.org/mozilla-central/rev/0364df22ac8d https://hg.mozilla.org/mozilla-central/rev/d18eba92bf43 https://hg.mozilla.org/mozilla-central/rev/8c785e07b91e https://hg.mozilla.org/mozilla-central/rev/975a6c2e3445 https://hg.mozilla.org/mozilla-central/rev/12d18f57b695 https://hg.mozilla.org/mozilla-central/rev/e78da982b74c https://hg.mozilla.org/mozilla-central/rev/53ff79321041