Bug 76453

Created attachment 122765 [details] Patch

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.

Created attachment 122916 [details] Patch 2

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 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).

Created attachment 122929 [details] Patch for landing

Comment on attachment 122929 [details] Patch for landing Clearing flags on attachment: 122929 Committed r105280: <http://trac.webkit.org/changeset/105280>

All reviewed patches have been landed. Closing bug.