Bug 76064 - [CSSRegions]Add back region style code removed in r104036
Summary: [CSSRegions]Add back region style code removed in r104036
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-11 08:50 PST by Mihnea Ovidenie
Modified: 2012-01-15 23:02 PST (History)
5 users (show)

See Also:


Attachments
Patch (31.80 KB, patch)
2012-01-11 08:57 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 2 (31.64 KB, patch)
2012-01-13 02:19 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihnea Ovidenie 2012-01-11 08:50:09 PST
Since the code reverted did not change the performance results, add the code back.
Comment 1 Mihnea Ovidenie 2012-01-11 08:57:10 PST
Created attachment 122038 [details]
Patch

Almost the same code as the one that was removed. The main difference comes from CSSStyleSelector.h, in which the definition of MatchedStyleDeclaration was changed by adding a bit field.
Comment 2 Tony Chang 2012-01-11 10:25:01 PST
Comment on attachment 122038 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122038&action=review

Just some nits, Hyatt should do the actual review.  Can you upload a patch that applies to tip-of-tree so the ews bots will run?

> Source/WebCore/css/CSSStyleSelector.cpp:220
> +    RuleSet(bool regionStyleRule = false);

explicit.  Also, I think passing in an enum would make the calling code easier to read and could be used in addMatchedDeclaration and RuleData.
Comment 3 Mihnea Ovidenie 2012-01-13 02:19:56 PST
Created attachment 122400 [details]
Patch 2
Comment 4 Tony Chang 2012-01-13 10:43:07 PST
Comment on attachment 122400 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=122400&action=review

> Source/WebCore/css/CSSStyleSelector.cpp:221
> +    RuleSet(ERegionStyleEnabled useInRegionStyle = DoNotUseInRegionStyle);

explicit
Comment 5 Dave Hyatt 2012-01-13 11:08:13 PST
Comment on attachment 122400 [details]
Patch 2

r=me
Comment 6 WebKit Review Bot 2012-01-13 11:43:10 PST
Comment on attachment 122400 [details]
Patch 2

Clearing flags on attachment: 122400

Committed r104965: <http://trac.webkit.org/changeset/104965>
Comment 7 WebKit Review Bot 2012-01-13 11:43:15 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Antti Koivisto 2012-01-14 13:29:51 PST
Comment on attachment 122400 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=122400&action=review

The CSSStyleSelector code in this patch is not good. The patch tramples all over the performance critical functions and data structures for no good reason (and with no performance data to prove there are no regressions). 

It is unfortunate that this landed as it is clearly r- and needs to be reverted (at least the CSSStyleSelector portion of it). I'd like to see a patch with CSSStyleSelector portion separated.

> Source/WebCore/css/CSSStyleSelector.cpp:173
> -    RuleData(CSSStyleRule*, CSSSelector*, unsigned position);
> +    RuleData(CSSStyleRule*, CSSSelector*, unsigned position, ERegionStyleEnabled useInRegionStyle = DoNotUseInRegionStyle);

RuleSet and RuleData structure exists for optimizing the style lookups. It is not ok to use them for implementing CSS features.

If needed, you could look "useInRegionStyle" up through CSSStyleRule*. If you have a RuleSet containing region styles only (like this patch has) you should never need this information anyway!

> Source/WebCore/css/CSSStyleSelector.cpp:962
> +    m_regionRules = adoptPtr(new RuleSet(UseInRegionStyle));
> +
> +    // From all the region style rules, select those that apply to the specified region.
> +    for (Vector<RefPtr<WebKitCSSRegionRule> >::iterator it = m_regionStyleRules.begin(); it != m_regionStyleRules.end(); ++it) {
> +        const CSSSelectorList& regionSelectorList = (*it)->selectorList();
> +        for (CSSSelector* s = regionSelectorList.first(); s; s = regionSelectorList.next(s)) {
> +            if (!m_checker.checkSelector(s, static_cast<Element*>(region->node())))
> +                continue;
> +            CSSRuleList* regionStylingRules = (*it)->cssRules();
> +            for (unsigned index = 0; index < regionStylingRules->length(); ++index) {
> +                CSSRule* regionStylingRule = regionStylingRules->item(index);
> +                if (regionStylingRule->isStyleRule())
> +                    m_regionRules->addStyleRule(static_cast<CSSStyleRule*>(regionStylingRule));
> +            }
> +        }
> +    }

This constructs a RuleSet on every region style lookup. RuleSets are meant to be cached structures built statically from style sheets. Furthermore, it actually builds up RuleSet, a style lookup optimization structure, by making selector queries. This makes no sense and is going to be horribly slow.

> Source/WebCore/css/CSSStyleSelector.cpp:1315
> -PassRefPtr<RenderStyle> CSSStyleSelector::styleForElement(Element* element, RenderStyle* defaultParent, bool allowSharing, bool resolveForRootDefault)
> +PassRefPtr<RenderStyle> CSSStyleSelector::styleForElement(Element* element, RenderStyle* defaultParent, bool allowSharing, bool resolveForRootDefault, RenderRegion* regionForStyling)

