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

https://bugs.webkit.org/show_bug.cgi?id=129467

Reviewed by Geoffrey Garen.

Exit early in childrenChanged when the attribute value didn't change.

* dom/Attr.cpp:
(WebCore::Attr::setValue):
(WebCore::Attr::childrenChanged):
* dom/Element.cpp:
(WebCore::Element::setAttributeInternal):
(WebCore::Element::attributeChanged):
(WebCore::Element::parserSetAttributes):
(WebCore::Element::removeAttributeInternal):
(WebCore::Element::didAddAttribute):
(WebCore::Element::didModifyAttribute):
(WebCore::Element::didRemoveAttribute):
(WebCore::Element::cloneAttributesFromElement):
* dom/Element.h:
* dom/StyledElement.cpp:
(WebCore::StyledElement::attributeChanged):
* dom/StyledElement.h:
* html/HTMLInputElement.cpp:
(WebCore::HTMLInputElement::updateType):
* mathml/MathMLElement.cpp:
(WebCore::MathMLElement::attributeChanged):
* mathml/MathMLElement.h:
* mathml/MathMLSelectElement.cpp:
(WebCore::MathMLSelectElement::attributeChanged):
* mathml/MathMLSelectElement.h:
* svg/SVGElement.cpp:
(WebCore::SVGElement::attributeChanged):
* svg/SVGElement.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@164856 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 4fe8b5d3efbc461af0b52035a60d8a1d8af9cefa..005c273384049548643dad2e7bdf2cdbfa545192 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,40 @@
+2014-02-27  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Element::attributeChanged shouldn't do any work when attribute value didn't change
+        https://bugs.webkit.org/show_bug.cgi?id=129467
+
+        Reviewed by Geoffrey Garen.
+
+        Exit early in childrenChanged when the attribute value didn't change.
+
+        * dom/Attr.cpp:
+        (WebCore::Attr::setValue):
+        (WebCore::Attr::childrenChanged):
+        * dom/Element.cpp:
+        (WebCore::Element::setAttributeInternal):
+        (WebCore::Element::attributeChanged):
+        (WebCore::Element::parserSetAttributes):
+        (WebCore::Element::removeAttributeInternal):
+        (WebCore::Element::didAddAttribute):
+        (WebCore::Element::didModifyAttribute):
+        (WebCore::Element::didRemoveAttribute):
+        (WebCore::Element::cloneAttributesFromElement):
+        * dom/Element.h:
+        * dom/StyledElement.cpp:
+        (WebCore::StyledElement::attributeChanged):
+        * dom/StyledElement.h:
+        * html/HTMLInputElement.cpp:
+        (WebCore::HTMLInputElement::updateType):
+        * mathml/MathMLElement.cpp:
+        (WebCore::MathMLElement::attributeChanged):
+        * mathml/MathMLElement.h:
+        * mathml/MathMLSelectElement.cpp:
+        (WebCore::MathMLSelectElement::attributeChanged):
+        * mathml/MathMLSelectElement.h:
+        * svg/SVGElement.cpp:
+        (WebCore::SVGElement::attributeChanged):
+        * svg/SVGElement.h:
+
 2014-02-27  Jinwoo Song  <jinwoo7.song@samsung.com>
 
         [EFL] Remove duplicated keyboard string key from keyMap

diff --git a/Source/WebCore/dom/Attr.cpp b/Source/WebCore/dom/Attr.cpp
index 2deb1843cc7791fe2e39f1cb537ffdf18c4be674..50ff0b67afc434666bf0cd1926cad62d9b54c6c8 100644 (file)
--- a/Source/WebCore/dom/Attr.cpp
+++ b/Source/WebCore/dom/Attr.cpp
@@ -122,13 +122,14 @@ void Attr::setValue(const AtomicString& value)
 
 void Attr::setValue(const AtomicString& value, ExceptionCode&)
 {
+    AtomicString oldValue = this->value();
     if (m_element)
-        m_element->willModifyAttribute(qualifiedName(), this->value(), value);
+        m_element->willModifyAttribute(qualifiedName(), oldValue, value);
 
     setValue(value);
 
     if (m_element)
-        m_element->didModifyAttribute(qualifiedName(), value);
+        m_element->didModifyAttribute(qualifiedName(), oldValue, value);
 }
 
 void Attr::setNodeValue(const String& v, ExceptionCode& ec)
@@ -165,9 +166,10 @@ void Attr::childrenChanged(const ChildChange&)
     StringBuilder valueBuilder;
     TextNodeTraversal::appendContents(this, valueBuilder);
 
