[Webkit-unassigned] [Bug 76453] [CSSRegions]Fix region style code in CSSStyleSelector

[bugzilla-daemon at webkit.org]

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


Antti Koivisto <koivisto at iki.fi> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #122765|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #2 from Antti Koivisto <koivisto at iki.fi>  2012-01-17 08:26:47 PST ---
(From update of attachment 122765)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.