[Webkit-unassigned] [Bug 76064] [CSSRegions]Add back region style code removed in r104036

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=76064





--- Comment #8 from Antti Koivisto <koivisto at iki.fi>  2012-01-14 13:29:51 PST ---
(From update of attachment 122400)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.