Bug 69838 – Resolve regular and visited link style in a single pass

• Component: CSS
• Status: RESOLVED FIXED

[Antti Koivisto]

We can simplify and speed up selector matching by removing the recursive matching done to generate the style for :visited pseudo selector. Both regular and visited link style can be generated in a single pass though the style selector.

Recursive visited style mathing is ~20% of the total time spent under styleForElement.

[Antti Koivisto]

Created attachment 110545 [details]
patch

The next step would be to get rid of the separate RenderStyle object for the visited style alltogether. The few fields used can be part of the regular RenderStyle.

[Darin Adler]

Comment on attachment 110545 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110545&action=review

I read over this and did not spot any mistakes.

> Source/WebCore/css/CSSStyleSelector.cpp:210
> -    unsigned m_position : 28;
> +    unsigned m_position : 26;

How many bits do we really need for this? Can it be even less than 26? How do we handle overflow?

> Source/WebCore/css/CSSStyleSelector.cpp:1292
> +        // pseudo element styles will continue to work for pseudo elements inside :visited
> +        // links.

I’d put that last word on the previous line.

> Source/WebCore/css/SelectorChecker.h:90
> +    enum LinkMatchMask {  MatchLink = 1, MatchVisited = 2, MatchAll = MatchLink | MatchVisited };

Extra space here before MatchLink.

> Source/WebCore/rendering/style/RenderStyle.cpp:260
> +    ASSERT(pseudo->styleType() > NOPSEUDO);

Might be worth a “why this is a good assertion” comment.

[Antti Koivisto]

(In reply to comment #2)
> (From update of attachment 110545 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110545&action=review
> 
> I read over this and did not spot any mistakes.
> 
> > Source/WebCore/css/CSSStyleSelector.cpp:210
> > -    unsigned m_position : 28;
> > +    unsigned m_position : 26;
> 
> How many bits do we really need for this? Can it be even less than 26? How do we handle overflow?

We need enough bits to hold the total count of all (author) style rules that may apply to a document. In practice we can do with way less bits than this still. Overflow is not handled. It is not particularly dangerous (just messes up the style resolution) and we are going to be in trouble with millions of rules well before that anyway.

[Antti Koivisto]

http://trac.webkit.org/changeset/97248

[Balazs Kelemen]

Our Qt-Debug-32bit bot has a css related assertion fail and I find this patch as the most likely reason. Could you take a look to the crash log in #69933? Thanks.

[Ryosuke Niwa]

svg/hixie/cascade/002.xml started failing on GTK after this patch:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=svg%2Fhixie%2Fcascade%2F002.xml

[Antti Koivisto]

(In reply to comment #6)
> svg/hixie/cascade/002.xml started failing on GTK after this patch:
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=svg%2Fhixie%2Fcascade%2F002.xml

Filed https://bugs.webkit.org/show_bug.cgi?id=70122 for this. Seems like our visited link test coverage is actually really bad as drt doesn't support them properly.

[Ryosuke Niwa]

(In reply to comment #7)
> (In reply to comment #6)
> > svg/hixie/cascade/002.xml started failing on GTK after this patch:
> > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=svg%2Fhixie%2Fcascade%2F002.xml
> 
> Filed https://bugs.webkit.org/show_bug.cgi?id=70122 for this. Seems like our visited link test coverage is actually really bad as drt doesn't support them properly.

Thanks! Is it possible to add some internals method to improve the test coverage?

[Antti Koivisto]

(In reply to comment #8)
> Thanks! Is it possible to add some internals method to improve the test coverage?

It just requires some simple fixes to make <a href=""> work correctly in DRT. That is always considered visited so it can be used for testing. Some tests use it already but end up with incorrect DRT results due to some easy bugs.