1191600 - stop restyling immediately if the root of a restyle gets the same style context

Status: RESOLVED INVALID

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

If we start a restyle, and the first frame we restyle gets the same style context back, we should return eRestyle_Stop, even if the various other conditions that would prevent us from returning eRestyle_Stop in the middle of a restyle (such as the style context having a pseudo or being shared) are true.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 2

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

What if (aRestyleHint & (eRestyle_Subtree | eRestyle_Force | eRestyle_ForceDescendants))?

(And I guess this mIsRoot is now mIsRootOfRestyle.)

Does it make sense to check canStopWithStyleChange for that, or does it make sense to recheck some of the conditions that led to it being false instead?

Comment 3

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #2)
> What if (aRestyleHint & (eRestyle_Subtree | eRestyle_Force |
> eRestyle_ForceDescendants))?

That's a very good point.

> Does it make sense to check canStopWithStyleChange for that, or does it make
> sense to recheck some of the conditions that led to it being false instead?

canStopWithStyleChange may also have been set to false for reasons other than aRestyleHint having those bits, so I think we need to just re-check aRestyleHint.

Comment 4

[Cameron McCormack (:heycam)]

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

Comment 5

[Cameron McCormack (:heycam)]

Attachment: patch v2

Comment 6

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

(In reply to Cameron McCormack (:heycam) from comment #3)
> (In reply to David Baron [:dbaron] ⏰UTC-7 from comment #2)
> > What if (aRestyleHint & (eRestyle_Subtree | eRestyle_Force |
> > eRestyle_ForceDescendants))?
> 
> That's a very good point.

Did this problem cause test failures?  (Did you run this patch through try?)

If not -- that seems disturbing -- either a sign of very poor test coverage, or perhaps more likely a sign that the patch isn't actually accomplishing what it's supposed to do.

Comment 7

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

Comment on attachment 8655794 [details] [diff] [review]
patch v2

I think you should also condition this on aSelf->GetAdditionalStyleContext(0) being non-null (like the first check in ComputeRestyleResultFromFrame).

r=dbaron with that (though see needinfo in previous comment)

Comment 8

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

er, being *null*

Comment 9

[Cameron McCormack (:heycam)]

Closing this since the old style system is gone.