[webkit-reviews] review granted: [Bug 234959] Make separate invalidation rulesets for negated selectors (inside :not()) : [Attachment 448601] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 234959: Make separate invalidation rulesets for negated selectors (inside
:not())
https://bugs.webkit.org/show_bug.cgi?id=234959

Attachment 448601: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 448601
  --> https://bugs.webkit.org/attachment.cgi?id=448601
Patch

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

> Source/WebCore/style/ClassChangeInvalidation.cpp:37
> +enum class ClassChangeType { Add, Remove };

Consider ": bool"?

> Source/WebCore/style/ClassChangeInvalidation.cpp:41
> +    AtomStringImpl* className;
> +    ClassChangeType type;

Consider providing default values rather than leaving these scalars
uninitialized?

> Source/WebCore/style/ClassChangeInvalidation.cpp:118
> +    auto invalidateBeforeChange = [](ClassChangeType classChangeType,
IsNegation isNegation) {

I think calling the local just "type" makes it slightly easier to read.

> Source/WebCore/style/ClassChangeInvalidation.cpp:121
> +	   if (classChangeType == ClassChangeType::Remove)
> +	       return isNegation == IsNegation::No;
> +	   return isNegation == IsNegation::Yes;

I’d wish for a simpler way to write this; I kind of feel like it’s a boolean
maze right now. But I can’t think of anything obviously better.

> Source/WebCore/style/StyleScopeRuleSets.h:49
> +    Vector<const CSSSelector*> invalidationSelectors { };

Do we really need the "{ }" here? A workaround for a compiler bug?