[Webkit-unassigned] [Bug 78589] Move the context invalidation code out from StylePropertySet

[bugzilla-daemon at webkit.org]

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





--- Comment #14 from Ryosuke Niwa <rniwa at webkit.org>  2012-02-15 19:42:00 PST ---
(From update of attachment 127228)
View in context: https://bugs.webkit.org/attachment.cgi?id=127228&action=review

> Source/WebCore/css/StylePropertySet.cpp:778
> +    if (Document* document = m_contextStyleSheet ? m_contextStyleSheet->findDocument() : 0)

I'm somewhat worried about the performance implication on replacing document() by findDocument() here since it's linear in the number of stylesheet hierarchy whereas it used to be constant. Doing that on every property assignment may be quite expensive.

> Source/WebCore/css/StylePropertySet.cpp:1051
> +CSSStyleDeclaration* StylePropertySet::ensureRuleCSSStyleDeclaration(const CSSRule* parentRule) const

So this function is still called in WebKitCSSKeyframeRule, EditingStyle, InspectorCSSAgent, & DragController but I guess their use cases require don't fit into either inline or rule style declarations.

> Source/WebCore/css/StylePropertySet.cpp:1081
> +    ASSERT_UNUSED(rule, static_cast<CSSStyleDeclaration*>(propertySetCSSOMWrapperMap().get(this))->parentRule() == rule);
> +    propertySetCSSOMWrapperMap().get(this)->clearParentRule();

I'm a bit worried about the fact we're introducing all these hash lookups. We might want to consider having a pointer in StylePropertySet instead.

> Source/WebCore/dom/StyledElement.cpp:71
> +    Element::insertedIntoDocument();
> +
> +    if (StylePropertySet* inlineStyle = inlineStyleDecl())
> +        inlineStyle->setContextStyleSheet(document()->elementSheet());
> +    if (StylePropertySet* attributeStyle = attributeData() ? attributeData()->attributeStyle() : 0)
> +        attributeStyle->setContextStyleSheet(document()->elementSheet());

So now we're pushing context stylesheet instead of pulling it. That's probably for setProperty and it makes sense but I'm worried that there are some edge cases where this doesn't work; particularly for some SVG elements...

> Source/WebCore/dom/StyledElement.cpp:129
> +        InspectorInstrumentation::didInvalidateStyleAttr(document(), this);

Why are we calling it here now?

> Source/WebCore/dom/StyledElement.cpp:144
> +    if (changes)
> +        inlineStyleChanged();

Could you add the scary comment about how we should be throwing exception here as well?

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