Bug 996796 patch 20 adds the following comment to RestyleManager::RebuildAllStyleData: // FIXME: Many of the callers probably don't need eRestyle_Subtree // because they're changing things that affect data computation rather // than selector matching; we could have a restyle hint passed in, and // substantially improve the performance of things like pref changes // and the restyling that we do for downloadable font loads. This bug covers doing this, i.e., improving the performance of callers of RebuildAllStyleData by passing in an appropriate nsRestyleHint for calls that change style data without changing which selectors match.

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

I confirmed locally that removing the patch https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/bcaafe07840a/optimize-mathml-sheet from the patch queue fixes the pair of assertions: ###!!! ASSERTION: aElement should be the element and not the pseudo-element: 'pseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement || !elementForAnimation->GetPrimaryFrame() || elementForAnimation->GetPrimaryFrame()->StyleContext()-> GetPseudoType() == nsCSSPseudoElements::ePseudo_NotPseudoElement', file /builds/slave/try-l64-d-00000000000000000000/build/layout/style/nsStyleSet.cpp, line 1602 leading to: REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/374420.xhtml | assertion count 2 is more than expected 0 assertions I'm just going to remove that patch from the queue since it's among the less important. (I think the most likely explanation might be that nothing else causes a restyle when we add the sheet -- which in turn means that the current code is correct.)

This patch is not intended to contain any changes in behavior.

This patch is not intended to contain any changes in behavior. The behavior changes for these callers are in the following patch.

This patch is not intended to contain any changes in behavior.

This patch is not intended to contain any changes in behavior. The behavior changes for these callers are in the following 2 patches.

This patch is not intended to contain any changes in behavior. The behavior changes in these callers are in the following 4 patches.

To see the end results of which FIXMEs I remove, and which changes I later optimize further.

Comment on attachment 8500069 [details] [diff] [review] patch 4 - Don't rerun selector matching for preference, charset, or system color changes >+++ b/layout/base/nsPresContext.cpp >+ // Preferences don't change the results of selector matching; they >+ // only change the computed style data. This means we can pass >+ // nsRestyleHint(0). Is that really true? Say the link color preference gets changed. This is implemented by blowing away the presshell's mPrefStyleSheet and creating a new one, with rules that are based on the new value of the preference. Won't we fail to pick that up if we don't rerun selector matching and thus continue to use a rulenode corresponding to the old rule we used to have? r- for this part of the patch. For the other two parts, why is nsRestyleHint(0) ok? Why do we not need eRestyle_ForceDescendants? It's worth documenting exactly what's going on there, because it's not clear to me.

Comment on attachment 8500070 [details] [diff] [review] patch 5 - Pass restyle hint to RestyleManager::PostRebuildAllStyleDataEvent r=me

Comment on attachment 8500073 [details] [diff] [review] patch 8 - Don't rerun selector matching when @counter-style rules change r=me, though perhaps document why this needs to be eRestyle_ForceDescendants, not nsRestyleHint(0).

