[webkit-reviews] review denied: [Bug 136063] REGRESSION: CSS not() selector does not work when it appears after or within @supports : [Attachment 236856] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Yuki Sekiguchi
<yuki.sekiguchi at access-company.com>'s request for review:
Bug 136063: REGRESSION: CSS not() selector does not work when it appears after
or within @supports
https://bugs.webkit.org/show_bug.cgi?id=136063

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=236856&action=review


> Source/WebCore/css/CSSParser.cpp:11347
> +	   // Since -webkit-supports-condition uses '{', which is
CharacterStartRuleGroup, only SupportsMode should be cleared.
> +	   if (m_parsingMode == SupportsMode)
> +	       m_parsingMode = NormalMode;

Windows is failing to compile because these three lines of code need to be
inside #if ENABLE(CSS3_CONDITIONAL_RULES)

> Source/WebCore/css/CSSParser.h:551
> +    bool isSupportsMode() const { return m_parsingMode == SupportsMode ||
m_parsingMode == SupportsConditionMode; };

Windows is failing to compile because this function needs to be inside #if
ENABLE(CSS3_CONDITIONAL_RULES).

The name of this function is not right. We would not say that a “CSS parser is
supports mode”, so we don’t want to write the code like that. Perhaps the name
isParsingSupports()?

Extra unneeded semicolon on the end of this line.

> LayoutTests/css3/supports-not-selector-expected.html:6
> +<div class="inside">This should be green if not() pseudo class selector
works inside @supports rule.</div>
> +<div class="outside">This should be green if not() pseudo class selector
works after/outside @supports rule.</div>

I suggest removing the class attributes.