Summary: | REGRESSION (r47444): PLT is 1% slower due to implementation of :valid and :invalid CSS selectors | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adele Peterson <adele> | ||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | tkent | ||||||||||||
Priority: | P2 | Keywords: | InRadar, Regression | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Attachments: |
|
Description
Adele Peterson
2010-01-22 18:07:10 PST
Created attachment 47254 [details]
preliminary patch
Created attachment 47255 [details]
preliminary patch
ignore the patches above- this approach doesn't save much because of how common maxLength is 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.
Comment on attachment 47360 [details]
patch
Also, considerValidity is not a great name - better ideas?
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 > > + 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.
Created attachment 47434 [details]
patch
Committed revision 53878. |