Bug 76876 – Make elements that don't have attributes smaller.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Andreas Kling]

Because you're worth it.

[Andreas Kling]

Created attachment 123652 [details]
Patch

[WebKit Review Bot]

Attachment 123652 [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/StyledElement.h:93:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 5 files


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

[WebKit Review Bot]

Comment on attachment 123652 [details]
Patch

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

New failing tests:
fast/css/getComputedStyle/computed-style-border-image.html

[Antti Koivisto]

Comment on attachment 123652 [details]
Patch

It would be good to add some comments to NamedNodeMap about the refactoring plans. The class is getting increasingly confusing as a place to hang random stuff.

[Andreas Kling]

Committed r105738: <http://trac.webkit.org/changeset/105738>

[Csaba Osztrogonác]

Reopen, because it made fast/css/getComputedStyle/computed-style-border-image.html crash (at least) on Qt. Could you check what happened?

[Andreas Kling]

(In reply to comment #6)
> Reopen, because it made fast/css/getComputedStyle/computed-style-border-image.html crash (at least) on Qt. Could you check what happened?

Naturally. Here's the stack:

#0  WebCore::CSSValue::cssText (this=0x0) at CSSValue.h:153
#1  0x013fddbd in WebCore::CSSValueList::customCssText () at /Users/andreaskling/src/WebKit/Source/WebCore/css/CSSValueList.cpp:133
#2  0x013fcd0a in WebCore::CSSValue::cssText (this=<value temporarily unavailable, due to optimizations>) at /Users/andreaskling/src/WebKit/Source/WebCore/css/CSSValue.cpp:145
#3  0x013b1066 in WebCore::CSSProperty::cssText (this=0x2fb5990) at /Users/andreaskling/src/WebKit/Source/WebCore/css/CSSProperty.cpp:32
#4  0x01386ff1 in WebCore::CSSMutableStyleDeclaration::cssText () at /Users/andreaskling/src/WebKit/Source/WebCore/css/CSSMutableStyleDeclaration.cpp:862
#5  0x01c3abfa in WebCore::StyledElement::updateStyleAttribute (this=0x2fb5f20) at /Users/andreaskling/src/WebKit/Source/WebCore/dom/StyledElement.cpp:116
#6  0x012e3da6 in WebCore::StyledElement::ensureInlineStyleDecl (this=0x2fb5f20) at Element.h:495
#7  0x017cacae in WebCore::jsElementStyle (exec=0xc8ea1a0, slotBase={u = {asInt64 = -21284731472, asDouble = -nan(0xffffb0b54c5b0), asBits = {payload = 190105008, tag = -5}}}) at /Users/andreaskling/Source/Apple/Build/Release/DerivedSources/WebCore/JSElement.cpp:270

I wonder why we have null CSSValues in the CSSValueList here, this smells fishy.

[Andreas Kling]

Looks like I exposed a bug that was introduced in <http://trac.webkit.org/changeset/105502>

[Andreas Kling]

Committed r105999: <http://trac.webkit.org/changeset/105999>

[Levi Weintraub]

(In reply to comment #9)
> Committed r105999: <http://trac.webkit.org/changeset/105999>

I'm worried that this caused a performance regression. Here is the graph: http://build.chromium.org/f/chromium/perf/mac-release-10.5/dhtml/report.html?history=150&rev=-1

I'm running this test with all Chromium changes up to the latest WebKit roll, which included only these revisions: http://trac.webkit.org/log/?rev=106007&stop_rev=105998&verbose=on

[Levi Weintraub]

We did a perf test right before this change, and the regression wasn't present. I'm going to rever this tomorrow morning and try again just for confirmation. Let me know if you have an objection.

[Andreas Kling]

(In reply to comment #11)
> We did a perf test right before this change, and the regression wasn't present. I'm going to rever this tomorrow morning and try again just for confirmation. Let me know if you have an objection.

Feel free to roll out, I won't have a chance to look until tomorrow anyway. :/

[Levi Weintraub]

(In reply to comment #12)
> Feel free to roll out, I won't have a chance to look until tomorrow anyway. :/

Thanks! Just did. We'll know by tomorrow if this was the root cause. Sorry for the bother.

[Levi Weintraub]

(In reply to comment #13)
> Thanks! Just did. We'll know by tomorrow if this was the root cause. Sorry for the bother.

http://trac.webkit.org/changeset/106201 was the rollout, and it did fix this performance regression.

[Andreas Kling]

Reopening to roll back in.

[Andreas Kling]

Created attachment 124878 [details]
Patch v2

AFAICT the performance regression was introduced by causing us to unnecessarily serialize the inline style declaration into the element's 'style' attribute a lot more often. This happened because I used Element::attributes(false) to force creation of the element's attribute map. This time around, I've added a single-purpose ensureAttributeMap() for that instead.

[WebKit Review Bot]

Comment on attachment 124878 [details]
Patch v2

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

New failing tests:
fast/doctypes/002.html

[Andreas Kling]

Committed r106435: <http://trac.webkit.org/changeset/106435>