[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 #15 from Antti Koivisto <koivisto at iki.fi>  2012-02-15 19:57:49 PST ---
(In reply to comment #14)
> (From update of attachment 127228 [details])
> 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.

In practice there is no real difference. For inline style the context stylesheet is the document->elementSheet() which has the document pointer directly.

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

I guess you mean ensureCSSStyleDeclaration()? Yeah, that is for non-rule, non-element use of CSSOM wrapper. Those cases are basically all architectural mistakes and should be cleaned up in favor of accessing the StylePropertySet directly.

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

There are no new look-ups from this patch (function is just triplicated for rule/element/empty cases). The strong belief is that eliminating the almost-always null pointers is a significant overall win. Note that this pattern is very common in WebCore.

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

As mentioned in the ChangeLog this is an intermediate step. The context stylesheet pointer is going to be removed in favor of passing the context in with the mutating function calls.

> > Source/WebCore/dom/StyledElement.cpp:129
> > +        InspectorInstrumentation::didInvalidateStyleAttr(document(), this);
> 
> Why are we calling it here now?

The inspector used to be notified by the StylePropertySet. Since the notification code is now in the CSSOM wrapper we need to do it manually here when mutating directly.

> > 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?

I don't think we should be (these are internal APIs, not visible to the outside world).

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