Closed
Bug 1397434
Opened 7 years ago
Closed 7 years ago
Remove the ActiveElementUsesStyle optimization from ActiveElementManager
Categories
(Core :: Panning and Zooming, enhancement, P3)
Core
Panning and Zooming
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.
Reporter | ||
Comment 1•7 years ago
|
||
MozReview-Commit-ID: D6nJP7lkCi3
Reporter | ||
Updated•7 years ago
|
Attachment #8905227 -
Flags: review?(emilio)
Comment 2•7 years ago
|
||
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-
Reporter | ||
Comment 3•7 years ago
|
||
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
Reporter | ||
Updated•7 years ago
|
Summary: stylo: ActiveElementUsesStyle APZ optimization broken in stylo → stylo: Implement ActiveElementUsesStyle APZ optimization for stylo
Comment 4•7 years ago
|
||
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
Assignee | ||
Comment 5•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 7•7 years ago
|
||
mozreview-review |
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
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/de5d70dd47bb
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•