Closed Bug 1133439 Opened 5 years ago Closed 5 years ago

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


(Core :: CSS Parsing and Computation, defect)

Not set



Tracking Status
firefox38 --- fixed


(Reporter: dbaron, Assigned: dbaron)



(2 files)

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 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.
Attachment #8566201 - Flags: review?(bbirtles) → review+
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
Attachment #8566202 - Flags: review?(bbirtles) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.