[Webkit-unassigned] [Bug 98021] Wrong display with "tr:nth-child(even) td" and missing <tbody>

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=98021


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #167684|review?                     |review-
               Flag|                            |




--- Comment #4 from Darin Adler <darin at apple.com>  2012-10-09 09:51:14 PST ---
(From update of attachment 167684)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.