Bug 74178 - Add new CSS nth-children parsing tests
Summary: Add new CSS nth-children parsing tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 69083
  Show dependency treegraph
 
Reported: 2011-12-09 06:20 PST by Zoltan Herczeg
Modified: 2011-12-11 23:45 PST (History)
3 users (show)

See Also:


Attachments
patch (6.90 KB, patch)
2011-12-09 06:45 PST, Zoltan Herczeg
darin: review+
zherczeg: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Herczeg 2011-12-09 06:20:34 PST
The test covers several valid and invalid nth-child tokens.
Comment 1 Zoltan Herczeg 2011-12-09 06:45:24 PST
Created attachment 118570 [details]
patch
Comment 2 Darin Adler 2011-12-09 09:35:25 PST
Comment on attachment 118570 [details]
patch

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

> Source/WebCore/css/CSSParser.cpp:8092
>      // The tokenizer checks for the construct of an+b.
>      // nth can also accept "n", "odd" or "even" but should not accept any other token.
> -    return equalIgnoringCase(token, "odd") || equalIgnoringCase(token, "even") || equalIgnoringCase(token, "n");
> +    return equalIgnoringCase(token, "odd") || equalIgnoringCase(token, "even")
> +        || equalIgnoringCase(token, "n") || equalIgnoringCase(token, "-n");

The comment now does not match the code. There is no clue here why "-n" is something we need to allow here.
Comment 3 Zoltan Herczeg 2011-12-09 10:04:17 PST
> The comment now does not match the code. There is no clue here why "-n" is something we need to allow here.

Thanks for the review. I'll fix the comment and land manually.
Comment 4 Darin Adler 2011-12-09 10:06:14 PST
When fixing the comment I suggest removing the part that says what the code does, since the code already seems to say that clearly enough, and enhancing the aspect of comment that says *why* the code does what it does.
Comment 5 Zoltan Herczeg 2011-12-11 23:45:53 PST
Changed the comment to:

    // The tokenizer checks for the construct of an+b.
    // However, since the {ident} rule precedes the {nth} rule, some of those
    // tokens are identified as string literal. Furthermore we need to accept
    // "odd" and "even" which does not match to an+b.

And landed: http://trac.webkit.org/changeset/102560