1133439 - split eRestyle_StyleAttribute into separate hints for animation and non-animation so PostRestyleEventCommon can distinguish

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Following bug 960465, RestyleManager::PostRestyleEventCommon will set mHavePendingNonAnimationRestyles for style changes that it is not sure aren't animation related.  We can currently separate out everything except for the SMIL changes that are in the style attribute level.  However, we can distinguish by having two separate change hints, and thus fully distinguish animations at the change hint level, which will improve the SMIL performance issue that is either not improved or worsened by bug 964065.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee029fea0fbe

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dee455c9586d

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1 - Split eRestyle_StyleAttribute into eRestyle_StyleAttribute and eRestyle_StyleAttributeAnimations

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2 - Distinguish animation-only SMIL restyles based on eRestyle_StyleAttribute_Animations in RestyleManager::PostRestyleEvent

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8566201 [details] [diff] [review]
patch 1 - Split eRestyle_StyleAttribute into eRestyle_StyleAttribute and eRestyle_StyleAttributeAnimations

>-        shell->RestyleForAnimation(this, eRestyle_StyleAttribute);
>+        // REVIEW:  Pass both eRestyle_StyleAttribute and
>+        // eRestyle_StyleAttribute_Animations since we don't know if
>+        // this style represents only the ticking of an existing
>+        // animation or whether it's a new or changed animation.
>+        shell->RestyleForAnimation(this, eRestyle_StyleAttribute |
>+                                         eRestyle_StyleAttribute_Animations);

What specifically were you looking for review feedback on? Or was this just a comment to explain to the reviewer what the code is doing?

I think it would be good to keep this comment since otherwise the code is a little surprising. Judging by the names of the hints, I would have assumed that eRestyle_StyleAttribute was a superset of eRestyle_StyleAttribute_Animations.

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8566202 [details] [diff] [review]
patch 2 - Distinguish animation-only SMIL restyles based on eRestyle_StyleAttribute_Animations in RestyleManager::PostRestyleEvent

>   if (aRestyleHint & ~(eRestyle_CSSTransitions | eRestyle_CSSAnimations |
>+                       eRestyle_StyleAttribute_Animations |
>                        eRestyle_SVGAttrAnimations)) {

Could we use eRestyle_AllHintsWithAnimations here now?

r=birtles with that change made, assuming it makes sense

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47474f11ad2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/73715117edd4

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47474f11ad2d
https://hg.mozilla.org/mozilla-central/rev/73715117edd4