[webkit-reviews] review granted: [Bug 199237] Implement @supports selector(). : [Attachment 407378] Fix the debug layout test crash by adding CSSSelector match check in containsUnknownWebKitPseudoElements

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Joonghun Park
<jh718.park at samsung.com>'s request for review:
Bug 199237: Implement @supports selector().
https://bugs.webkit.org/show_bug.cgi?id=199237

Attachment 407378: Fix the debug layout test crash by adding CSSSelector match
check in containsUnknownWebKitPseudoElements

https://bugs.webkit.org/attachment.cgi?id=407378&action=review




--- Comment #16 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 407378
  --> https://bugs.webkit.org/attachment.cgi?id=407378
Fix the debug layout test crash by adding CSSSelector match check in
containsUnknownWebKitPseudoElements

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

> Source/WebCore/ChangeLog:26
> +2020-08-26  Joonghun Park  <jh718.park at samsung.com>
> +
> +	   Implement @supports selector().
> +	   https://bugs.webkit.org/show_bug.cgi?id=199237
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Tests: css3/conditional/w3c/at-supports-040.html
> +		  css3/conditional/w3c/at-supports-041.html
> +		  css3/conditional/w3c/at-supports-042.html
> +
> +	   * css/CSSValueKeywords.in:
> +	   * css/parser/CSSParserImpl.h:
> +	   (WebCore::CSSParserImpl::context const):
> +	   * css/parser/CSSSelectorParser.cpp:
> +	   (WebCore::CSSSelectorParser::supportsComplexSelector):
> +	   (WebCore::CSSSelectorParser::containsUnknownWebKitPseudoElements):
> +	   * css/parser/CSSSupportsParser.cpp:
> +	   (WebCore::CSSSupportsParser::supportsCondition):
> +	   (WebCore::CSSSupportsParser::consumeCondition):
> +	   (WebCore::CSSSupportsParser::consumeNegation):
> +	  
(WebCore::CSSSupportsParser::consumeSupportsFeatureOrGeneralEnclosed):
> +	   (WebCore::CSSSupportsParser::consumeSupportsSelectorFn):
> +	   (WebCore::CSSSupportsParser::consumeConditionInParenthesis):
> +	  
(WebCore::CSSSupportsParser::consumeDeclarationConditionOrGeneralEnclosed):
Deleted.
> +	   * css/parser/CSSSupportsParser.h:

You should type some words about this patch and link to the spec.

> Source/WebCore/css/parser/CSSSelectorParser.cpp:155
> +    return !containsUnknownWebKitPseudoElements(*complexSelector) ? true :
false;

Just

return !containsUnknownWebKitPseudoElements(*complexSelector);

or maybe

if (containsUnknownWebKitPseudoElements(*complexSelector))
    return false;

return true;

to emphasize that false is a rare special case.

> Source/WebCore/css/parser/CSSSelectorParser.cpp:983
> +bool CSSSelectorParser::containsUnknownWebKitPseudoElements(const
CSSSelector& complexSelector)
> +{
> +    for (auto current = &complexSelector; current; current =
current->tagHistory()) {
> +	   if (current->match() == CSSSelector::PseudoElement &&
current->pseudoElementType() == CSSSelector::PseudoElementWebKitCustom)
> +	       return true;
> +    }
> +
> +    return false;
> +}

Why are PseudoElementWebKitCustom excluded? (seems ok but is there some
specific reason?)

> Source/WebCore/css/parser/CSSSupportsParser.cpp:132
> +CSSSupportsParser::SupportsResult
CSSSupportsParser::consumeSupportsSelectorFn(CSSParserTokenRange& range)

WebKit coding style generally uses full words so this should be
consumeSupportsSelectorFunction. Unless there is some spec or other good reason
for this naming?