+    AtomicString oldValue = value();
     AtomicString newValue = valueBuilder.toAtomicString();
     if (m_element)
-        m_element->willModifyAttribute(qualifiedName(), value(), newValue);
+        m_element->willModifyAttribute(qualifiedName(), oldValue, newValue);
 
     if (m_element)
         elementAttribute().setValue(newValue);
@@ -175,7 +177,7 @@ void Attr::childrenChanged(const ChildChange&)
         m_standaloneValue = newValue;
 
     if (m_element)
-        m_element->attributeChanged(qualifiedName(), newValue);
+        m_element->attributeChanged(qualifiedName(), oldValue, newValue);
 }
 
 bool Attr::isId() const

diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
index df2a7c7aeeaa729420066195cec5f2b641d58644..3408df5755d0613d8cd6e6a256a365b536cb8f9b 100644 (file)
--- a/Source/WebCore/dom/Element.cpp
+++ b/Source/WebCore/dom/Element.cpp
@@ -1025,11 +1025,13 @@ inline void Element::setAttributeInternal(unsigned index, const QualifiedName& n
         return;
     }
 
-    bool valueChanged = newValue != attributeAt(index).value();
-    QualifiedName attributeName = (!inSynchronizationOfLazyAttribute || valueChanged) ? attributeAt(index).name() : name;
+    const Attribute& attribute = attributeAt(index);
+    AtomicString oldValue = attribute.value();
+    bool valueChanged = newValue != oldValue;
+    const QualifiedName& attributeName = (!inSynchronizationOfLazyAttribute || valueChanged) ? attribute.name() : name;
 
     if (!inSynchronizationOfLazyAttribute)
-        willModifyAttribute(attributeName, attributeAt(index).value(), newValue);
+        willModifyAttribute(attributeName, oldValue, newValue);
 
     if (valueChanged) {
         // If there is an Attr node hooked to this attribute, the Attr::setValue() call below
@@ -1042,7 +1044,7 @@ inline void Element::setAttributeInternal(unsigned index, const QualifiedName& n
     }
 
     if (!inSynchronizationOfLazyAttribute)
-        didModifyAttribute(attributeName, newValue);
+        didModifyAttribute(attributeName, oldValue, newValue);
 }
 
 static inline AtomicString makeIdForStyleResolution(const AtomicString& value, bool inQuirksMode)
@@ -1062,12 +1064,15 @@ static bool checkNeedsStyleInvalidationForIdChange(const AtomicString& oldId, co
     return false;
 }
 
-void Element::attributeChanged(const QualifiedName& name, const AtomicString& newValue, AttributeModificationReason)
+void Element::attributeChanged(const QualifiedName& name, const AtomicString& oldValue, const AtomicString& newValue, AttributeModificationReason)
 {
     parseAttribute(name, newValue);
 
     document().incDOMTreeVersion();
 
+    if (oldValue == newValue)
+        return;
+
     StyleResolver* styleResolver = document().styleResolverIfExists();
     bool testShouldInvalidateStyle = inRenderedDocument() && styleResolver && styleChangeType() < FullStyleChange;
     bool shouldInvalidateStyle = false;
@@ -1244,7 +1249,7 @@ void Element::parserSetAttributes(const Vector<Attribute>& attributeVector)
 
     // Use attributeVector instead of m_elementData because attributeChanged might modify m_elementData.
     for (unsigned i = 0; i < attributeVector.size(); ++i)
-        attributeChanged(attributeVector[i].name(), attributeVector[i].value(), ModifiedDirectly);
+        attributeChanged(attributeVector[i].name(), nullAtom, attributeVector[i].value(), ModifiedDirectly);
 }
 
 bool Element::hasAttributes() const
@@ -1803,7 +1808,7 @@ void Element::removeAttributeInternal(unsigned index, SynchronizationOfLazyAttri
     elementData.removeAttribute(index);
 
     if (!inSynchronizationOfLazyAttribute)
-        didRemoveAttribute(name);
+        didRemoveAttribute(name, valueBeingRemoved);
 }
 
 void Element::addAttributeInternal(const QualifiedName& name, const AtomicString& value, SynchronizationOfLazyAttribute inSynchronizationOfLazyAttribute)
