Bug 34029 – REGRESSION (r47444): PLT is 1% slower due to implementation of :valid and :invalid CSS selectors

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Adele Peterson]

Created attachment 47253 [details]
preliminary patch

:valid and :invalid CSS selectors slows down pages that make no use of the new feature

<rdar://problem/7169464>

[Adele Peterson]

Created attachment 47254 [details]
preliminary patch

[Adele Peterson]

Created attachment 47255 [details]
preliminary patch

[Adele Peterson]

ignore the patches above- this approach doesn't save much because of how common maxLength is

[Adele Peterson]

Created attachment 47360 [details]
patch

I'm still running tests, but I'm posting this for review anyway.  I set a breakpoint and verified with this patch, I won't hit the validity checks at all while running the PLT.  The tests I'm constructing now involve adding and removing valid/invalid pseudostyles and making sure that style sharing happens in the right circumstances.

[Adele Peterson]

Comment on attachment 47360 [details]
patch

Also, considerValidity is not a great name - better ideas?

[Darin Adler]

Comment on attachment 47360 [details]
patch

> +                if (!m_element->document()->considerValidity())

For boolean member functions we try to make ones that fit in a sentence "document <xxx>" so this function needs a different name, one that is not a verb phrase. Maybe "containsValidityStyleRules" or reverse the sense and have it be "containsNoValidityStyleRules" since there *may* be some rules, but there may no longer be. I hope there's a good, relatively short name. I chose the word "rules" over "selectors" in my name because of length. I'm not sure "contains" is the right word.

> +            case CSSSelector::PseudoValid: {
> +                if (!e)
> +                    return false;
> +                e->document()->setConsiderValidity();
> +                return e->willValidate() && e->isValidFormControlElement();
> +            } case CSSSelector::PseudoInvalid: {
> +                if (!e)
> +                    return false;
> +                e->document()->setConsiderValidity();
> +                return e->willValidate() && !e->isValidFormControlElement();
> +            } case CSSSelector::PseudoChecked: {

No braces needed here. It's also strange to put the end brace on the same line as the case. Not our usual style, I don't think.

review- because of the lack of a test case

[Adele Peterson]

> > +            case CSSSelector::PseudoValid: {
> > +                if (!e)
> > +                    return false;
> > +                e->document()->setConsiderValidity();
> > +                return e->willValidate() && e->isValidFormControlElement();
> > +            } case CSSSelector::PseudoInvalid: {
> > +                if (!e)
> > +                    return false;
> > +                e->document()->setConsiderValidity();
> > +                return e->willValidate() && !e->isValidFormControlElement();
> > +            } case CSSSelector::PseudoChecked: {
> 
> No braces needed here. It's also strange to put the end brace on the same line
> as the case. Not our usual style, I don't think.

That's the style used in the rest of the method for cases with multiple lines.

[Adele Peterson]

Created attachment 47434 [details]
patch

[Adele Peterson]

Committed revision 53878.