Bug 136975 – Add the baseline implementation of :nth-child(An+B of selector-list)

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

Add the baseline implementation of :nth-child(An+B of selector-list)

[Benjamin Poulain]

Created attachment 238407 [details]
Patch

[Darin Adler]

Comment on attachment 238407 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238407&action=review

> Source/WebCore/css/SelectorChecker.cpp:676
> +                        for (const CSSSelector* subSelector = selectorList->first(); subSelector; subSelector = CSSSelectorList::next(subSelector)) {

The word “subselector” is a single word so doesn’t need a capital letter in the middle.

> Source/WebCore/css/SelectorChecker.cpp:677
> +                            CheckingContextWithStatus subContext(context);

Same thing for “subcontext”. Even a coined word like this isn’t two words.

> Source/WebCore/css/SelectorChecker.cpp:684
> +                            PseudoId ignoreDynamicPseudo = NOPSEUDO;
> +
> +                            if (matchRecursively(subContext, ignoreDynamicPseudo) == SelectorMatches) {

I think you should remove the blank line here. The paragraphing is a little confusing with that blank line.

> Source/WebCore/css/SelectorChecker.cpp:704
> +                        RenderStyle* childStyle = context.elementStyle ? context.elementStyle : element->renderStyle();
> +                        element->setChildIndex(count);
> +                        if (childStyle)
> +                            childStyle->setUnique();

Existing code, but: If we moved the setChildIndex call up one line, we could define childStyle inside the if statement; just like you did in the new code.

Which in turn makes it clear that most of this if statement can be moved out of the if/else statement and doesn’t need to be repeated twice. The setChildIndex part is the only part that needs to stay here.

[Benjamin Poulain]

Committed r173853: <http://trac.webkit.org/changeset/173853>