@@ -2764,21 +2769,21 @@ void Element::willModifyAttribute(const QualifiedName& name, const AtomicString&
 
 void Element::didAddAttribute(const QualifiedName& name, const AtomicString& value)
 {
-    attributeChanged(name, value);
+    attributeChanged(name, nullAtom, value);
     InspectorInstrumentation::didModifyDOMAttr(&document(), this, name.localName(), value);
     dispatchSubtreeModifiedEvent();
 }
 
-void Element::didModifyAttribute(const QualifiedName& name, const AtomicString& value)
+void Element::didModifyAttribute(const QualifiedName& name, const AtomicString& oldValue, const AtomicString& newValue)
 {
-    attributeChanged(name, value);
-    InspectorInstrumentation::didModifyDOMAttr(&document(), this, name.localName(), value);
+    attributeChanged(name, oldValue, newValue);
+    InspectorInstrumentation::didModifyDOMAttr(&document(), this, name.localName(), newValue);
     // Do not dispatch a DOMSubtreeModified event here; see bug 81141.
 }
 
-void Element::didRemoveAttribute(const QualifiedName& name)
+void Element::didRemoveAttribute(const QualifiedName& name, const AtomicString& oldValue)
 {
-    attributeChanged(name, nullAtom);
+    attributeChanged(name, oldValue, nullAtom);
     InspectorInstrumentation::didRemoveDOMAttr(&document(), this, name.localName());
     dispatchSubtreeModifiedEvent();
 }
@@ -2979,9 +2984,8 @@ void Element::cloneAttributesFromElement(const Element& other)
     else
         m_elementData = other.m_elementData->makeUniqueCopy();
 
-    for (const Attribute& attribute : attributesIterator()) {
-        attributeChanged(attribute.name(), attribute.value(), ModifiedByCloning);
-    }
+    for (const Attribute& attribute : attributesIterator())
+        attributeChanged(attribute.name(), nullAtom, attribute.value(), ModifiedByCloning);
 }
 
 void Element::cloneDataFromElement(const Element& other)

diff --git a/Source/WebCore/dom/Element.h b/Source/WebCore/dom/Element.h
index 175f13a22776d21a160957a1eb454b074712ec8e..39667e3a314d32c004cba9e194908201fddd3435 100644 (file)
--- a/Source/WebCore/dom/Element.h
+++ b/Source/WebCore/dom/Element.h
@@ -290,7 +290,7 @@ public:
     };
 
     // This method is called whenever an attribute is added, changed or removed.
-    virtual void attributeChanged(const QualifiedName&, const AtomicString&, AttributeModificationReason = ModifiedDirectly);
+    virtual void attributeChanged(const QualifiedName&, const AtomicString& oldValue, const AtomicString& newValue, AttributeModificationReason = ModifiedDirectly);
     virtual void parseAttribute(const QualifiedName&, const AtomicString&) { }
 
     // Only called by the parser immediately after element construction.
@@ -612,8 +612,8 @@ private:
 
     void didAddAttribute(const QualifiedName&, const AtomicString&);
     void willModifyAttribute(const QualifiedName&, const AtomicString& oldValue, const AtomicString& newValue);
-    void didModifyAttribute(const QualifiedName&, const AtomicString&);
-    void didRemoveAttribute(const QualifiedName&);
+    void didModifyAttribute(const QualifiedName&, const AtomicString& oldValue, const AtomicString& newValue);
+    void didRemoveAttribute(const QualifiedName&, const AtomicString& oldValue);
 
     void synchronizeAttribute(const QualifiedName&) const;
     void synchronizeAttribute(const AtomicString& localName) const;

diff --git a/Source/WebCore/dom/StyledElement.cpp b/Source/WebCore/dom/StyledElement.cpp
index 3b9475dc4fc6631a8bc543c6be852e053b39b8a0..49b396586a7aa2ceeacf8e3e4b1ab9abba192f28 100644 (file)
--- a/Source/WebCore/dom/StyledElement.cpp
+++ b/Source/WebCore/dom/StyledElement.cpp
@@ -149,7 +149,7 @@ MutableStyleProperties& StyledElement::ensureMutableInlineStyle()
     return static_cast<MutableStyleProperties&>(*inlineStyle);
 }
 
-void StyledElement::attributeChanged(const QualifiedName& name, const AtomicString& newValue, AttributeModificationReason reason)
+void StyledElement::attributeChanged(const QualifiedName& name, const AtomicString& oldValue, const AtomicString& newValue, AttributeModificationReason reason)
 {
     if (name == styleAttr)
         styleAttributeChanged(newValue, reason);
@@ -158,7 +158,7 @@ void StyledElement::attributeChanged(const QualifiedName& name, const AtomicStri
         setNeedsStyleRecalc(InlineStyleChange);
     }
 
-    Element::attributeChanged(name, newValue, reason);
+    Element::attributeChanged(name, oldValue, newValue, reason);
 }
 
 PropertySetCSSStyleDeclaration* StyledElement::inlineStyleCSSOMWrapper()

