Bug 136975

Summary: Add the baseline implementation of :nth-child(An+B of selector-list)
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Severity: Normal CC: koivisto
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch darin: review+

Description Benjamin Poulain 2014-09-19 22:25:00 PDT
Add the baseline implementation of :nth-child(An+B of selector-list)
Comment 1 Benjamin Poulain 2014-09-19 22:28:38 PDT
Created attachment 238407 [details]
Comment 2 Darin Adler 2014-09-21 17:21:53 PDT
Comment on attachment 238407 [details]

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.
Comment 3 Benjamin Poulain 2014-09-22 15:10:05 PDT
Committed r173853: <http://trac.webkit.org/changeset/173853>