1363244 - stylo: Optimize Servo_HasAuthorSpecifiedRules to avoid calling lazy_pseudo_rules when possible

Status: RESOLVED DUPLICATE of bug 1364412

(Core :: CSS Parsing and Computation, enhancement)

Description

[Matt Brubeck (:mbrubeck)]

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.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: Bug 1363244 - stylo: Avoid calling lazy_pseudo_rules when possible.

Review commit: https://reviewboard.mozilla.org/r/138600/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138600/

Comment 2

[Matt Brubeck (:mbrubeck)]

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`.

Comment 3

[Bobby Holley (:bholley)]

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?

Comment 4

[Bobby Holley (:bholley)]

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?

Comment 5

[Matt Brubeck (: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

Comment 6

[Boris Zbarsky [:bzbarsky]]

Is this actually fixed?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 8

[Matt Brubeck (:mbrubeck)]

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?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Yes, I think that makes sense, since bug 1364412 also contains those new passes.