RESOLVED FIXED 141373
Add parsing support for CSS Selector L4's case-insensitive attribute
https://bugs.webkit.org/show_bug.cgi?id=141373
Summary Add parsing support for CSS Selector L4's case-insensitive attribute
Benjamin Poulain
Reported 2015-02-08 13:08:23 PST
Add parsing support for CSS Selector L4's case-insensitive attribute
Attachments
Patch (267.24 KB, patch)
2015-02-08 13:20 PST, Benjamin Poulain
darin: review+
Benjamin Poulain
Comment 1 2015-02-08 13:20:41 PST
Darin Adler
Comment 2 2015-02-08 16:21:43 PST
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
Comment 3 2015-02-08 18:57:17 PST
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
Comment 4 2015-02-08 19:45:00 PST
Note You need to log in before you can comment on or make changes to this bug.