Bug 234959 – Make separate invalidation rulesets for negated selectors (inside :not())

• Component: CSS
• Status: RESOLVED FIXED

[Antti Koivisto]

With this information we can reduce invalidation traversal by half.

[Antti Koivisto]

Created attachment 448594 [details]
Patch

[Antti Koivisto]

Created attachment 448596 [details]
Patch

[Antti Koivisto]

Created attachment 448601 [details]
Patch

[Darin Adler]

Comment on attachment 448601 [details]
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?

[Simon Fraser (smfr)]

Comment on attachment 448601 [details]
Patch

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

> Source/WebCore/style/StyleScopeRuleSets.h:48
> +    IsNegation isNegation;
> +    RefPtr<RuleSet> ruleSet;

There will be some padding here.

[Antti Koivisto]

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

Yeah, in the EWS compiler version. We talked about this earlier.

[Antti Koivisto]

Created attachment 448608 [details]
Patch for landing

[EWS]

Committed r287772 (245832@main): <https://commits.webkit.org/245832@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448608 [details].

[Radar WebKit Bug Importer]

<rdar://problem/87264291>