762345 - Skip all the QIing stuff in the TreeMatchContext constructor if possible

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This was added in bug 722853 and it's showing up in querySelector profiles.

Can we not skip all this stuff based on our visited handling value?

Comment 1

[(no longer active)]

What's the exact semantics of mVisitedHandling?

Comment 2

[Boris Zbarsky [:bzbarsky]]

That's the part I can't recall exactly....

But for the case of querySelector, the semantic is "treat all links as unvisited".

Comment 3

[(no longer active)]

Does that only happen when we're in PB mode then?  Or can it also be on as a result of a pref being set, etc.?

Comment 4

[Boris Zbarsky [:bzbarsky]]

What do you mean?

The point is that querySelector(":visited") will always return null, no matter what's going on with PB mode.  So there's no point checking for the PB mode stuff under querySelector, since it always treats all links as unvisited anyway, no matter what.

Comment 5

[(no longer active)]

Attachment: Patch (v1)

Like this?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 631078 [details] [diff] [review]
Patch (v1)

The if check is backwards, no?

Comment 7

[(no longer active)]

Attachment: Patch (v2)

Indeed it is!  :-)

Which makes me think, how should I test this patch?  I ran the PB tests and the layout/style tests and everything seemed to be fine.  I guess I'll push it to try.

Comment 8

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=d5514760a513

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 631087 [details] [diff] [review]
Patch (v2)

r=me

Comment 10

[(no longer active)]

Hmm, this patch is not correct, it fails a test that we have for exactly this reason:

https://tbpl.mozilla.org/php/getParsedLog.php?id=12462342&tree=Try

Is the TreeMatchContext used for things other than querySelector?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yes, of course. It's used for all selector matching.

The real question is whether the eRelevantLinkUnvisited value is used elsewhere... and if so why ignoring private browsing there makes that elsewhere fail.

Comment 12

[(no longer active)]

(In reply to Boris Zbarsky (:bz) from comment #11)
> Yes, of course. It's used for all selector matching.
> 
> The real question is whether the eRelevantLinkUnvisited value is used
> elsewhere... and if so why ignoring private browsing there makes that
> elsewhere fail.

Some of the TreeMatchContext's in nsStyleSet.cpp are constructed using eRelevantLinkUnvisited, which is why this happens, I think.

Given that, can we really avoid accessing the docshell here?

Comment 13

[Boris Zbarsky [:bzbarsky]]

For the querySelector case, absolutely.  We only need to touch the docshell to find out whether it's OK to tell the consumer about visitedness stuff, but querySelector doesn't expose that to start with...

Comment 14

[(no longer active)]

So, is it find if I add a new argument to the TreeMatchContext's ctor which is set by the caller to ask for this check to be skipped?

Comment 15

[Boris Zbarsky [:bzbarsky]]

It would be fine by my, esp, seeing as the constructor is inlined so the compiler could optimize this pretty well.

Comment 16

[(no longer active)]

Attachment: Patch (v3)

Comment 17

[(no longer active)]

http://tbpl.mozilla.org/?tree=Try&rev=a1253bedc70a

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 631260 [details] [diff] [review]
Patch (v3)

r=me

Comment 19

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e87b81b5796f

Comment 20

[(no longer active)]

Comment on attachment 631260 [details] [diff] [review]
Patch (v3)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 722853
User impact if declined: perf regression
Testing completed (on m-c, etc.): locally and test suites being run
Risk to taking this patch (and alternatives if risky): minimal
String or UUID changes made by this patch: none

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e87b81b5796f

Comment 22

[Alex Keybl [:akeybl]]

Comment on attachment 631260 [details] [diff] [review]
Patch (v3)

[Triage Comment]
Low risk perf fix for a FF14 regression. Approved for Aurora 15 and Beta 14.

Comment 23

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/9421cb7212c3
http://hg.mozilla.org/releases/mozilla-beta/rev/3a950b9f6fdc