Summary: | Element::attributeChanged shouldn't do any work when attribute value didn't change | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||
Component: | DOM | Assignee: | Ryosuke Niwa <rniwa> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | barraclough, benjamin, ggaren, kling, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Ryosuke Niwa
2014-02-27 21:01:28 PST
Created attachment 225438 [details]
Fixes the bug
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 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. Committed r164856: <http://trac.webkit.org/changeset/164856> |