CSSStyleSelector::styleForElement is the core function for style resolve. It is not ok to add new parameters here for this kind of narrow special use case. If you need specialized lookups you should add new functions (similar to CSSStyleSelector::styleForPage etc).

> Source/WebCore/css/CSSStyleSelector.cpp:1488
> -PassRefPtr<RenderStyle> CSSStyleSelector::pseudoStyleForElement(PseudoId pseudo, Element* e, RenderStyle* parentStyle)
> +PassRefPtr<RenderStyle> CSSStyleSelector::pseudoStyleForElement(PseudoId pseudo, Element* e, RenderStyle* parentStyle, RenderRegion* regionForStyling)

As far as I see, you are not even calling this new version of pseudoStyleForElement.

> Source/WebCore/css/CSSStyleSelector.cpp:2319
>      }
> -    for (int i = startIndex; i <= endIndex; ++i)
> +    for (int i = startIndex; i <= endIndex; ++i) {
> +        m_applyPropertyToRegionStyle = m_matchedDecls[i].useInRegionStyle;
>          applyDeclaration<applyFirst>(m_matchedDecls[i].styleDeclaration.get(), isImportant, inheritedOnly);
> +        m_applyPropertyToRegionStyle = false;
> +    }

This strategy is used specifically for resolving both visited and regular link style efficiently in parallel (and it is ugly there too). It is not meant that you emulate this code for some feature work.

> Source/WebCore/css/CSSStyleSelector.cpp:2701
> +    // Filter region style property
> +    if (applyPropertyToRegionStyle() && !isValidRegionStyleProperty(id))
> +        return;

The filtering is used for link styles for security reasons. Why does this feature need filtering here? Generally the rendering code should just ignore the properties it is not using.

> Source/WebCore/css/CSSStyleSelector.h:151
> +    void setRegionForStyling(RenderRegion* region) { m_regionForStyling = region; }
> +    RenderRegion* regionForStyling() const { return m_regionForStyling; }

As far as I can see, m_regionForStyling field is not used for anything in this patch (except one if (m_regionForStyling)) so I don't see why it exists.
Comment 9 Antti Koivisto 2012-01-15 10:41:17 PST
Hmm, reading the spec some of this starts making more sense. Maybe this is better fixed in place.

You will want to create the RuleSets for the regions on style selector initialization time. Creating them on every style look up is super slow. I believe you are also getting the specificity (and so the applying order) wrong by matching them separately from the author style. Per spec the @region should behave like @media and not affect the ordering.

Something like this might work for a better and more correct implementation:

1) Cache the region RuleSets in the containing RuleSet. This is done by adding (roughly)

Vector<<pair<CSSSelector, RuleSet*> > m_regionSelectorsAndRuleSets;

member to RuleSets.

3) These sub-RuleSets are created by RuleSet::addRulesFromSheet (thus only once per style selector). The rule position is set to match the actual stylesheet position.

2) When matching the region style (the region element is set) CSSStyleSelector::matchRules will loop though m_regionSelectorsAndRuleSets, if any, and try to match the selectors to the region element.

3) If the region selector matches then CSSStyleSelector::matchRules is invoked recursively for the sub-RuleSet (this will require some refactoring of matchRules, the actual matching needs to be split out).

The (confusingly similar) m_regionStyleRules and m_regionRules members of CSSStyleSelector both go away.
Comment 10 Mihnea Ovidenie 2012-01-15 23:02:48 PST
(In reply to comment #9)
> Hmm, reading the spec some of this starts making more sense. Maybe this is better fixed in place.
> 
> You will want to create the RuleSets for the regions on style selector initialization time. Creating them on every style look up is super slow. I believe you are also getting the specificity (and so the applying order) wrong by matching them separately from the author style. Per spec the @region should behave like @media and not affect the ordering.
> 
> Something like this might work for a better and more correct implementation:
> 
> 1) Cache the region RuleSets in the containing RuleSet. This is done by adding (roughly)
> 
> Vector<<pair<CSSSelector, RuleSet*> > m_regionSelectorsAndRuleSets;
> 
> member to RuleSets.
> 
> 3) These sub-RuleSets are created by RuleSet::addRulesFromSheet (thus only once per style selector). The rule position is set to match the actual stylesheet position.
> 
> 2) When matching the region style (the region element is set) CSSStyleSelector::matchRules will loop though m_regionSelectorsAndRuleSets, if any, and try to match the selectors to the region element.
> 
> 3) If the region selector matches then CSSStyleSelector::matchRules is invoked recursively for the sub-RuleSet (this will require some refactoring of matchRules, the actual matching needs to be split out).
> 
> The (confusingly similar) m_regionStyleRules and m_regionRules members of CSSStyleSelector both go away.

Thanks for the review/suggestions. I will leave the actual code in place (not roll out the patch) and come up with another patch that will improve the current solution.