Bug 129467 - Element::attributeChanged shouldn't do any work when attribute value didn't change
Summary: Element::attributeChanged shouldn't do any work when attribute value didn't c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-02-27 21:01 PST by Ryosuke Niwa
Modified: 2014-02-27 23:44 PST (History)
5 users (show)

See Also:

Attachments
Fixes the bug (18.69 KB, patch)
2014-02-27 22:46 PST, Ryosuke Niwa 		ggaren: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2014-02-27 21:01:28 PST 
Right now attributeChanged computes the id for style resolution and so forth even if the attribute is set to the same value.
Don't do that.
Comment 1 Radar WebKit Bug Importer 2014-02-27 21:02:12 PST 
<rdar://problem/16192801>
Comment 2 Ryosuke Niwa 2014-02-27 22:46:06 PST 
Created attachment 225438 [details]
Fixes the bug
Comment 3 Geoffrey Garen 2014-02-27 22:53:30 PST 
Comment on attachment 225438 [details]
Fixes the bug

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

> Source/WebCore/dom/Attr.cpp:125
> +    AtomicString oldValue = this->value();

const AtomicString&

> Source/WebCore/dom/Attr.cpp:169
> +    AtomicString oldValue = value();

const AtomicString&

> Source/WebCore/dom/Element.cpp:1029
> +    AtomicString oldValue = attribute.value();

const AtomicString&

> Source/WebCore/dom/Element.cpp:1031
> +    QualifiedName attributeName = (!inSynchronizationOfLazyAttribute || valueChanged) ? attribute.name() : name;

const QualifiedName&

> Source/WebCore/dom/Element.cpp:1074
>      document().incDOMTreeVersion();
>  
> +    if (oldValue == newValue)
> +        return;

Why do we incDOMTreeVersion unconditionally, even in the case where we're not going to make a change?
Comment 4 Ryosuke Niwa 2014-02-27 22:56:56 PST 
Comment on attachment 225438 [details]
Fixes the bug

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

>> Source/WebCore/dom/Attr.cpp:125
>> +    AtomicString oldValue = this->value();
> 
> const AtomicString&

Unfortunately we can't do that because setValue may remove the last ref to the old value.

>> Source/WebCore/dom/Attr.cpp:169
>> +    AtomicString oldValue = value();
> 
> const AtomicString&

Ditto.

>> Source/WebCore/dom/Element.cpp:1029
>> +    AtomicString oldValue = attribute.value();
> 
> const AtomicString&

Ditto.

>> Source/WebCore/dom/Element.cpp:1031
>> +    QualifiedName attributeName = (!inSynchronizationOfLazyAttribute || valueChanged) ? attribute.name() : name;
> 
> const QualifiedName&

Will fix this one.

>> Source/WebCore/dom/Element.cpp:1074
>> +        return;
> 
> Why do we incDOMTreeVersion unconditionally, even in the case where we're not going to make a change?

There are some DOM elements that do work inside parseAttribute or in overridden attributedChanged
even if the old value matched the new value so I think it's a good idea to always increment the DOM tree version here.
Comment 5 Ryosuke Niwa 2014-02-27 23:44:53 PST 
Committed r164856: <http://trac.webkit.org/changeset/164856>