Closed Bug 1363244 Opened 3 years ago Closed 3 years ago

stylo: Optimize Servo_HasAuthorSpecifiedRules to avoid calling lazy_pseudo_rules when possible

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1364412

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

Attachments

(1 file)

This is a follow-up to bug 1349651.  From bug 1349651 comment #28:

> In the case that IsNativeAnonymous() returns true, we should see if
> element->GetPseudoElementType() matches the pseudo on the style context. If
> so, we should just ask servo for the primary (not pseudo) rule node for that
> element.
Assignee: nobody → mbrubeck
Status: NEW → ASSIGNED
This patch caused 8 tests to pass and 1 to fail:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=727be9ca583da2809807cab0f086efb5479f1233

In the failing test "widget/reftests/progressbar-fallback-default-style.html", `elem` and `aFrame->StyleContext()` both have their pseudo type set to ":-moz-progress-bar", but has_author_style_rules does not find any border properties in the primary rule nodes for `elem`.
Hm, so this patch definitely looks right, but it's supposed to be a transparent optimization, so the fact that it changes behavior means that there's something wrong somewhere (not necessarily in this patch). Emilio, WDYT?
Flags: needinfo?(emilio+bugs)
Flags: needinfo?(emilio+bugs)
Can you retry this on top of emilio's patch in bug 1364377 (with the appropriate expectation adjustments from his try push) and see if this is now a transparent optimization?
Flags: needinfo?(mbrubeck)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4)
> Can you retry this on top of emilio's patch in bug 1364377 (with the
> appropriate expectation adjustments from his try push) and see if this is
> now a transparent optimization?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5abe1441370f18cb5b33f539f860704ad7e05466
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: needinfo?(mbrubeck)
Resolution: --- → FIXED
Is this actually fixed?
Flags: needinfo?(mbrubeck)
It's probably not. That being said, in bug 1364412 I'm simplifying a lot of this to avoid resolving style in HasAuthorSpecifiedRules at all.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
When combined with the patch from bug 1364377, this causes even more unexpected passes, and zero new failures.  (Perhaps these tests were previously failing because the original code was not resolving the correct rule node, maybe because it passed an incorrect combination of element+pseudo?).

Emilio, do you want to fold this into bug 1364412?
Flags: needinfo?(mbrubeck)
Yes, I think that makes sense, since bug 1364412 also contains those new passes.
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1364412
Attachment #8867004 - Flags: review?(bobbyholley)
You need to log in before you can comment on or make changes to this bug.