Bug 76453

Summary: [CSSRegions]Fix region style code in CSSStyleSelector
Product: WebKit Reporter: Mihnea Ovidenie <mihnea>
Component: CSSAssignee: Mihnea Ovidenie <mihnea>
Status: RESOLVED FIXED    
Severity: Normal CC: macpherson, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 57312    
Attachments:
Description Flags
Patch
koivisto: review-, koivisto: commit-queue-
Patch 2
none
Patch for landing none

Description Mihnea Ovidenie 2012-01-17 07:09:08 PST
Follow up after comments on https://bugs.webkit.org/show_bug.cgi?id=76064
Comment 1 Mihnea Ovidenie 2012-01-17 07:24:27 PST
Created attachment 122765 [details]
Patch
Comment 2 Antti Koivisto 2012-01-17 08:26:47 PST
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.
Comment 3 Mihnea Ovidenie 2012-01-18 06:22:57 PST
Created attachment 122916 [details]
Patch 2
Comment 4 Mihnea Ovidenie 2012-01-18 06:33:07 PST
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
Comment 5 Antti Koivisto 2012-01-18 06:47:38 PST
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).
Comment 6 Mihnea Ovidenie 2012-01-18 07:33:55 PST
Created attachment 122929 [details]
Patch for landing
Comment 7 WebKit Review Bot 2012-01-18 08:57:34 PST
Comment on attachment 122929 [details]
Patch for landing

Clearing flags on attachment: 122929

Committed r105280: <http://trac.webkit.org/changeset/105280>
Comment 8 WebKit Review Bot 2012-01-18 08:57:39 PST
All reviewed patches have been landed.  Closing bug.