Bug 203252 – Move MatchResult and related types out of StyleResolver

• Component: CSS
• Status: RESOLVED FIXED

[Antti Koivisto]

Reverse the dependency between StyleResolver and ElementRuleCollector. The former should depend on the latter.

[Antti Koivisto]

Created attachment 381552 [details]
wip

[Antti Koivisto]

Created attachment 381579 [details]
wip

[Antti Koivisto]

Created attachment 381581 [details]
wip

[Antti Koivisto]

Created attachment 381667 [details]
wip

[Antti Koivisto]

Created attachment 381668 [details]
wip

[Antti Koivisto]

Created attachment 381675 [details]
patch

[Said Abou-Hallawa]

Comment on attachment 381675 [details]
patch

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

> Source/WebCore/ChangeLog:12
> +        The desired cascade level is indicated with en enum instead of a range.

Typo: en enum.

> Source/WebCore/css/ElementRuleCollector.cpp:130
>      if (!isCacheable)
>          m_result.isCacheable = false;

I think these two lines should be moved before addMatchedProperties() above because if isCacheable is false, addMatchedProperties() should try to fix m_result.isCacheable.

> Source/WebCore/css/ElementRuleCollector.cpp:622
> +    if (matchedProperties.styleScopeOrdinal != Style::ScopeOrdinal::Element)
> +        m_result.isCacheable = false;
> +
> +    if (m_result.isCacheable) {

Maybe the part of fixing the m_result.isCacheable deserves its own function. 

    m_result.isCacheable = fixIsCacheable(matchedProperties);
    switch (declarationOrigin) {
        ...
    }

I think it can be even be a static. It will also save lines by early returns instead of setting m_result.isCacheable = false; break; below.

> Source/WebCore/css/ElementRuleCollector.cpp:632
> +            if (!current.isInherited()) {

Can we flip the condition and continue:

if (current.isInherited())
    continue;

> Source/WebCore/css/StyleResolver.cpp:2255
> +static auto& declarationsForCascadeLevel(const MatchResult& matchResult, CascadeLevel cascadeLevel)

This function can be added to MatchResult. It can be even a bracket operator. So instead of:

    declarationsForCascadeLevel(matchResult, cascadeLevel)

It can be

    matchResult[cascadeLevel]

> Source/WebCore/css/StyleResolver.h:421
> +        Vector<MatchedProperties> userAgentDeclarations;
> +        Vector<MatchedProperties> userDeclarations;
> +        Vector<MatchedProperties> authorDeclarations;

Can't we replace these member by just one MatchResult?  I think all what we do with these members is assigning their values from a MatchResult and comparing them with a MatchResult.

[Antti Koivisto]

> This function can be added to MatchResult. ]

I actually had that but it requires some further refactoring (moving CascadeLevel enum). I decided I need to stop at some point.

> 
> > Source/WebCore/css/StyleResolver.h:421
> > +        Vector<MatchedProperties> userAgentDeclarations;
> > +        Vector<MatchedProperties> userDeclarations;
> > +        Vector<MatchedProperties> authorDeclarations;
> 
> Can't we replace these member by just one MatchResult?  I think all what we
> do with these members is assigning their values from a MatchResult and
> comparing them with a MatchResult.

Almost, the issue is that MatchResult has large inline capacity for these (it is a temporary type used during collection) while cache entries have none (they are long-lived and immutable).

Making that work requires either templating MatchResult or proving that inline capacity is unnecessary (it probably is) which is best left for another patch.

[Antti Koivisto]

Created attachment 381781 [details]
patch

[WebKit Commit Bot]

Comment on attachment 381781 [details]
patch

Clearing flags on attachment: 381781

Committed r251532: <https://trac.webkit.org/changeset/251532>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56571489>