Bug 80370 – Enable matched declaration caching for elements with a style attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Julien Chaffraix]

http://trac.webkit.org/changeset/106741 disabled this as there were some issues with properly invalidating our matched declaration cache entries.

Let's bring the cache back on!

[Julien Chaffraix]

Created attachment 130273 [details]
Early prototype: add an inline version to StylePropertySet, include this information to MatchedProperties

FYI this patch has its own issues that needs to be ironed out (like prevent people from abusing the logic for non-inline style StylePropertySet and causing perf regressions). I think however that the logic handles all cases of invalidation (JavaScript vs DOM). An earlier version shaved ~1% on Moz page cycler.

Comments welcome on the approach taken!

[Antti Koivisto]

Created attachment 130395 [details]
another possible patch

This takes somewhat different approach by making inline style property sets immutable as long as there is no CSSOM wrapper (no one accesses element.style). Immutability means they are inherently cacheable.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/dom/ElementAttributeData.h:96:  The parameter name "element" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 18 files


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

[Early Warning System Bot]

Comment on attachment 130395 [details]
another possible patch

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

[Ryosuke Niwa]

Comment on attachment 130395 [details]
another possible patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=80370
> +        Re-enable matched declaration caching for elements with a style attribute

The order is wrong here...

> Source/WebCore/dom/StyledElement.cpp:108
> +                // Keep inline style property sets without CSSOM wrappers immutable. This way they can be cached.

I'm confused by this comment. This isn't really a place where we make the property set immutable, or is it?

[WebKit Review Bot]

Comment on attachment 130395 [details]
another possible patch

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

New failing tests:
editing/undo/remove-css-property-and-remove-style.html
fast/css/rem-dynamic-scaling.html
editing/execCommand/toggle-compound-styles.html
editing/execCommand/5770834-1.html
editing/style/push-down-font-styles.html
editing/style/push-down-implicit-styles.html
editing/execCommand/remove-format-multiple-elements.html
editing/style/push-down-inline-styles.html
fast/multicol/vertical-rl/break-properties.html
editing/style/push-down-implicit-styles-around-list.html
editing/style/inline-style-container.html
editing/execCommand/empty-span-removal.html

[Julien Chaffraix]

Comment on attachment 130395 [details]
another possible patch

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

A couple more comments but I agree that this approach is a more light-weight alternative to what I had in mind (and catch a few bugs at compile time).

> Source/WebCore/dom/StyledElement.cpp:129
> +    ensureAttributeData()->ensureMutableInlineStyle(this)->setProperty(propertyID, document()->cssValuePool()->createIdentifierValue(identifier), important);

It really looks like ensureAttributeData()->ensureMutableInlineStyle(this) should have its own method. I wonder if the naming couldn't be improved to underline that we may copy our style as 'mutable' is fairly generic IMHO. I couldn't find a better naming unfortunately.

> Source/WebCore/dom/StyledElement.h:43
> +    const StylePropertySet* inlineStyle() const { return attributeData() ? attributeData()->inlineStyle() : 0; }
> +    const StylePropertySet* ensureInlineStyle() { return ensureAttributeData()->ensureInlineStyle(this); }

Those renamings are making Qt sad:

../../../Source/WebKit/qt/Api/qwebelement.cpp: In member function 'QString QWebElement::styleProperty(const QString&, QWebElement::StyleResolveStrategy) const':
../../../Source/WebKit/qt/Api/qwebelement.cpp:847: error: 'class WebCore::StyledElement' has no member named 'ensureInlineStyleDecl'
../../../Source/WebKit/qt/Api/qwebelement.cpp: In member function 'void QWebElement::setStyleProperty(const QString&, const QString&)':
../../../Source/WebKit/qt/Api/qwebelement.cpp:912: error: 'class WebCore::StyledElement' has no member named 'ensureInlineStyleDecl'

> Source/WebCore/editing/ApplyStyleCommand.cpp:512
> +            RefPtr<StylePropertySet> inlineStyle = element-> ensureInlineStyle()->copy();

Extra space.

> Source/WebCore/editing/ApplyStyleCommand.cpp:726
> +            RefPtr<StylePropertySet> inlineStyle = element-> ensureInlineStyle()->copy();

Ditto.

[WebKit Review Bot]

Comment on attachment 130395 [details]
another possible patch

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

New failing tests:
editing/undo/remove-css-property-and-remove-style.html
fast/css/rem-dynamic-scaling.html
editing/execCommand/toggle-compound-styles.html
editing/execCommand/5770834-1.html
editing/style/push-down-font-styles.html
editing/style/push-down-implicit-styles.html
editing/execCommand/remove-format-multiple-elements.html
editing/style/push-down-inline-styles.html
fast/multicol/vertical-rl/break-properties.html
editing/style/push-down-implicit-styles-around-list.html
editing/style/inline-style-container.html
editing/execCommand/empty-span-removal.html

[Antti Koivisto]

Created attachment 130590 [details]
bug fixes, qt build fix

[Early Warning System Bot]

Comment on attachment 130590 [details]
bug fixes, qt build fix

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

[WebKit Review Bot]

Comment on attachment 130590 [details]
bug fixes, qt build fix

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

New failing tests:
media/track/track-cue-rendering.html

[Antti Koivisto]

Created attachment 130609 [details]
another qt build fix attempt

[WebKit Review Bot]

Comment on attachment 130609 [details]
another qt build fix attempt

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

New failing tests:
media/track/track-cue-rendering.html

[WebKit Review Bot]

Comment on attachment 130609 [details]
another qt build fix attempt

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

New failing tests:
media/track/track-cue-rendering.html

[Andreas Kling]

Comment on attachment 130609 [details]
another qt build fix attempt

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

This looks pretty cool. r=me, assuming the EWS failure is a flake.

> Source/WebCore/dom/ElementAttributeData.cpp:70
> +        m_inlineStyleDecl = m_inlineStyleDecl->copy();
> +        m_inlineStyleDecl->setStrictParsing(element->isHTMLElement() && !element->document()->inQuirksMode());

I feel like copy() should return a StylePropertySet with the same strictness setting as the original.

[Antti Koivisto]

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

(and http://trac.webkit.org/changeset/110190 to fix qt build)