Bug 78589 - Move the context invalidation code out from StylePropertySet
Summary: Move the context invalidation code out from StylePropertySet
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 77745 78744
  Show dependency treegraph
 
Reported: 2012-02-14 01:28 PST by Antti Koivisto
Modified: 2012-04-11 20:55 PDT (History)
12 users (show)

See Also:


Attachments
give bots some excercise (73.58 KB, patch)
2012-02-14 14:23 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
another (73.58 KB, patch)
2012-02-14 14:26 PST, Antti Koivisto
pnormand: commit-queue-
Details | Formatted Diff | Diff
another (73.25 KB, patch)
2012-02-14 15:25 PST, Antti Koivisto
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
patch (80.72 KB, patch)
2012-02-15 13:54 PST, Antti Koivisto
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2012-02-14 01:28:17 PST
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.
Comment 1 Antti Koivisto 2012-02-14 14:23:04 PST
Created attachment 127044 [details]
give bots some excercise
Comment 2 Antti Koivisto 2012-02-14 14:26:23 PST
Created attachment 127045 [details]
another
Comment 3 Philippe Normand 2012-02-14 14:33:26 PST
Comment on attachment 127045 [details]
another

Attachment 127045 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11514444
Comment 4 Early Warning System Bot 2012-02-14 15:17:53 PST
Comment on attachment 127045 [details]
another

Attachment 127045 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11518444
Comment 5 WebKit Review Bot 2012-02-14 15:20:32 PST
Comment on attachment 127045 [details]
another

Attachment 127045 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11522421
Comment 6 Antti Koivisto 2012-02-14 15:25:40 PST
Created attachment 127057 [details]
another
Comment 7 Early Warning System Bot 2012-02-14 15:55:44 PST
Comment on attachment 127057 [details]
another

Attachment 127057 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11515437
Comment 8 Gyuyoung Kim 2012-02-14 16:19:24 PST
Comment on attachment 127057 [details]
another

Attachment 127057 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11522447
Comment 9 WebKit Review Bot 2012-02-14 17:38:15 PST
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
Comment 10 WebKit Review Bot 2012-02-14 18:35:42 PST
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
Comment 11 Antti Koivisto 2012-02-15 13:54:33 PST
Created attachment 127228 [details]
patch
Comment 12 Ryosuke Niwa 2012-02-15 18:39:41 PST
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 :(
Comment 13 Ryosuke Niwa 2012-02-15 18:40:37 PST
Apparently this patch reveals editing bugs but it doesn't regress anything so LGTM for editing.
Comment 14 Ryosuke Niwa 2012-02-15 19:42:00 PST
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?
Comment 15 Antti Koivisto 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).
Comment 16 Antti Koivisto 2012-02-15 20:06:28 PST
> > 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.
Comment 17 Ryosuke Niwa 2012-02-15 22:11:29 PST
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.
Comment 18 Ryosuke Niwa 2012-02-15 22:28:23 PST
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.
Comment 19 Antti Koivisto 2012-02-15 22:44:08 PST
> 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.
Comment 20 WebKit Review Bot 2012-02-15 23:30:58 PST
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.
Comment 21 Antti Koivisto 2012-02-15 23:39:40 PST
http://trac.webkit.org/changeset/107899