[webkit-reviews] review denied: [Bug 34029] REGRESSION (r47444): PLT is 1% slower due to implementation of :valid and :invalid CSS selectors : [Attachment 47360] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Adele Peterson <adele at apple.com>'s
request for review:
Bug 34029: REGRESSION (r47444): PLT is 1% slower due to implementation of
:valid and :invalid CSS selectors
https://bugs.webkit.org/show_bug.cgi?id=34029

Attachment 47360: patch
https://bugs.webkit.org/attachment.cgi?id=47360&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +		   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