Bug 129467 – Element::attributeChanged shouldn't do any work when attribute value didn't change

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

[Ryosuke Niwa]

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.

[Radar WebKit Bug Importer]

<rdar://problem/16192801>

[Ryosuke Niwa]

Created attachment 225438 [details]
Fixes the bug

[Geoffrey Garen]

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?

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Committed r164856: <http://trac.webkit.org/changeset/164856>