diff --git a/Source/WebCore/dom/StyledElement.h b/Source/WebCore/dom/StyledElement.h
index 2d80c3416f3ea6b1d681f0c400a07e541a291818..565501b7514263f8f289f94cb0d856e6eb32270e 100644 (file)
--- a/Source/WebCore/dom/StyledElement.h
+++ b/Source/WebCore/dom/StyledElement.h
@@ -69,7 +69,7 @@ protected:
     {
     }
 
-    virtual void attributeChanged(const QualifiedName&, const AtomicString&, AttributeModificationReason = ModifiedDirectly) override;
+    virtual void attributeChanged(const QualifiedName&, const AtomicString& oldValue, const AtomicString& newValue, AttributeModificationReason = ModifiedDirectly) override;
 
     virtual bool isPresentationAttribute(const QualifiedName&) const { return false; }
 

diff --git a/Source/WebCore/html/HTMLInputElement.cpp b/Source/WebCore/html/HTMLInputElement.cpp
index df20d3e6d044db0779ad51d6b1b6a0959a1fc3a3..18d04f9df30416baa3edb7dd902ee2324adf480c 100644 (file)
--- a/Source/WebCore/html/HTMLInputElement.cpp
+++ b/Source/WebCore/html/HTMLInputElement.cpp
@@ -513,12 +513,13 @@ void HTMLInputElement::updateType()
 
     if (didRespectHeightAndWidth != m_inputType->shouldRespectHeightAndWidthAttributes()) {
         ASSERT(elementData());
+        // FIXME: We don't have the old attribute values so we pretend that we didn't have the old values.
         if (const Attribute* height = findAttributeByName(heightAttr))
-            attributeChanged(heightAttr, height->value());
+            attributeChanged(heightAttr, nullAtom, height->value());
         if (const Attribute* width = findAttributeByName(widthAttr))
-            attributeChanged(widthAttr, width->value());
+            attributeChanged(widthAttr, nullAtom, width->value());
         if (const Attribute* align = findAttributeByName(alignAttr))
-            attributeChanged(alignAttr, align->value());
+            attributeChanged(alignAttr, nullAtom, align->value());
     }
 
     if (renderer())

diff --git a/Source/WebCore/mathml/MathMLElement.cpp b/Source/WebCore/mathml/MathMLElement.cpp
index 24017925158c3c4c40101e86dc01dcf10a1f72d2..42fe25d8a40ed241a130fecc1bd8b659286b1e72 100644 (file)
--- a/Source/WebCore/mathml/MathMLElement.cpp
+++ b/Source/WebCore/mathml/MathMLElement.cpp
@@ -132,14 +132,14 @@ bool MathMLElement::childShouldCreateRenderer(const Node& child) const
     return child.isTextNode() || child.isMathMLElement();
 }
 
-void MathMLElement::attributeChanged(const QualifiedName& name, const AtomicString& newValue, AttributeModificationReason reason)
+void MathMLElement::attributeChanged(const QualifiedName& name, const AtomicString& oldValue, const AtomicString& newValue, AttributeModificationReason reason)
 {
     if (isSemanticAnnotation() && (name == MathMLNames::srcAttr || name == MathMLNames::encodingAttr)) {
         Element* parent = parentElement();
         if (parent && parent->isMathMLElement() && parent->hasTagName(semanticsTag))
             toMathMLElement(parent)->updateSelectedChild();
     }
-    StyledElement::attributeChanged(name, newValue, reason);
+    StyledElement::attributeChanged(name, oldValue, newValue, reason);
 }
 
 }

diff --git a/Source/WebCore/mathml/MathMLElement.h b/Source/WebCore/mathml/MathMLElement.h
index 02f14a263532b3146e0897ca079e6fbac07ba188..9f988a9a9d61678f76fde4f972e194a495b38ba3 100644 (file)
--- a/Source/WebCore/mathml/MathMLElement.h
+++ b/Source/WebCore/mathml/MathMLElement.h
@@ -59,7 +59,7 @@ protected:
 
     virtual void parseAttribute(const QualifiedName&, const AtomicString&) override;
     virtual bool childShouldCreateRenderer(const Node&) const override;
-    virtual void attributeChanged(const QualifiedName&, const AtomicString& newValue, AttributeModificationReason) override;
+    virtual void attributeChanged(const QualifiedName&, const AtomicString& oldValue, const AtomicString& newValue, AttributeModificationReason) override;
 
     virtual bool isPresentationAttribute(const QualifiedName&) const override;
     virtual void collectStyleForPresentationAttribute(const QualifiedName&, const AtomicString&, MutableStyleProperties&) override;

