[webkit-reviews] review granted: [Bug 136975] Add the baseline implementation of :nth-child(An+B of selector-list) : [Attachment 238407] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 136975: Add the baseline implementation of :nth-child(An+B of
selector-list)
https://bugs.webkit.org/show_bug.cgi?id=136975

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

------- Additional Comments from Darin Adler <darin at apple.com>
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.