[webkit-reviews] review granted: [Bug 72933] "Raw" pseudo selectors don't match if immediately after a child or descendant combinator : [Attachment 116481] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Roland Steiner
<rolandsteiner at chromium.org>'s request for review:
Bug 72933: "Raw" pseudo selectors don't match if immediately after a child or
descendant combinator
https://bugs.webkit.org/show_bug.cgi?id=72933

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=116481&action=review


I’m going to say review+ but there are many style issues here so it’s a
borderline case.

I think this is quite a bit of copied and pasted code. Is there a way we can
avoid that?

> Source/WebCore/css/CSSStyleSelector.cpp:1799
> +    CSSSelector* sel = ruleData.selector();

Please use words like “selector” rather than abbreviations like “sel”.

> Source/WebCore/css/SelectorChecker.cpp:539
> +SelectorChecker::SelectorMatch SelectorChecker::checkPseudoIDSelector(
> +    CSSSelector* sel,
> +    Element* e,
> +    PseudoId& dynamicPseudo,
> +    VisitedMatchType visitedMatchType,
> +    RenderStyle* elementStyle,
> +    RenderStyle* elementParentStyle) const

Is this large new function a copy of the checkSelector function? If so, can we
figure out a way to refactor so we don’t need to copy the entire function?

Please don’t use this “one argument per line” formatting in WebKit code since
it’s not our normal style.

Please use words like “selector” and “element” rather than abbreviations like
“sel” and “e” unless you are preserving existing code.

The name “dynamic pseudo” is missing a noun. Perhaps “dynamic pseudo ID” would
be a better name?

> Source/WebCore/css/SelectorChecker.cpp:584
> +	       e = host;
> +	       for (;;) {
> +		   ContainerNode* n = e->parentNode();
> +		   if (!n || !n->isElementNode())
> +		       return SelectorFailsCompletely;
> +		   e = static_cast<Element*>(n);
> +		   SelectorMatch match = checkSelector(sel, e, dynamicPseudo,
false, visitedMatchType);
> +		   if (match != SelectorFailsLocally)
> +		       return match;
> +	       }

This could be written more cleanly with the parentElement function.

    while ((e = e->parentElement())

Or written as a for loop (see an example below).

> Source/WebCore/css/SelectorChecker.cpp:588
> +	   case CSSSelector::Child:
> +	       {

Our normal style puts the brace on the same line as the : instead of the next
line.

> Source/WebCore/css/SelectorChecker.cpp:592
> +		   ContainerNode* n = host->parentNode();
> +		   if (!n || !n->isElementNode())
> +		       return SelectorFailsCompletely;
> +		   e = toElement(n);

Again, parentElement() is the best way to write this.

> Source/WebCore/css/SelectorChecker.cpp:610
> +		   Node* n = host->previousSibling();
> +		   while (n && !n->isElementNode())
> +		       n = n->previousSibling();
> +		   if (!n)
> +		       return SelectorFailsCompletely;
> +		   e = static_cast<Element*>(n);

This could just be:

    e = host->previousElementSibling();
    if (!e)
	return SelectorFailsCompletely;

> Source/WebCore/css/SelectorChecker.cpp:621
> +	       if (!m_isCollectingRulesOnly && host->parentNode() &&
host->parentNode()->isElementNode()) {
> +		   RenderStyle* parentStyle =
host->parentNode()->renderStyle();
> +		   if (parentStyle)
> +		      
parentStyle->setChildrenAffectedByForwardPositionalRules();
> +	       }

This code, repeated twice, should probably go into a helper function.

> Source/WebCore/css/SelectorChecker.cpp:634
> +	       e = host;
> +	       while (true) {
> +		   Node* n = e->previousSibling();
> +		   while (n && !n->isElementNode())
> +		       n = n->previousSibling();
> +		   if (!n)
> +		       return SelectorFailsCompletely;
> +		   e = static_cast<Element*>(n);
> +		   SelectorMatch match = checkSelector(sel, e, dynamicPseudo,
false, visitedMatchType);
> +		   if (match != SelectorFailsLocally)
> +		       return match;
> +	       };
> +	       return SelectorFailsCompletely;

There is a stray semicolon here, but also this can be written as a conventional
loop:

    element = host;
    while ((element = element->previousElementSibling())) {
	SelectorMatch match = checkSelector(selector, element, dynamicPseudo,
false, visitedMatchType);
	if (match != SelectorFailsLocally)
	    return match;
    }

Or it could be written as a for loop:

    for (element = host->previousElementSibling(); element; element =
element->previousElementSibling()

> Source/WebCore/css/SelectorChecker.cpp:643
> +	       // A selector is invalid if something follows a pseudo-element
> +	       // We make an exception for scrollbar pseudo elements and allow
a set of pseudo classes (but nothing else)
> +	       // to follow the pseudo elements.
> +	       if ((elementStyle || m_isCollectingRulesOnly)
> +		   && dynamicPseudo != NOPSEUDO
> +		   && dynamicPseudo != SELECTION
> +		   && !((RenderScrollbar::scrollbarForStyleResolve() ||
dynamicPseudo == SCROLLBAR_CORNER || dynamicPseudo == RESIZER) && sel->m_match
== CSSSelector::PseudoClass))

This rule is hard to read in a single expression like this. I suggest we put it
in a named helper function.