(In reply to Boris Zbarsky [:bz] from comment #18) > Comment on attachment 8500069 [details] [diff] [review] > patch 4 - Don't rerun selector matching for preference, charset, or system > color changes > For the other two parts, why is nsRestyleHint(0) ok? Why do we not need > eRestyle_ForceDescendants? It's worth documenting exactly what's going on > there, because it's not clear to me. RebuildAllStyleData adds eRestyle_ForceDescendants internally before passing it off to other code. The only places where I'm explicitly passing eRestyle_ForceDescendants are to MediaFeatureValuesChanged, which I guess deserves a clearer comment at the declaration of MediaFeatureValuesChanged. How about: The sensible values to pass for aRestyleHint are: * nsRestyleHint(0) to rebuild style data, with rerunning of selector matching, only if media features have changed * eRestyle_ForceDescendants to force rebuilding of style data (but still only rerun selector matching if media features have changed). (RebuildAllStyleData always adds eRestyle_ForceDescendants internally, so here we're only using it to distinguish from nsRestyleHint(0) whether we need to call RebuildAllStyleData at all.) * eRestyle_Self to force rebuilding of style data with rerunning of selector matching

(In reply to Boris Zbarsky [:bz] from comment #20) > Comment on attachment 8500073 [details] [diff] [review] > patch 8 - Don't rerun selector matching when @counter-style rules change > > r=me, though perhaps document why this needs to be eRestyle_ForceDescendants, > not nsRestyleHint(0). Oops, it doesn't. And neither does patch 7. (I had it as nsRestyleHint(0) originally, then decided to change it, but I should probably change it back.)

> RebuildAllStyleData adds eRestyle_ForceDescendants internally Ah, in DoRebuildAllStyleData outside the patch context, I see. I think it's worth documenting that the only values it makes sense to pass to RebuildAllStyleData are nsRestyleHint(0) and eRestyle_Subtree, right? > * eRestyle_Self to force rebuilding of style data with rerunning of selector matching You mean eRestyle_Subtree? Yes, documenting this for MediaFeatureValuesChanged would make sense.

Comment on attachment 8501760 [details] [diff] [review] patch 4 - Don't rerun selector matching for charset or system color changes Do add the docs about what sort of values make sense to be passed to RebuildAllStyleData?

Comment on attachment 8501761 [details] [diff] [review] patch 4a - Explain why we need to rerun selector matching for preference changes r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/05102e8d7362 https://hg.mozilla.org/integration/mozilla-inbound/rev/7f4682912226 https://hg.mozilla.org/integration/mozilla-inbound/rev/d6c1291b371e https://hg.mozilla.org/integration/mozilla-inbound/rev/7aa21afed324 https://hg.mozilla.org/integration/mozilla-inbound/rev/fd1674f6a37a https://hg.mozilla.org/integration/mozilla-inbound/rev/56ee0c224417 https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f6fe019942 https://hg.mozilla.org/integration/mozilla-inbound/rev/a1bc385f0ad4 https://hg.mozilla.org/integration/mozilla-inbound/rev/6e95d6d112f3 https://hg.mozilla.org/integration/mozilla-inbound/rev/7b05dc3f7ae9 https://hg.mozilla.org/integration/mozilla-inbound/rev/14e9ff2f94f7 https://hg.mozilla.org/integration/mozilla-inbound/rev/56ac899771e6 https://hg.mozilla.org/integration/mozilla-inbound/rev/096b2b23db0d https://hg.mozilla.org/integration/mozilla-inbound/rev/fd52f15d25d3 https://hg.mozilla.org/integration/mozilla-inbound/rev/5839fbd7b8c6

https://hg.mozilla.org/mozilla-central/rev/05102e8d7362 https://hg.mozilla.org/mozilla-central/rev/7f4682912226 https://hg.mozilla.org/mozilla-central/rev/d6c1291b371e https://hg.mozilla.org/mozilla-central/rev/7aa21afed324 https://hg.mozilla.org/mozilla-central/rev/fd1674f6a37a https://hg.mozilla.org/mozilla-central/rev/56ee0c224417 https://hg.mozilla.org/mozilla-central/rev/d4f6fe019942 https://hg.mozilla.org/mozilla-central/rev/a1bc385f0ad4 https://hg.mozilla.org/mozilla-central/rev/6e95d6d112f3 https://hg.mozilla.org/mozilla-central/rev/7b05dc3f7ae9 https://hg.mozilla.org/mozilla-central/rev/14e9ff2f94f7 https://hg.mozilla.org/mozilla-central/rev/56ac899771e6 https://hg.mozilla.org/mozilla-central/rev/096b2b23db0d https://hg.mozilla.org/mozilla-central/rev/fd52f15d25d3 https://hg.mozilla.org/mozilla-central/rev/5839fbd7b8c6