Bug 78589 – Move the context invalidation code out from StylePropertySet

• Component: CSS
• Status: RESOLVED FIXED

[Antti Koivisto]

StylePropertySet should be independent of its context so that they can in the future be shared between documents. The context invalidation code should move to the CSSOM wrapper.

[Antti Koivisto]

Created attachment 127044 [details]
give bots some excercise

[Antti Koivisto]

Created attachment 127045 [details]
another

[Philippe Normand]

Comment on attachment 127045 [details]
another

Attachment 127045 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11514444

[Early Warning System Bot]

Comment on attachment 127045 [details]
another

Attachment 127045 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11518444

[WebKit Review Bot]

Comment on attachment 127045 [details]
another

Attachment 127045 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11522421

[Antti Koivisto]

Created attachment 127057 [details]
another

[Early Warning System Bot]

Comment on attachment 127057 [details]
another

Attachment 127057 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11515437

[Gyuyoung Kim]

Comment on attachment 127057 [details]
another

Attachment 127057 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11522447

[WebKit Review Bot]

Comment on attachment 127057 [details]
another

Attachment 127057 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11523438

New failing tests:
editing/undo/remove-css-property-and-remove-style.html
inspector/styles/styles-update-from-js.html
editing/style/push-down-inline-styles.html

[WebKit Review Bot]

Comment on attachment 127057 [details]
another

Attachment 127057 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11518529

New failing tests:
editing/undo/remove-css-property-and-remove-style.html
inspector/styles/styles-update-from-js.html
editing/style/push-down-inline-styles.html

[Antti Koivisto]

Created attachment 127228 [details]
patch

[Ryosuke Niwa]

Comment on attachment 127228 [details]
patch

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

> Source/WebCore/editing/ApplyStyleCommand.cpp:390
> -            inlineStyleDecl->removeProperty(CSSPropertyFontSize);
> +            element->removeInlineStyleProperty(CSSPropertyFontSize);

Wait, this can't be right. This won't be undoable. But I guess it was always like that.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:537
> -                element->ensureInlineStyleDecl()->setProperty(CSSPropertyDisplay, CSSValueInline);
> +                element->setInlineStyleProperty(CSSPropertyDisplay, CSSValueInline);
>              if (element->renderer() && element->renderer()->style()->isFloating())
> -                element->ensureInlineStyleDecl()->setProperty(CSSPropertyFloat, CSSValueNone);
> +                element->setInlineStyleProperty(CSSPropertyFloat, CSSValueNone);

These are also undoable :(

[Ryosuke Niwa]

Apparently this patch reveals editing bugs but it doesn't regress anything so LGTM for editing.

[Ryosuke Niwa]

Comment on attachment 127228 [details]
patch

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?

[Antti Koivisto]

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

[Antti Koivisto]

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

Also, if we at some point figure out we need to cache the inline style wrapper pointer for direct access, we can easily do it in the StyledElement level without bloating stylesheet data structures.

[Ryosuke Niwa]

Comment on attachment 127228 [details]
patch

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

> Source/WebCore/css/StylePropertySet.cpp:1200
> +    bool changes = m_propertySet->removeProperty(propertyID, &result);
> +    if (changes) {

It seems like we don't need this local variable. Are you adding it as a documentation?

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

I think you meant to have StyleAttributeMutationScope here since that's what has been removed from StylePropertySet::parseDeclaration.

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

Ditto about inspector -> mutation scope. In fact, you can't define a mutation scope here. You need to define it in each one of setInlineStyleProperty and call enqueueMutationRecord when changes is true.

[Ryosuke Niwa]

Comment on attachment 127228 [details]
patch

On my second thought, not instantiating mutation scope is fine because all calls to setInlineStyleProperty are invisible to scripts except ones in Editor.cpp, RemoveCSSPropertyCommand.cpp, & ReplaceSelectionCommand.cpp. Please revert those before landing the patch.

[Antti Koivisto]

> It seems like we don't need this local variable. Are you adding it as a documentation?

Right. The purpose of the bool return value is rather unclear otherwise.

> On my second thought, not instantiating mutation scope is fine because all calls to setInlineStyleProperty are invisible to scripts except ones in Editor.cpp, RemoveCSSPropertyCommand.cpp, & ReplaceSelectionCommand.cpp. Please revert those before landing the patch.

Replaced these with calls to the CSSOM wrapper as discussed.

[WebKit Review Bot]

Attachment 127228 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: dataTransfer.types (HTML5 drag & drop) should return DOMStringList
Using index info to reconstruct a base tree...
<stdin>:84: trailing whitespace.
        
<stdin>:333: trailing whitespace.
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
<stdin>:334: trailing whitespace.
 width="300" height="300" onload="runRepaintTest()">
<stdin>:335: trailing whitespace.
<script xlink:href="../../fast/repaint/resources/repaint.js" />
<stdin>:336: trailing whitespace.
<script><![CDATA[
warning: squelched 16 whitespace errors
warning: 21 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 dataTransfer.types (HTML5 drag & drop) should return DOMStringList

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

http://trac.webkit.org/changeset/107899

[James Coleman]

All of our thoughts, words, and deeds flow from a chain
https://snake-game.io