Bug 76064 – [CSSRegions]Add back region style code removed in r104036

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Mihnea Ovidenie]

Since the code reverted did not change the performance results, add the code back.

[Mihnea Ovidenie]

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.

[Tony Chang]

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.

[Mihnea Ovidenie]

Created attachment 122400 [details]
Patch 2

[Tony Chang]

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

[Dave Hyatt]

Comment on attachment 122400 [details]
Patch 2

r=me

[WebKit Review Bot]

Comment on attachment 122400 [details]
Patch 2

Clearing flags on attachment: 122400

Committed r104965: <http://trac.webkit.org/changeset/104965>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

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.

[Antti Koivisto]

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.

[Mihnea Ovidenie]

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