diff --git a/Source/WebCore/mathml/MathMLSelectElement.cpp b/Source/WebCore/mathml/MathMLSelectElement.cpp
index a22d584bd2bd873e1319f21e7475523c964befe1..a127bfd854e995dad96b5ed79041731a36900603 100644 (file)
--- a/Source/WebCore/mathml/MathMLSelectElement.cpp
+++ b/Source/WebCore/mathml/MathMLSelectElement.cpp
@@ -69,12 +69,12 @@ void MathMLSelectElement::childrenChanged(const ChildChange& change)
     MathMLInlineContainerElement::childrenChanged(change);
 }
 
-void MathMLSelectElement::attributeChanged(const QualifiedName& name, const AtomicString& newValue, AttributeModificationReason reason)
+void MathMLSelectElement::attributeChanged(const QualifiedName& name, const AtomicString& oldValue, const AtomicString& newValue, AttributeModificationReason reason)
 {
     if (hasLocalName(mactionTag) && (name == MathMLNames::actiontypeAttr || name == MathMLNames::selectionAttr))
         updateSelectedChild();
 
-    MathMLInlineContainerElement::attributeChanged(name, newValue, reason);
+    MathMLInlineContainerElement::attributeChanged(name, oldValue, newValue, reason);
 }
 
 int MathMLSelectElement::getSelectedActionChildAndIndex(Element*& selectedChild)

diff --git a/Source/WebCore/mathml/MathMLSelectElement.h b/Source/WebCore/mathml/MathMLSelectElement.h
index 30f3ecc60a4842366d6b250ea43722025ef46c26..9c1b2ffdfc6e1ab4b3af392d2b30c7963d746945 100644 (file)
--- a/Source/WebCore/mathml/MathMLSelectElement.h
+++ b/Source/WebCore/mathml/MathMLSelectElement.h
@@ -43,7 +43,7 @@ private:
 
     virtual void finishParsingChildren() override;
     virtual void childrenChanged(const ChildChange&) override;
-    virtual void attributeChanged(const QualifiedName&, const AtomicString&, AttributeModificationReason = ModifiedDirectly) override;
+    virtual void attributeChanged(const QualifiedName&, const AtomicString& oldValue, const AtomicString& newValue, AttributeModificationReason = ModifiedDirectly) override;
     virtual void defaultEventHandler(Event*) override;
     virtual bool willRespondToMouseClickEvents() override;
 

diff --git a/Source/WebCore/svg/SVGElement.cpp b/Source/WebCore/svg/SVGElement.cpp
index 000b86c5398e218d4a14cef9f07059aee764c9b3..2029cb349cf446b79efec9063775f172e307f608 100644 (file)
--- a/Source/WebCore/svg/SVGElement.cpp
+++ b/Source/WebCore/svg/SVGElement.cpp
@@ -716,9 +716,9 @@ bool SVGElement::childShouldCreateRenderer(const Node& child) const
     return false;
 }
 
-void SVGElement::attributeChanged(const QualifiedName& name, const AtomicString& newValue, AttributeModificationReason)
+void SVGElement::attributeChanged(const QualifiedName& name, const AtomicString& oldValue, const AtomicString& newValue, AttributeModificationReason)
 {
-    StyledElement::attributeChanged(name, newValue);
+    StyledElement::attributeChanged(name, oldValue, newValue);
 
     if (isIdAttributeName(name))
         document().accessSVGExtensions()->rebuildAllElementReferencesForTarget(this);

diff --git a/Source/WebCore/svg/SVGElement.h b/Source/WebCore/svg/SVGElement.h
index 13478a8c0ae467d5923032dc29804ea8db65ad97..8c9f0f540464855de38c890cf878ac9ae5c83847 100644 (file)
--- a/Source/WebCore/svg/SVGElement.h
+++ b/Source/WebCore/svg/SVGElement.h
@@ -148,7 +148,7 @@ protected:
     virtual void parseAttribute(const QualifiedName&, const AtomicString&) override;
 
     virtual void finishParsingChildren() override;
-    virtual void attributeChanged(const QualifiedName&, const AtomicString&, AttributeModificationReason = ModifiedDirectly) override;
+    virtual void attributeChanged(const QualifiedName&, const AtomicString& oldValue, const AtomicString& newValue, AttributeModificationReason = ModifiedDirectly) override;
     virtual bool childShouldCreateRenderer(const Node&) const override;
 
     SVGElementRareData& ensureSVGRareData();