[Webkit-unassigned] [Bug 203252] Move MatchResult and related types out of StyleResolver

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=203252

Said Abou-Hallawa <sabouhallawa at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sabouhallawa at apple.com

--- Comment #7 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 381675
  --> https://bugs.webkit.org/attachment.cgi?id=381675
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20191023/916a4e22/attachment.html>