Bug 136975 - Add the baseline implementation of :nth-child(An+B of selector-list)
Summary: Add the baseline implementation of :nth-child(An+B of selector-list)
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
Depends on:
Reported: 2014-09-19 22:25 PDT by Benjamin Poulain
Modified: 2014-09-22 15:10 PDT (History)
1 user (show)

See Also:

Patch (181.10 KB, patch)
2014-09-19 22:28 PDT, Benjamin Poulain
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>