1058346 - add a restyle hint to restyle only SVG Animation rules

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

This is needed for bug 977991.

Comment 1

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

I actually realize I can't *use* the hint until bug 977991 is done, but I may as well keep these patches in a separate bug.

Comment 2

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

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

Comment 3

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

Attachment: patch 1 - Expose a simpler variant of RulesMatching from SVGAnimationRuleProcessor

This allows nsStyleSet::RuleNodeWithReplacement to call it without
constructing an entire (and unnecessary) ElementRuleProcessorData, which
will happen in patch 2.

Comment 4

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

Attachment: patch 2 - Add eRestyle_SVGAnimations and support it in nsStyleSet::RuleNodeWithReplacement

This allows posting a restyle that says that only the rule(s) from the
SVGAnimationSheet cascade level will be replaced, which avoids running
selector matching.

This is needed to land bug 977991 prior to landing bug 960465, since
that requires replacing all levels that contain animations.

(I'll rename this to match the name determined in bug 1057231.)

Comment 5

[Brian Birtles (:birtles)]

Comment on attachment 8479496 [details] [diff] [review]
patch 1 - Expose a simpler variant of RulesMatching from SVGAnimationRuleProcessor

Review of attachment 8479496 [details] [diff] [review]:
-----------------------------------------------------------------

r=birtles

Comment 6

[Brian Birtles (:birtles)]

Comment on attachment 8479497 [details] [diff] [review]
patch 2 - Add eRestyle_SVGAnimations and support it in nsStyleSet::RuleNodeWithReplacement

Review of attachment 8479497 [details] [diff] [review]:
-----------------------------------------------------------------

r=birtles although we need a test somewhere if this code is indeed not being tested.

::: layout/style/nsStyleSet.cpp
@@ +1349,5 @@
>                                      nsRestyleHint aReplacements)
>  {
>    NS_ABORT_IF_FALSE(!(aReplacements & ~(eRestyle_CSSTransitions |
> +                                        eRestyle_CSSAnimations |
> +                                        eRestyle_SVGAnimations)),

Just a curiosity here, is there any value in converting NS_ABORT_IF_FALSE to MOZ_ASSERT? I was under the impression that MOZ_ASSERT was what we should use for new code but I don't know if it's worth the churn of updating existing code.

@@ +1428,5 @@
> +              mRuleProcessors[eSVGAnimationSheet].get());
> +          if (ruleProcessor &&
> +              aPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement) {
> +            ruleProcessor->ElementRulesMatching(aElement, &ruleWalker);
> +          }

I assume this is valid because SVG animation never targets pseudo elements. I wonder if this is worth an assertion anyway? (In case SVG grows a way of targeting pseudo-elements, or we reuse this mechanism for animations generated via the Web Animations API.) Or have I misunderstood something about how this is called?

Also, I set a breakpoint on the call to ElementRulesMatching above and ran all the mochitests in dom/smil/test and the reftests in layout/reftests/svg/smil/ and it wasn't hit. Do you know if this code is tested anywhere? Perhaps in one of the other bugs in this series? If not, I think we need a test that exercises this line.

Comment 7

[Brian Birtles (:birtles)]

(In reply to David Baron [:dbaron] (UTC+2) (needinfo? for questions) (away/busy until Sep 11) from comment #1)
> I actually realize I can't *use* the hint until bug 977991 is done, but I
> may as well keep these patches in a separate bug.

Oh, that would explain why the lines mentioned in comment 6 aren't tested. Please ignore that comment.

Comment 8

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

(In reply to Brian Birtles (:birtles) from comment #6)
> Comment on attachment 8479497 [details] [diff] [review]
> Just a curiosity here, is there any value in converting NS_ABORT_IF_FALSE to
> MOZ_ASSERT? I was under the impression that MOZ_ASSERT was what we should
> use for new code but I don't know if it's worth the churn of updating
> existing code.

I don't think it matters much which one we use.

> I assume this is valid because SVG animation never targets pseudo elements.
> I wonder if this is worth an assertion anyway? (In case SVG grows a way of
> targeting pseudo-elements, or we reuse this mechanism for animations
> generated via the Web Animations API.) Or have I misunderstood something
> about how this is called?

Yes, that's true.  I added some comments to the relevant SVGAttrAnimationRuleProcessor::RulesMatching methods to point out that this could would need updating if it ever did do something for pseudo-elements.

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cc74bf25e33
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4ed770bf60a

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4cc74bf25e33
https://hg.mozilla.org/mozilla-central/rev/b4ed770bf60a