[webkit-reviews] review denied: [Bug 76453] [CSSRegions]Fix region style code in CSSStyleSelector : [Attachment 122765] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Mihnea Ovidenie
<mihnea at adobe.com>'s request for review:
Bug 76453: [CSSRegions]Fix region style code in CSSStyleSelector
https://bugs.webkit.org/show_bug.cgi?id=76453

Attachment 122765: Patch
https://bugs.webkit.org/attachment.cgi?id=122765&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
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.