WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
76453
[CSSRegions]Fix region style code in CSSStyleSelector
https://bugs.webkit.org/show_bug.cgi?id=76453
Summary
[CSSRegions]Fix region style code in CSSStyleSelector
Mihnea Ovidenie
Reported
2012-01-17 07:09:08 PST
Follow up after comments on
https://bugs.webkit.org/show_bug.cgi?id=76064
Attachments
Patch
(27.08 KB, patch)
2012-01-17 07:24 PST
,
Mihnea Ovidenie
koivisto
: review-
koivisto
: commit-queue-
Details
Formatted Diff
Diff
Patch 2
(30.99 KB, patch)
2012-01-18 06:22 PST
,
Mihnea Ovidenie
no flags
Details
Formatted Diff
Diff
Patch for landing
(31.21 KB, patch)
2012-01-18 07:33 PST
,
Mihnea Ovidenie
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Mihnea Ovidenie
Comment 1
2012-01-17 07:24:27 PST
Created
attachment 122765
[details]
Patch
Antti Koivisto
Comment 2
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.
Mihnea Ovidenie
Comment 3
2012-01-18 06:22:57 PST
Created
attachment 122916
[details]
Patch 2
Mihnea Ovidenie
Comment 4
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
Antti Koivisto
Comment 5
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).
Mihnea Ovidenie
Comment 6
2012-01-18 07:33:55 PST
Created
attachment 122929
[details]
Patch for landing
WebKit Review Bot
Comment 7
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
>
WebKit Review Bot
Comment 8
2012-01-18 08:57:39 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug