Avoid unnecessary work when removing attributes from an element
authoradamk@chromium.org <adamk@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 21 Dec 2011 01:32:54 +0000 (01:32 +0000)
committeradamk@chromium.org <adamk@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 21 Dec 2011 01:32:54 +0000 (01:32 +0000)
https://bugs.webkit.org/show_bug.cgi?id=74953

Reviewed by Ryosuke Niwa.

Source/WebCore:

Various codepaths in Element and NamedNodeMap repeatedly search
through the list of attributes during a single operation. To avoid
this, I've added new getters to NamedNodeMap that return indices
rather than Attribute*s (they return WTF::notFound if no match is
found). These new methods are now used during removeAttribute
operations, as well as setAttribute and NamedNodeMap::setNamedItem
(along with a new replaceAttribute helper method).

The other optimization here involves the creation/destruction
of never-references Attr nodes. This is now avoided by calling
NamedNodeMap::removeAttribute directly instead of going through
NamedNodeMap::removeNamedItem.

As a cleanup after the above changes, the ExceptionCode argument is
gone from Element::removeAttribute and friends (it was never set
previously). The bulk of the files mentioned below are simply updating
callers to these methods.

No new tests, no change in behavior expected.

* dom/DatasetDOMStringMap.cpp:
(WebCore::DatasetDOMStringMap::deleteItem):
* dom/Element.cpp:
(WebCore::Element::removeAttribute):
(WebCore::Element::setBooleanAttribute):
(WebCore::Element::removeAttributeNS):
(WebCore::Element::setAttribute):
(WebCore::Element::setAttributeInternal):
* dom/Element.h:
* dom/Element.idl:
* dom/NamedNodeMap.cpp:
(WebCore::NamedNodeMap::setNamedItem):
(WebCore::NamedNodeMap::removeNamedItem):
(WebCore::NamedNodeMap::getAttributeItemIndexSlowCase):
(WebCore::NamedNodeMap::replaceAttribute):
(WebCore::NamedNodeMap::removeAttribute):
* dom/NamedNodeMap.h:
(WebCore::NamedNodeMap::getAttributeItem):
(WebCore::NamedNodeMap::getAttributeItemIndex):
(WebCore::NamedNodeMap::removeAttribute):
* editing/ApplyStyleCommand.cpp:
(WebCore::ApplyStyleCommand::pushDownInlineStyleAroundNode):
* editing/InsertParagraphSeparatorCommand.cpp:
(WebCore::InsertParagraphSeparatorCommand::cloneHierarchyUnderNewBlock):
* editing/SplitElementCommand.cpp:
(WebCore::SplitElementCommand::executeApply):
* html/HTMLElement.cpp:
(WebCore::HTMLElement::setContentEditable):
* html/HTMLInputElement.cpp:
(WebCore::HTMLInputElement::setType):
* inspector/InspectorDOMAgent.cpp:
(WebCore::InspectorDOMAgent::setAttributesAsText):
(WebCore::InspectorDOMAgent::removeAttribute):
* svg/SVGUseElement.cpp:
(WebCore::SVGUseElement::transferUseAttributesToReplacedElement):

Source/WebKit/qt:

* Api/qwebelement.cpp:
(QWebElement::removeAttribute): Updated a caller of
WebCore::Element::removeAttribute to remove ExceptionCode argument.

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

16 files changed:
Source/WebCore/ChangeLog
Source/WebCore/dom/DatasetDOMStringMap.cpp
Source/WebCore/dom/Element.cpp
Source/WebCore/dom/Element.h
Source/WebCore/dom/Element.idl
Source/WebCore/dom/NamedNodeMap.cpp
Source/WebCore/dom/NamedNodeMap.h
Source/WebCore/editing/ApplyStyleCommand.cpp
Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp
Source/WebCore/editing/SplitElementCommand.cpp
Source/WebCore/html/HTMLElement.cpp
Source/WebCore/html/HTMLInputElement.cpp
Source/WebCore/inspector/InspectorDOMAgent.cpp
Source/WebCore/svg/SVGUseElement.cpp
Source/WebKit/qt/Api/qwebelement.cpp
Source/WebKit/qt/ChangeLog

index 95663ff..81f4efa 100644 (file)
@@ -1,3 +1,66 @@
+2011-12-20  Adam Klein  <adamk@chromium.org>
+
+        Avoid unnecessary work when removing attributes from an element
+        https://bugs.webkit.org/show_bug.cgi?id=74953
+
+        Reviewed by Ryosuke Niwa.
+
+        Various codepaths in Element and NamedNodeMap repeatedly search
+        through the list of attributes during a single operation. To avoid
+        this, I've added new getters to NamedNodeMap that return indices
+        rather than Attribute*s (they return WTF::notFound if no match is
+        found). These new methods are now used during removeAttribute
+        operations, as well as setAttribute and NamedNodeMap::setNamedItem
+        (along with a new replaceAttribute helper method).
+
+        The other optimization here involves the creation/destruction
+        of never-references Attr nodes. This is now avoided by calling
+        NamedNodeMap::removeAttribute directly instead of going through
+        NamedNodeMap::removeNamedItem.
+
+        As a cleanup after the above changes, the ExceptionCode argument is
+        gone from Element::removeAttribute and friends (it was never set
+        previously). The bulk of the files mentioned below are simply updating
+        callers to these methods.
+
+        No new tests, no change in behavior expected.
+
+        * dom/DatasetDOMStringMap.cpp:
+        (WebCore::DatasetDOMStringMap::deleteItem):
+        * dom/Element.cpp:
+        (WebCore::Element::removeAttribute):
+        (WebCore::Element::setBooleanAttribute):
+        (WebCore::Element::removeAttributeNS):
+        (WebCore::Element::setAttribute):
+        (WebCore::Element::setAttributeInternal):
+        * dom/Element.h:
+        * dom/Element.idl:
+        * dom/NamedNodeMap.cpp:
+        (WebCore::NamedNodeMap::setNamedItem):
+        (WebCore::NamedNodeMap::removeNamedItem):
+        (WebCore::NamedNodeMap::getAttributeItemIndexSlowCase):
+        (WebCore::NamedNodeMap::replaceAttribute):
+        (WebCore::NamedNodeMap::removeAttribute):
+        * dom/NamedNodeMap.h:
+        (WebCore::NamedNodeMap::getAttributeItem):
+        (WebCore::NamedNodeMap::getAttributeItemIndex):
+        (WebCore::NamedNodeMap::removeAttribute):
+        * editing/ApplyStyleCommand.cpp:
+        (WebCore::ApplyStyleCommand::pushDownInlineStyleAroundNode):
+        * editing/InsertParagraphSeparatorCommand.cpp:
+        (WebCore::InsertParagraphSeparatorCommand::cloneHierarchyUnderNewBlock):
+        * editing/SplitElementCommand.cpp:
+        (WebCore::SplitElementCommand::executeApply):
+        * html/HTMLElement.cpp:
+        (WebCore::HTMLElement::setContentEditable):
+        * html/HTMLInputElement.cpp:
+        (WebCore::HTMLInputElement::setType):
+        * inspector/InspectorDOMAgent.cpp:
+        (WebCore::InspectorDOMAgent::setAttributesAsText):
+        (WebCore::InspectorDOMAgent::removeAttribute):
+        * svg/SVGUseElement.cpp:
+        (WebCore::SVGUseElement::transferUseAttributesToReplacedElement):
+
 2011-12-20  Eric Carlson  <eric.carlson@apple.com>
 
         WebVTT cues sometimes render when they should not
index e68e9d9..e0d875d 100644 (file)
@@ -199,8 +199,7 @@ void DatasetDOMStringMap::deleteItem(const String& name, ExceptionCode& ec)
         return;
     }
 
-    ExceptionCode dummy;
-    m_element->removeAttribute(convertPropertyNameToAttributeName(name), dummy);
+    m_element->removeAttribute(convertPropertyNameToAttributeName(name));
 }
 
 } // namespace WebCore
index b15003c..0fdb577 100644 (file)
@@ -188,24 +188,26 @@ void Element::enqueueAttributesMutationRecordIfRequested(const QualifiedName& at
 }
 #endif
 
-void Element::removeAttribute(const QualifiedName& name, ExceptionCode& ec)
+void Element::removeAttribute(const QualifiedName& name)
 {
-    if (m_attributeMap) {
-        ec = 0;
-        RefPtr<Node> attrNode = m_attributeMap->removeNamedItem(name, ec);
-        if (ec == NOT_FOUND_ERR)
-            ec = 0;
-    }
+    if (!m_attributeMap)
+        return;
+
+    size_t index = m_attributeMap->getAttributeItemIndex(name);
+    if (index == notFound)
+        return;
+
+    willModifyAttribute(name, m_attributeMap->attributeItem(index)->value(), nullAtom);
+
+    m_attributeMap->removeAttribute(index);
 }
 
-void Element::setBooleanAttribute(const QualifiedName& name, bool b)
+void Element::setBooleanAttribute(const QualifiedName& name, bool value)
 {
-    if (b)
+    if (value)
         setAttribute(name, emptyAtom);
-    else {
-        ExceptionCode ex;
-        removeAttribute(name, ex);
-    }
+    else
+        removeAttribute(name);
 }
 
 Node::NodeType Element::nodeType() const
@@ -627,20 +629,20 @@ void Element::setAttribute(const AtomicString& name, const AtomicString& value,
 
     const AtomicString& localName = shouldIgnoreAttributeCase(this) ? name.lower() : name;
 
-    // Allocate attribute map if necessary.
-    Attribute* old = attributes(false)->getAttributeItem(localName, false);
-    setAttributeInternal(old, old ? old->name() : QualifiedName(nullAtom, localName, nullAtom), value);
+    size_t index = attributes(false)->getAttributeItemIndex(localName, false);
+    const QualifiedName& qName = index != notFound ? m_attributeMap->attributeItem(index)->name() : QualifiedName(nullAtom, localName, nullAtom);
+    setAttributeInternal(index, qName, value);
 }
 
 void Element::setAttribute(const QualifiedName& name, const AtomicString& value)
 {
-    // Allocate attribute map if necessary.
-    Attribute* old = attributes(false)->getAttributeItem(name);
-    setAttributeInternal(old, name, value);
+    setAttributeInternal(attributes(false)->getAttributeItemIndex(name), name, value);
 }
 
-inline void Element::setAttributeInternal(Attribute* old, const QualifiedName& name, const AtomicString& value)
+inline void Element::setAttributeInternal(size_t index, const QualifiedName& name, const AtomicString& value)
 {
+    Attribute* old = index != notFound ? m_attributeMap->attributeItem(index) : 0;
+
 #if ENABLE(INSPECTOR)
     if (!isSynchronizingStyleAttribute())
         InspectorInstrumentation::willModifyDOMAttr(document(), this);
@@ -651,7 +653,7 @@ inline void Element::setAttributeInternal(Attribute* old, const QualifiedName& n
     willModifyAttribute(name, old ? old->value() : nullAtom, value);
 
     if (old && value.isNull())
-        m_attributeMap->removeAttribute(name);
+        m_attributeMap->removeAttribute(index);
     else if (!old && !value.isNull())
         m_attributeMap->addAttribute(createAttribute(name, value));
     else if (old) {
@@ -1461,25 +1463,31 @@ void Element::setAttributeNS(const AtomicString& namespaceURI, const AtomicStrin
     setAttribute(qName, value);
 }
 
-void Element::removeAttribute(const String& name, ExceptionCode& ec)
+void Element::removeAttribute(const String& name)
 {
-    InspectorInstrumentation::willModifyDOMAttr(document(), this);
+    if (!m_attributeMap)
+        return;
 
     String localName = shouldIgnoreAttributeCase(this) ? name.lower() : name;
+    size_t index = m_attributeMap->getAttributeItemIndex(localName, false);
+    if (index == notFound)
+        return;
+
+    Attribute* attribute = m_attributeMap->attributeItem(index);
+
+    InspectorInstrumentation::willModifyDOMAttr(document(), this);
+
+    if (!attribute->isNull())
+        willModifyAttribute(attribute->name(), attribute->value(), nullAtom);
+
+    m_attributeMap->removeAttribute(index);
 
-    if (m_attributeMap) {
-        ec = 0;
-        RefPtr<Node> attrNode = m_attributeMap->removeNamedItem(localName, ec);
-        if (ec == NOT_FOUND_ERR)
-            ec = 0;
-    }
-    
     InspectorInstrumentation::didRemoveDOMAttr(document(), this, name);
 }
 
-void Element::removeAttributeNS(const String& namespaceURI, const String& localName, ExceptionCode& ec)
+void Element::removeAttributeNS(const String& namespaceURI, const String& localName)
 {
-    removeAttribute(QualifiedName(nullAtom, localName, namespaceURI), ec);
+    removeAttribute(QualifiedName(nullAtom, localName, namespaceURI));
 }
 
 PassRefPtr<Attr> Element::getAttributeNode(const String& name)
index 620a0b3..92d59c9 100644 (file)
@@ -110,7 +110,7 @@ public:
     bool hasAttribute(const QualifiedName&) const;
     const AtomicString& getAttribute(const QualifiedName&) const;
     void setAttribute(const QualifiedName&, const AtomicString& value);
-    void removeAttribute(const QualifiedName&, ExceptionCode&);
+    void removeAttribute(const QualifiedName&);
 
     // Typed getters and setters for language bindings.
     int getIntegralAttribute(const QualifiedName& attributeName) const;
@@ -176,8 +176,8 @@ public:
     // Returns the absolute bounding box translated into screen coordinates:
     LayoutRect screenRect() const;
 
-    void removeAttribute(const String& name, ExceptionCode&);
-    void removeAttributeNS(const String& namespaceURI, const String& localName, ExceptionCode&);
+    void removeAttribute(const String& name);
+    void removeAttributeNS(const String& namespaceURI, const String& localName);
 
     PassRefPtr<Attr> getAttributeNode(const String& name);
     PassRefPtr<Attr> getAttributeNodeNS(const String& namespaceURI, const String& localName);
@@ -395,7 +395,7 @@ private:
     virtual NodeType nodeType() const;
     virtual bool childTypeAllowed(NodeType) const;
 
-    void setAttributeInternal(Attribute* old, const QualifiedName&, const AtomicString& value);
+    void setAttributeInternal(size_t index, const QualifiedName&, const AtomicString& value);
     virtual PassRefPtr<Attribute> createAttribute(const QualifiedName&, const AtomicString& value);
     
 #ifndef NDEBUG
index 81032e6..81fb260 100644 (file)
@@ -33,8 +33,7 @@ module core {
         [OldStyleObjC] void setAttribute(in [Optional=CallWithDefaultValue] DOMString name,
                                          in [Optional=CallWithDefaultValue] DOMString value)
             raises(DOMException);
-        void removeAttribute(in [Optional=CallWithDefaultValue] DOMString name)
-            raises(DOMException);
+        void removeAttribute(in [Optional=CallWithDefaultValue] DOMString name);
         Attr getAttributeNode(in [Optional=CallWithDefaultValue] DOMString name);
         Attr setAttributeNode(in [Optional=CallWithDefaultValue] Attr newAttr)
             raises(DOMException);
@@ -51,8 +50,7 @@ module core {
                                            in [Optional=CallWithDefaultValue] DOMString value)
             raises(DOMException);
         [OldStyleObjC] void removeAttributeNS(in [ConvertNullToNullString] DOMString namespaceURI,
-                                              in DOMString localName)
-            raises(DOMException);
+                                              in DOMString localName);
         [OldStyleObjC] NodeList getElementsByTagNameNS(in [ConvertNullToNullString,Optional=CallWithDefaultValue] DOMString namespaceURI,
                                                        in [Optional=CallWithDefaultValue] DOMString localName);
         [OldStyleObjC] Attr getAttributeNodeNS(in [ConvertNullToNullString,Optional=CallWithDefaultValue] DOMString namespaceURI,
index 86b7de3..0149d54 100644 (file)
@@ -108,7 +108,8 @@ PassRefPtr<Node> NamedNodeMap::setNamedItem(Node* node, ExceptionCode& ec)
     Attr* attr = static_cast<Attr*>(node);
 
     Attribute* attribute = attr->attr();
-    Attribute* oldAttribute = getAttributeItem(attribute->name());
+    size_t index = getAttributeItemIndex(attribute->name());
+    Attribute* oldAttribute = index != notFound ? attributeItem(index) : 0;
     if (oldAttribute == attribute)
         return node; // we know about it already
 
@@ -125,10 +126,9 @@ PassRefPtr<Node> NamedNodeMap::setNamedItem(Node* node, ExceptionCode& ec)
     RefPtr<Attr> oldAttr;
     if (oldAttribute) {
         oldAttr = oldAttribute->createAttrIfNeeded(m_element);
-        removeAttribute(attribute->name());
-    }
-
-    addAttribute(attribute);
+        replaceAttribute(index, attribute);
+    } else
+        addAttribute(attribute);
     return oldAttr.release();
 }
 
@@ -144,17 +144,18 @@ PassRefPtr<Node> NamedNodeMap::removeNamedItem(const QualifiedName& name, Except
 {
     ASSERT(m_element);
 
-    Attribute* attribute = getAttributeItem(name);
-    if (!attribute) {
+    size_t index = getAttributeItemIndex(name);
+    if (index == notFound) {
         ec = NOT_FOUND_ERR;
         return 0;
     }
 
+    Attribute* attribute = attributeItem(index);
     RefPtr<Attr> attr = attribute->createAttrIfNeeded(m_element);
 
     m_element->willModifyAttribute(attribute->name(), attribute->value(), nullAtom);
 
-    removeAttribute(name);
+    removeAttribute(index);
     return attr.release();
 }
 
@@ -171,7 +172,7 @@ void NamedNodeMap::copyAttributesToVector(Vector<RefPtr<Attribute> >& copy)
     copy = m_attributes;
 }
 
-Attribute* NamedNodeMap::getAttributeItemSlowCase(const String& name, bool shouldIgnoreAttributeCase) const
+size_t NamedNodeMap::getAttributeItemIndexSlowCase(const String& name, bool shouldIgnoreAttributeCase) const
 {
     unsigned len = length();
 
@@ -180,16 +181,16 @@ Attribute* NamedNodeMap::getAttributeItemSlowCase(const String& name, bool shoul
         const QualifiedName& attrName = m_attributes[i]->name();
         if (!attrName.hasPrefix()) {
             if (shouldIgnoreAttributeCase && equalIgnoringCase(name, attrName.localName()))
-                return m_attributes[i].get();
+                return i;
         } else {
             // FIXME: Would be faster to do this comparison without calling toString, which
             // generates a temporary string by concatenation. But this branch is only reached
             // if the attribute name has a prefix, which is rare in HTML.
             if (equalPossiblyIgnoringCase(name, attrName.toString(), shouldIgnoreAttributeCase))
-                return m_attributes[i].get();
+                return i;
         }
     }
-    return 0;
+    return notFound;
 }
 
 void NamedNodeMap::clearAttributes()
@@ -259,39 +260,43 @@ void NamedNodeMap::addAttribute(PassRefPtr<Attribute> prpAttribute)
     }
 }
 
-void NamedNodeMap::removeAttribute(const QualifiedName& name)
+void NamedNodeMap::removeAttribute(size_t index)
 {
-    unsigned len = length();
-    unsigned index = len;
-    for (unsigned i = 0; i < len; ++i) {
-        if (m_attributes[i]->name().matches(name)) {
-            index = i;
-            break;
-        }
-    }
-
-    if (index >= len)
-        return;
+    ASSERT(index < length());
 
-    // Remove the attribute from the list
-    RefPtr<Attribute> attr = m_attributes[index].get();
-    if (Attr* a = m_attributes[index]->attr())
-        a->m_element = 0;
+    RefPtr<Attribute> attribute = m_attributes[index];
+    if (Attr* attr = attribute->attr())
+        attr->m_element = 0;
 
     m_attributes.remove(index);
 
-    // Notify the element that the attribute has been removed
-    // dispatch appropriate mutation events
-    if (m_element && !attr->m_value.isNull()) {
-        AtomicString value = attr->m_value;
-        attr->m_value = nullAtom;
-        m_element->attributeChanged(attr.get());
-        attr->m_value = value;
+    if (m_element && !attribute->m_value.isNull()) {
+        AtomicString value = attribute->m_value;
+        attribute->m_value = nullAtom;
+        m_element->attributeChanged(attribute.get());
+        attribute->m_value = value;
     }
     if (m_element)
         m_element->dispatchSubtreeModifiedEvent();
 }
 
+void NamedNodeMap::replaceAttribute(size_t index, PassRefPtr<Attribute> attribute)
+{
+    ASSERT(index < length());
+
+    if (Attr* attr = m_attributes[index]->attr())
+        attr->m_element = 0;
+    m_attributes[index] = attribute;
+    if (Attr* attr = m_attributes[index]->attr())
+        attr->m_element = m_element;
+
+    if (m_element) {
+        m_element->attributeChanged(m_attributes[index].get());
+        if (m_attributes[index]->name() != styleAttr)
+            m_element->dispatchSubtreeModifiedEvent();
+    }
+}
+
 void NamedNodeMap::setClass(const String& classStr) 
 { 
     if (!element()->hasClass()) { 
index 34f3860..3534014 100644 (file)
@@ -27,6 +27,7 @@
 
 #include "Attribute.h"
 #include "SpaceSplitString.h"
+#include <wtf/NotFound.h>
 
 namespace WebCore {
 
@@ -65,6 +66,7 @@ public:
 
     Attribute* attributeItem(unsigned index) const { return m_attributes[index].get(); }
     Attribute* getAttributeItem(const QualifiedName&) const;
+    size_t getAttributeItemIndex(const QualifiedName&) const;
 
     void copyAttributesToVector(Vector<RefPtr<Attribute> >&);
 
@@ -89,6 +91,7 @@ public:
     // These functions do no error checking.
     void addAttribute(PassRefPtr<Attribute>);
     void removeAttribute(const QualifiedName&);
+    void removeAttribute(size_t index);
 
     Element* element() const { return m_element; }
 
@@ -110,9 +113,11 @@ private:
     void detachAttributesFromElement();
     void detachFromElement();
     Attribute* getAttributeItem(const String& name, bool shouldIgnoreAttributeCase) const;
-    Attribute* getAttributeItemSlowCase(const String& name, bool shouldIgnoreAttributeCase) const;
+    size_t getAttributeItemIndex(const String& name, bool shouldIgnoreAttributeCase) const;
+    size_t getAttributeItemIndexSlowCase(const String& name, bool shouldIgnoreAttributeCase) const;
     void setAttributes(const NamedNodeMap&);
     void clearAttributes();
+    void replaceAttribute(size_t index, PassRefPtr<Attribute>);
     int declCount() const;
 
     int m_mappedAttributeCount;
@@ -124,34 +129,59 @@ private:
 
 inline Attribute* NamedNodeMap::getAttributeItem(const QualifiedName& name) const
 {
-    unsigned len = length();
+    size_t index = getAttributeItemIndex(name);
+    if (index != notFound)
+        return m_attributes[index].get();
+    return 0;
+}
+
+inline size_t NamedNodeMap::getAttributeItemIndex(const QualifiedName& name) const
+{
+    size_t len = length();
     for (unsigned i = 0; i < len; ++i) {
         if (m_attributes[i]->name().matches(name))
-            return m_attributes[i].get();
+            return i;
     }
+    return notFound;
+}
+
+inline Attribute* NamedNodeMap::getAttributeItem(const String& name, bool shouldIgnoreAttributeCase) const
+{
+    size_t index = getAttributeItemIndex(name, shouldIgnoreAttributeCase);
+    if (index != notFound)
+        return m_attributes[index].get();
     return 0;
 }
 
 // We use a boolean parameter instead of calling shouldIgnoreAttributeCase so that the caller
 // can tune the behavior (hasAttribute is case sensitive whereas getAttribute is not).
-inline Attribute* NamedNodeMap::getAttributeItem(const String& name, bool shouldIgnoreAttributeCase) const
+inline size_t NamedNodeMap::getAttributeItemIndex(const String& name, bool shouldIgnoreAttributeCase) const
 {
     unsigned len = length();
     bool doSlowCheck = shouldIgnoreAttributeCase;
-    
+
     // Optimize for the case where the attribute exists and its name exactly matches.
     for (unsigned i = 0; i < len; ++i) {
         const QualifiedName& attrName = m_attributes[i]->name();
         if (!attrName.hasPrefix()) {
             if (name == attrName.localName())
-                return m_attributes[i].get();
+                return i;
         } else
             doSlowCheck = true;
     }
 
     if (doSlowCheck)
-        return getAttributeItemSlowCase(name, shouldIgnoreAttributeCase);
-    return 0;
+        return getAttributeItemIndexSlowCase(name, shouldIgnoreAttributeCase);
+    return notFound;
+}
+
+inline void NamedNodeMap::removeAttribute(const QualifiedName& name)
+{
+    size_t index = getAttributeItemIndex(name);
+    if (index == notFound)
+        return;
+
+    removeAttribute(index);
 }
 
 } // namespace WebCore
index bfffa3a..54d2163 100644 (file)
@@ -990,9 +990,7 @@ void ApplyStyleCommand::pushDownInlineStyleAroundNode(EditingStyle* style, Node*
             if (!child->contains(targetNode) && elementsToPushDown.size()) {
                 for (size_t i = 0; i < elementsToPushDown.size(); i++) {
                     RefPtr<Element> wrapper = elementsToPushDown[i]->cloneElementWithoutChildren();
-                    ExceptionCode ec = 0;
-                    wrapper->removeAttribute(styleAttr, ec);
-                    ASSERT(!ec);
+                    wrapper->removeAttribute(styleAttr);
                     surroundNodeRangeWithElement(child, child, wrapper);
                 }
             }
index 6484c58..f40c15c 100644 (file)
@@ -137,9 +137,7 @@ PassRefPtr<Element> InsertParagraphSeparatorCommand::cloneHierarchyUnderNewBlock
     for (size_t i = ancestors.size(); i != 0; --i) {
         RefPtr<Element> child = ancestors[i - 1]->cloneElementWithoutChildren();
         // It should always be okay to remove id from the cloned elements, since the originals are not deleted.
-        ExceptionCode ec = 0;
-        child->removeAttribute(idAttr, ec);
-        ASSERT(!ec);
+        child->removeAttribute(idAttr);
         appendNode(child, parent);
         parent = child.release();
     }
index 19bb8bf..ac77bf9 100644 (file)
@@ -61,8 +61,7 @@ void SplitElementCommand::executeApply()
         return;
 
     // Delete id attribute from the second element because the same id cannot be used for more than one element
-    m_element2->removeAttribute(HTMLNames::idAttr, ec);
-    ASSERT(!ec);
+    m_element2->removeAttribute(HTMLNames::idAttr);
 
     size_t size = children.size();
     for (size_t i = 0; i < size; ++i)
index 6573ca6..c19fe94 100644 (file)
@@ -752,7 +752,7 @@ void HTMLElement::setContentEditable(const String& enabled, ExceptionCode& ec)
     else if (equalIgnoringCase(enabled, "plaintext-only"))
         setAttribute(contenteditableAttr, "plaintext-only");
     else if (equalIgnoringCase(enabled, "inherit"))
-        removeAttribute(contenteditableAttr, ec);
+        removeAttribute(contenteditableAttr);
     else
         ec = SYNTAX_ERR;
 }
index 7e03ec6..1596d68 100644 (file)
@@ -532,10 +532,9 @@ void HTMLInputElement::setType(const String& type)
     // We should write a test case to show that setting to the empty string does not remove the
     // attribute in other browsers and then fix this. Note that setting to null *does* remove
     // the attribute and setAttribute implements that.
-    if (type.isEmpty()) {
-        ExceptionCode ec;
-        removeAttribute(typeAttr, ec);
-    } else
+    if (type.isEmpty())
+        removeAttribute(typeAttr);
+    else
         setAttribute(typeAttr, type);
 }
 
index 7a9f26d..546c77d 100644 (file)
@@ -528,9 +528,7 @@ void InspectorDOMAgent::setAttributesAsText(ErrorString* errorString, int elemen
 
     const NamedNodeMap* attrMap = toHTMLElement(child)->attributes(true);
     if (!attrMap && name) {
-        element->removeAttribute(*name, ec);
-        if (ec)
-            *errorString = "Could not remove attribute";
+        element->removeAttribute(*name);
         return;
     }
 
@@ -544,9 +542,7 @@ void InspectorDOMAgent::setAttributesAsText(ErrorString* errorString, int elemen
     }
 
     if (!foundOriginalAttribute && name) {
-        element->removeAttribute(*name, ec);
-        if (ec)
-            *errorString = "Could not remove attribute";
+        element->removeAttribute(*name);
         return;
     }
 }
@@ -554,12 +550,8 @@ void InspectorDOMAgent::setAttributesAsText(ErrorString* errorString, int elemen
 void InspectorDOMAgent::removeAttribute(ErrorString* errorString, int elementId, const String& name)
 {
     Element* element = assertElement(errorString, elementId);
-    if (element) {
-        ExceptionCode ec = 0;
-        element->removeAttribute(name, ec);
-        if (ec)
-            *errorString = "Exception while removing attribute";
-    }
+    if (element)
+        element->removeAttribute(name);
 }
 
 void InspectorDOMAgent::removeNode(ErrorString* errorString, int nodeId)
index 5e67ff5..55f70f4 100644 (file)
@@ -1035,11 +1035,11 @@ void SVGUseElement::transferUseAttributesToReplacedElement(SVGElement* from, SVG
 
     to->setAttributesFromElement(*from);
 
-    to->removeAttribute(SVGNames::xAttr, ASSERT_NO_EXCEPTION);
-    to->removeAttribute(SVGNames::yAttr, ASSERT_NO_EXCEPTION);
-    to->removeAttribute(SVGNames::widthAttr, ASSERT_NO_EXCEPTION);
-    to->removeAttribute(SVGNames::heightAttr, ASSERT_NO_EXCEPTION);
-    to->removeAttribute(XLinkNames::hrefAttr, ASSERT_NO_EXCEPTION);
+    to->removeAttribute(SVGNames::xAttr);
+    to->removeAttribute(SVGNames::yAttr);
+    to->removeAttribute(SVGNames::widthAttr);
+    to->removeAttribute(SVGNames::heightAttr);
+    to->removeAttribute(XLinkNames::hrefAttr);
 }
 
 bool SVGUseElement::selfHasRelativeLengths() const
index 21e8004..07716ef 100644 (file)
@@ -463,8 +463,7 @@ void QWebElement::removeAttribute(const QString &name)
 {
     if (!m_element)
         return;
-    ExceptionCode exception = 0;
-    m_element->removeAttribute(name, exception);
+    m_element->removeAttribute(name);
 }
 
 /*!
@@ -477,8 +476,7 @@ void QWebElement::removeAttributeNS(const QString &namespaceUri, const QString &
 {
     if (!m_element)
         return;
-    WebCore::ExceptionCode exception = 0;
-    m_element->removeAttributeNS(namespaceUri, name, exception);
+    m_element->removeAttributeNS(namespaceUri, name);
 }
 
 /*!
@@ -2134,4 +2132,3 @@ void QtWebElementRuntime::initialize()
     JSC::Bindings::registerCustomType(id, convertJSValueToWebElementVariant, convertWebElementVariantToJSValue);
 #endif
 }
-
index 2928bcd..84b7a23 100644 (file)
@@ -1,3 +1,14 @@
+2011-12-20  Adam Klein  <adamk@chromium.org>
+
+        Avoid unnecessary work when removing attributes from an element
+        https://bugs.webkit.org/show_bug.cgi?id=74953
+
+        Reviewed by Ryosuke Niwa.
+
+        * Api/qwebelement.cpp:
+        (QWebElement::removeAttribute): Updated a caller of
+        WebCore::Element::removeAttribute to remove ExceptionCode argument.
+
 2011-12-20  Rafael Brandao  <rafael.lobo@openbossa.org>
 
         [Qt] Extend QQuickWebview::navigationRequested API