Bug 78381 – Attribute styles should be created lazily.

• Component: DOM
• Status: RESOLVED FIXED

[Andreas Kling]

In order to make the matched declaration cache work for elements with presentation attributes again, we need the attributeStyle() to be immutable. Lazy initialization is one way to achieve this.

[Andreas Kling]

Created attachment 126571 [details]
WIP for EWS and the overly curious

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/css/CSSStyleSelector.cpp', ..." exit_code: 1

Last 3072 characters of output:
ace/operators] [4]
Source/WebCore/html/HTMLHRElement.cpp:101:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/html/HTMLElement.cpp:216:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/html/HTMLElement.cpp:217:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/html/HTMLElement.cpp:218:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/html/HTMLElement.cpp:219:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/html/HTMLParagraphElement.h:38:  The parameter name "style" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLIFrameElement.h:39:  The parameter name "style" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLBodyElement.cpp:89:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/html/HTMLBodyElement.cpp:90:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/html/HTMLBodyElement.cpp:91:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/html/HTMLBodyElement.cpp:92:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/html/HTMLBodyElement.cpp:93:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/html/HTMLBodyElement.cpp:94:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/html/HTMLBodyElement.cpp:95:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/html/HTMLTableElement.cpp:278:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Source/WebCore/html/HTMLIFrameElement.cpp:60:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 19 in 65 files


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

[Antti Koivisto]

Comment on attachment 126571 [details]
WIP for EWS and the overly curious

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

> Source/WebCore/html/HTMLEmbedElement.cpp:85
> +void HTMLEmbedElement::collectStyleForAttribute(Attribute* attr, StylePropertySet* style)
> +{
> +    if (attr->name() == hiddenAttr) {
> +        if (equalIgnoringCase(attr->value(), "yes") || equalIgnoringCase(attr->value(), "true")) {
> +            addCSSLengthToStyle(style, CSSPropertyWidth, "0"); // FIXME: Pass as integer.
> +            addCSSLengthToStyle(style, CSSPropertyHeight, "0"); // FIXME: Pass as integer.
> +        }
> +    } else
> +        HTMLPlugInImageElement::parseAttribute(attr);

This is calling collectStyleForAttribute-> parseAttribute (as picked up by Darin watching over the shoulder while I was going through the patch)

[WebKit Review Bot]

Comment on attachment 126571 [details]
WIP for EWS and the overly curious

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

New failing tests:
plugins/mouse-click-plugin-clears-selection.html
plugins/netscape-plugin-setwindow-size.html
plugins/netscape-plugin-setwindow-size-2.html
svg/W3C-I18N/g-dirRTL-ubOverride.svg
http/tests/inspector/inspect-element.html
css2.1/20110323/abspos-containing-block-initial-004b.htm
plugins/application-plugin-plugins-disabled.html
fast/backgrounds/animated-svg-as-mask.html
plugins/object-embed-plugin-scripting.html
svg/W3C-SVG-1.1/animate-elem-02-t.svg
css2.1/20110323/abspos-containing-block-initial-004d.htm
svg/W3C-I18N/g-dirLTR-ubNone.svg
svg/W3C-SVG-1.1/animate-elem-03-t.svg
svg/W3C-SVG-1.1/animate-elem-04-t.svg
svg/W3C-SVG-1.1/animate-elem-07-t.svg
fast/text/whitespace/pre-wrap-overflow-selection.html
plugins/netscape-dom-access.html
svg/W3C-I18N/g-dirLTR-ubOverride.svg
accessibility/aria-describedby-on-input.html
svg/W3C-I18N/text-anchor-dirLTR-anchorEnd.svg
fast/text/lang-mapped-to-webkit-locale.xhtml
svg/W3C-SVG-1.1/animate-elem-05-t.svg
platform/chromium-linux/svg/text/text-with-geometric-precision.svg
svg/W3C-SVG-1.1/animate-elem-06-t.svg
svg/W3C-I18N/g-dirRTL-ubNone.svg

[Andreas Kling]

Created attachment 126627 [details]
Perplexed patch

[WebKit Review Bot]

Attachment 126627 [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/html/HTMLElement.cpp:215:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/html/HTMLElement.cpp:216:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/html/HTMLElement.cpp:217:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/html/HTMLElement.cpp:218:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/html/HTMLElement.cpp:219:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/html/HTMLParagraphElement.h:38:  The parameter name "style" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLIFrameElement.h:39:  The parameter name "style" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLTableElement.cpp:278:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Source/WebCore/html/HTMLIFrameElement.cpp:60:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 9 in 66 files


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

[Andreas Kling]

Created attachment 126628 [details]
Porpoise patch

Same patch with stylebot fixes.

[Antti Koivisto]

Comment on attachment 126628 [details]
Porpoise patch

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

r=me

Clearly there would be stuff to tweak here but it is good to get this in first.

> Source/WebCore/dom/ElementAttributeData.cpp:56
> +StylePropertySet* ElementAttributeData::ensureNewAttributeStyle(StyledElement* element)

"ensureNew" sounds bit strange though I don't have good suggestions.

> Source/WebCore/dom/StyledElement.h:72
> +    static void addCSSLengthToStyle(StylePropertySet*, int propertyID, const String& value);
> +    static void addCSSColorToStyle(StylePropertySet*, int propertyID, const String& color);

Since these are HTML specific maybe they could move down to HTMLElement

> Source/WebCore/html/HTMLTableColElement.cpp:71
> +        setNeedsAttributeStyleUpdate();
>          if (!attr->value().isEmpty()) {
> -            addCSSLength(CSSPropertyWidth, attr->value());
>              if (renderer() && renderer()->isTableCol()) {
>                  RenderTableCol* col = toRenderTableCol(renderer());
>                  int newWidth = width().toInt();
>                  if (newWidth != col->width())
>                      col->setNeedsLayoutAndPrefWidthsRecalc();

I suppose width() still gets calculated here?

[Andreas Kling]

Committed r107484: <http://trac.webkit.org/changeset/107484>

[Andreas Kling]

(In reply to comment #8)
> (From update of attachment 126628 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126628&action=review
> 
> > Source/WebCore/dom/ElementAttributeData.cpp:56
> > +StylePropertySet* ElementAttributeData::ensureNewAttributeStyle(StyledElement* element)
> 
> "ensureNew" sounds bit strange though I don't have good suggestions.

Used a setter instead as discussed.

> 
> > Source/WebCore/dom/StyledElement.h:72
> > +    static void addCSSLengthToStyle(StylePropertySet*, int propertyID, const String& value);
> > +    static void addCSSColorToStyle(StylePropertySet*, int propertyID, const String& color);
> 
> Since these are HTML specific maybe they could move down to HTMLElement

Done.

> > Source/WebCore/html/HTMLTableColElement.cpp:71
> > +        setNeedsAttributeStyleUpdate();
> >          if (!attr->value().isEmpty()) {
> > -            addCSSLength(CSSPropertyWidth, attr->value());
> >              if (renderer() && renderer()->isTableCol()) {
> >                  RenderTableCol* col = toRenderTableCol(renderer());
> >                  int newWidth = width().toInt();
> >                  if (newWidth != col->width())
> >                      col->setNeedsLayoutAndPrefWidthsRecalc();
> 
> I suppose width() still gets calculated here?

width() simply does getAttribute(widthAttr). We should use attr->value() instead for that here, but that's for another patch.