Bug 141373 – Add parsing support for CSS Selector L4's case-insensitive attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

Add parsing support for CSS Selector L4's case-insensitive attribute

[Benjamin Poulain]

Created attachment 246247 [details]
Patch

[Darin Adler]

Comment on attachment 246247 [details]
Patch

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

> Source/WebCore/css/CSSGrammar.y.in:1297
> +        if (UNLIKELY($1.length() != 1))
> +            YYERROR;
> +
> +        UChar character = $1[0];
> +        if (LIKELY(character == 'i' || character == 'I'))
> +            $$ = true;
> +        else
> +            YYERROR;

We have a more efficient function for this in ASCIICType.h, named isASCIIAlphaCaselessEqual.

I would write:

    if (UNLIKELY($1.length() != 1 || !isASCIIAlphaCaselessEqual($1[0], 'i')))
        YYERROR;
    $$ = true;

Is YYERROR exactly what we want here? Do we have enough test coverage to see if our error recovery prevents us from discarding rules other than the one with the badly formed selector?

> Source/WebCore/css/CSSSelector.cpp:-664
> -                    str.append(']');

Was this a bug? Do we have test coverage?

> Source/WebCore/css/CSSSelector.cpp:687
> +                str.append(" i]");

Should be appendLiteral.

[Benjamin Poulain]

Thanks for the review!

(In reply to comment #2)
> We have a more efficient function for this in ASCIICType.h, named
> isASCIIAlphaCaselessEqual.
> 
> I would write:
> 
>     if (UNLIKELY($1.length() != 1 || !isASCIIAlphaCaselessEqual($1[0], 'i')))
>         YYERROR;
>     $$ = true;

Yep, that's a good idea.
 
> Is YYERROR exactly what we want here? Do we have enough test coverage to see
> if our error recovery prevents us from discarding rules other than the one
> with the badly formed selector?

Invalid selector is quite destructive, the entire rule is discarded.
There are two levels of recovery that ensure we nuke everything. I can add a little test for that.

> > Source/WebCore/css/CSSSelector.cpp:-664
> > -                    str.append(']');
> 
> Was this a bug? Do we have test coverage?

This did not change the behavior, ']' is added below in the general path.
I'll put this back, I did not intend to be confusing.

[Benjamin Poulain]

Committed r179819: <http://trac.webkit.org/changeset/179819>