Closed Bug 1397434 Opened 2 years ago Closed 2 years ago

Remove the ActiveElementUsesStyle optimization from ActiveElementManager

Categories

(Core :: Panning and Zooming, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: bholley, Assigned: botond)

Details

Attachments

(2 files)

See bug 1385789 comment 18. It turns out we have the right machinery to implement this, it's just not entirely hooked up. I've got a patch.
Attachment #8905227 - Flags: review?(emilio)
Comment on attachment 8905227 [details] [diff] [review]
Hook up ServoRestyleManager::HasStateDependentStyle and eliminate HasStateDependency. v1

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

This is not what this API expects, that's why we don't implement HasStateDependentStyle.

This will return true if there's any :active selector in any stylesheet potentially applying to the element, so this would be somewhat useless.

This is somewhat related to bug 1374135, but for :active.

One approach to fix HasStateDependentStyle is to do it the way Gecko does, which is walking the rules (and would be equivalent to running the invalidation stuff).

However the code wants to check whether any ancestor on the parent chain has any :active-dependent style, so this can get somewhat expensive.

The way to fix this properly is recording during selector-matching in MatchingContext whether we've tried to match any :active pseudo-class and, if so, set a bit on the ComputedValues, and look at these when being queried. Let me know if you want me to look into this / it is urgent, shouldn't be extremely hard.
Attachment #8905227 - Flags: review?(emilio) → review-
Ok. Let's at least make things correct in bug 1397500, then we can implement this if we have time.

See bug 1385789 comment 18 for a description of what this optimization provides.
Assignee: bobbyholley → nobody
Priority: P2 → P3
Summary: stylo: ActiveElementUsesStyle APZ optimization broken in stylo → stylo: Implement ActiveElementUsesStyle APZ optimization for stylo
Given bug 1385789 comment 18, it seems to me that removing is also an option since this optimization doesn't seem to be necessary anymore.
Summary: stylo: Implement ActiveElementUsesStyle APZ optimization for stylo → stylo: Implement ActiveElementUsesStyle APZ optimization for stylo or remove it
Further to an IRC conversation with Emilio [1] about how this optimization may not be the best idea given that the style check involved is expensive, I discussed this with Kats and we're just going to remove the APZ optimization.

I'm going to re-purpose this bug to do that.

[1] http://logs.glob.uno/?c=mozilla%23developers&s=6+Sep+2017&e=6+Sep+2017#c1652683
Assignee: nobody → botond
Component: CSS Parsing and Computation → Panning and Zooming
Summary: stylo: Implement ActiveElementUsesStyle APZ optimization for stylo or remove it → Remove the ActiveElementUsesStyle optimization from ActiveElementManager
Comment on attachment 8906762 [details]
Bug 1397434 - Remove the ActiveElementUsesStyle optimization in ActiveElementManager.

https://reviewboard.mozilla.org/r/178486/#review183520
Attachment #8906762 - Flags: review?(bugmail) → review+
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de5d70dd47bb
Remove the ActiveElementUsesStyle optimization in ActiveElementManager. r=kats
https://hg.mozilla.org/mozilla-central/rev/de5d70dd47bb
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.