[webkit-reviews] review granted: [Bug 137348] CSS Selectors Level 4: Add parsing for :matches : [Attachment 239139] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 137348: CSS Selectors Level 4: Add parsing for :matches
https://bugs.webkit.org/show_bug.cgi?id=137348

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=239139&action=review


Exciting!

> Source/WebCore/ChangeLog:11
> +	   Curretnly :not doesn't accept any functional pseudo classes,
:not(:matches(...)) is rejected.

Typo: Curretnly

> Source/WebCore/ChangeLog:12
> +	   And currently, :matches(:visited, :link) is allowed in the parsing
phase.

That's okay, our specialized behavior can be at runtime.

> Source/WebCore/css/CSSSelector.cpp:450
> +		   const CSSSelector* firstSubSelector =
cs->selectorList()->first();
> +		   for (const CSSSelector* subSelector = firstSubSelector;
subSelector; subSelector = CSSSelectorList::next(subSelector)) {
> +		       if (subSelector != firstSubSelector)
> +			   str.appendLiteral(", ");
> +		       str.append(subSelector->selectorText());
> +		   }

It would be a good idea to move this into a static function. The code of
:nth-child(of) does more or less the same thing, both could use the same
function.

> LayoutTests/ChangeLog:7
> +

Wow, that's some serious testing going on here! :)

> LayoutTests/ChangeLog:32
> +

Maybe I missed it, but I did not see any test for canonicalization of the
selector when accessed through CSS OM.

For example, testing that :matches(a,b,c) returns :matches(a, b, c).

> LayoutTests/fast/css/parsing-css-matches-4.html:29
> +    // "*",

I guess you commented some for efficiency? Better remove them entirely, someone
could be confused by the commented ones.

> LayoutTests/fast/css/parsing-css-matches-5.html:22
> +var invalidSelectors = [

Let's add unbalanced parenthesis to this set:
-:not(
-:matches(
-:nth-child(2n+1 of .foo