[webkit-reviews] review denied: [Bug 98021] Wrong display with "tr:nth-child(even) td" and missing <tbody> : [Attachment 167684] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Takashi Sakamoto <tasak at google.com>'s
request for review:
Bug 98021: Wrong display with "tr:nth-child(even) td" and missing <tbody>
https://bugs.webkit.org/show_bug.cgi?id=98021

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

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


> Source/WebCore/css/SelectorChecker.cpp:1391
> +inline int
SelectorChecker::DOMTraversalStrategy<SelectorChecker::SelectorCheckingContext>
::countElementsBefore(const SelectorChecker::SelectorCheckingContext& context, 
Element* element, bool resolvingState)

I think a better name for this “resolving state” boolean would be
shouldSetChildIndex.

Also, there’s an extra stray space here after the comma.

> Source/WebCore/css/SelectorChecker.cpp:1410
> +    if (resolvingState) {
> +	   const Element* previousSibling = element->previousElementSibling();
> +	   RenderStyle* childStyle = context.elementStyle ?
context.elementStyle : element->renderStyle();
> +	   // Don't setChildIndex if childStyle is a shared style.
> +	   if (childStyle && !(previousSibling &&
previousSibling->renderStyle() == childStyle))
> +	       childStyle->setChildIndex(count + 1);
> +    }

A better way to express the “don’t set the child index if the style is shared”
is to make a named local variable. I’d write the code like this:

    if (shouldSetChildIndex) {
	if (RenderStyle* childStyle = context.elementStyle ?
context.elementStyle : element->renderStyle()) {
	    const Element* previousSibling = element->previousElementSibling();

	    bool isSharedStyle = previousSibling &&
previousSibling->renderStyle() == childStyle;
	    if (!isSharedStyle)
		childStyle->setIndex(count + 1);
	}
    }

This way, the code comments itself so you don’t need that comment.

Will doing these additional previousElementSibling linked list walks have a
measurable performance impact?

I think it’s really unfortunate here that the “count + 1” rule is hardcoded
here as well as at the call site.

I don’t understand why this code has to be moved inside the countElementsBefore
function. The block of code here doesn’t seem to take advantage of the state at
all. It seems like we just added the previousElementSibling logic, but this
logic could have been added in checkOneSelector without moving the code.

> Source/WebCore/css/SelectorChecker.h:91
> +	   static int countElementsBefore(const Context&, Element*, bool);

This bool needs an argument name. The rule is that arguments with meanings that
are obvious given the type can be omitted, but this bool does not qualify.
Frankly the qualified name arguments for types in the surrounding functions
also don’t, and need an argument name, but this is an even more extreme
example.