Bug 76453 – [CSSRegions]Fix region style code in CSSStyleSelector

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Mihnea Ovidenie]

Follow up after comments on https://bugs.webkit.org/show_bug.cgi?id=76064

[Mihnea Ovidenie]

Created attachment 122765 [details]
Patch

[Antti Koivisto]

Comment on attachment 122765 [details]
Patch

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

Looks good. I would still like to see another patch due to some factoring and naming issues, and other nits.

> Source/WebCore/css/CSSStyleSelector.cpp:257
> +    Vector<pair<CSSSelector*, RuleSet*> > m_regionSelectorsAndRuleSets;

I would prefer 

struct SelectorRuleSetPair {
    CSSSelector* selector;
    OwnPtr<RuleSet> ruleSet;
};

instead of pair<>, the resulting code will read better (which is why I said "roughly" in the comment that where I told to use a pair :).

You should use use OwnPtr<RuleSet> so you don't need explicit deletion.

> Source/WebCore/css/CSSStyleSelector.cpp:636
> -void CSSStyleSelector::matchRules(RuleSet* rules, int& firstRuleIndex, int& lastRuleIndex, bool includeEmptyRules)
> +void CSSStyleSelector::collectRules(RuleSet* rules, int& firstRuleIndex, int& lastRuleIndex, bool includeEmptyRules)

collectRules() is somewhat vague.

How about collectMatchingRules()?  Also rename matchRulesForList() to -> collectMatchingRulesForList().

> Source/WebCore/css/CSSStyleSelector.cpp:682
> +    collectRules(rules, firstRuleIndex, lastRuleIndex, includeEmptyRules);
> +
> +    if (m_regionForStyling) {
> +        for (unsigned index = 0; index < rules->m_regionSelectorsAndRuleSets.size(); ++index) {
> +            CSSSelector* regionSelector = rules->m_regionSelectorsAndRuleSets.at(index).first;
> +            if (checkRegionSelector(regionSelector, static_cast<Element*>(m_regionForStyling->node()))) {
> +                RuleSet* regionRules = rules->m_regionSelectorsAndRuleSets.at(index).second;
> +                collectRules(regionRules, firstRuleIndex, lastRuleIndex, includeEmptyRules);
> +            }
> +        }
> +    }

I think the loop should be factored into a separate function, collectMatchingRulesForRegion() or similar.

You should copy the size to variable before looping.
unsigned size = rules->m_regionSelectorsAndRuleSets.size();

Please use the usual 'i' instead of 'index' as the loop variable name.

> Source/WebCore/css/CSSStyleSelector.cpp:1758
> +    for (unsigned index = 0; index < m_authorStyle->m_regionSelectorsAndRuleSets.size(); ++index) {

The same comment about index variable name and size in a local.

> Source/WebCore/css/CSSStyleSelector.cpp:1764
> +        for (unsigned index = 0; index < m_userStyle->m_regionSelectorsAndRuleSets.size(); ++index) {

The same comment about index variable name and size in a local.

> Source/WebCore/css/CSSStyleSelector.cpp:2041
> +RuleSet::~RuleSet()
> +{
> +    for (unsigned index = 0; index < m_regionSelectorsAndRuleSets.size(); ++index)
> +        delete m_regionSelectorsAndRuleSets.at(index).second;
> +}

With OwnPtr this won't be needed.

> Source/WebCore/css/CSSStyleSelector.cpp:2098
> +    for (unsigned index = 0; index < regionStylingRules->length(); ++index) {

The same comment about index variable name and size in a local.

> Source/WebCore/css/CSSStyleSelector.cpp:2248
> +    bool styleDeclarationInsideRegionRule = false;
> +    if (m_regionForStyling) {
> +        CSSRule* parentRule = styleDeclaration->parentRule();
> +        while (parentRule) {
> +            if (parentRule->isRegionRule()) {
> +                styleDeclarationInsideRegionRule = true;
> +                break;
> +            }
> +            parentRule = parentRule->parentRule();
> +        }
> +    }

You should factor this into an inline function, isInRegionRule(CSSStyleDeclaration*) or something.

[Mihnea Ovidenie]

Created attachment 122916 [details]
Patch 2

[Mihnea Ovidenie]

Filed a bug about adding a new test for region styling that will test how specificity/position of css rules inside region rules is taken into account: https://bugs.webkit.org/show_bug.cgi?id=76537

[Antti Koivisto]

Comment on attachment 122916 [details]
Patch 2

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

r=me, with a few nits

> Source/WebCore/css/CSSStyleSelector.cpp:674
> +void CSSStyleSelector::collectMatchingRulesForRegion(RuleSet* rules, int& firstRuleIndex, int& lastRuleIndex, bool includeEmptyRules)
> +{
> +    if (m_regionForStyling) {

You should use early return here:

if (!m_regionForStyling)
    return;

> Source/WebCore/css/CSSStyleSelector.cpp:2245
> +inline bool CSSStyleSelector::isInsideRegionRule(CSSMutableStyleDeclaration* styleDeclaration)
> +{

This would be nicer as a static non-member inline. The caller should test if it needs to be called (m_regionForStyling is set).

[Mihnea Ovidenie]

Created attachment 122929 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 122929 [details]
Patch for landing

Clearing flags on attachment: 122929

Committed r105280: <http://trac.webkit.org/changeset/105280>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.