Make Element::attributes() less error-prone and simplify call sites

Source/core/dom/Element.cpp

Index: Source/core/dom/Element.cpp
diff --git a/Source/core/dom/Element.cpp b/Source/core/dom/Element.cpp
index 899045fefa31be39fa3c4a11fc497c45e52c5153..2b27d19d3e17d004cabfc7f561c92ed1c25fc6dc 100644
--- a/Source/core/dom/Element.cpp
+++ b/Source/core/dom/Element.cpp
@@ -431,7 +431,7 @@ const AtomicString& Element::getAttribute(const QualifiedName& name) const
     if (!elementData())
         return nullAtom;
     synchronizeAttribute(name);
-    if (const Attribute* attribute = attributes().find(name))
+    if (const Attribute* attribute = elementData()->attributes().find(name))
         return attribute->value();
     return nullAtom;
 }
@@ -910,7 +910,7 @@ ALWAYS_INLINE void Element::setAttributeInternal(size_t index, const QualifiedNa
         return;
     }
 
-    const Attribute& existingAttribute = attributes().at(index);
+    const Attribute& existingAttribute = elementData()->attributes().at(index);
     QualifiedName existingAttributeName = existingAttribute.name();
 
     if (!inSynchronizationOfLazyAttribute)
@@ -1135,12 +1135,6 @@ void Element::parserSetAttributes(const Vector<Attribute>& attributeVector)
         attributeChangedFromParserOrByCloning(attributeVector[i].name(), attributeVector[i].value(), ModifiedDirectly);
 }
 
-bool Element::hasAttributes() const
-{
-    synchronizeAllAttributes();
-    return elementData() && !elementData()->attributes().isEmpty();
-}
-
 bool Element::hasEquivalentAttributes(const Element* other) const
 {
     synchronizeAllAttributes();
@@ -1192,13 +1186,11 @@ const AtomicString& Element::locateNamespacePrefix(const AtomicString& namespace
     if (!prefix().isNull() && namespaceURI() == namespaceToLocate)
         return prefix();
 
-    if (hasAttributes()) {
-        AttributeCollection attributes = this->attributes();
-        AttributeCollection::const_iterator end = attributes.end();
-        for (AttributeCollection::const_iterator it = attributes.begin(); it != end; ++it) {
-            if (it->prefix() == xmlnsAtom && it->value() == namespaceToLocate)
-                return it->localName();
-        }
+    AttributeCollection attributes = this->attributes();
+    AttributeCollection::const_iterator end = attributes.end();
+    for (AttributeCollection::const_iterator it = attributes.begin(); it != end; ++it) {
+        if (it->prefix() == xmlnsAtom && it->value() == namespaceToLocate)
+            return it->localName();
     }
 
     if (Element* parent = parentElement())
@@ -2493,10 +2485,9 @@ void Element::cancelFocusAppearanceUpdate()
 
 void Element::normalizeAttributes()
 {
-    if (!hasAttributes())
-        return;
+    synchronizeAllAttributes();
     WillBeHeapVector<RefPtrWillBeMember<Attr> >* attrNodes = attrNodeList();
-    if (!attrNodes)
+    if (!attrNodes || attrNodes->isEmpty())

[adamk, 2014/08/04 18:54:34]

Why did this change?

[Inactive, 2014/08/04 19:09:46]

I wanted to avoid the ElementData() null check in hasAttributes(), but it could
stay as is without any perf regression (I did not change hasAttributes()).

I'll revert this part of the change to reduce the diff.

[Inactive, 2014/08/04 19:39:52]

Done.

         return;
     // Copy the Attr Vector because Node::normalize() can fire synchronous JS
     // events (e.g. DOMSubtreeModified) and a JS listener could add / remove
@@ -2969,7 +2960,7 @@ void Element::detachAllAttrNodesFromElement()
     AttrNodeList* list = this->attrNodeList();
     ASSERT(list);
 
-    AttributeCollection attributes = this->attributes();
+    AttributeCollection attributes = elementData()->attributes();

[adamk, 2014/08/04 18:54:34]

I've seen enough of these that it worries me someone will get it wrong in the
future. Perhaps even Element.cpp should use attributesWithoutUpdate() instead of
elementData()->attributes() to make it clearer what the callers are trying to
do? Or do you think the branch there is too expensive?

[Inactive, 2014/08/04 19:09:46]

I did not want do cause more work than before my patch to avoid any chance of
perf regression. If we call attributesWithoutUpdate() here, then we do an extra
null-check on ElementData(). I haven't measured if it matters, I just did not
want to chance it.

[adamk, 2014/08/04 19:29:51]

The more I think about this the more it's a reasonable compromise to maintain
existing performance...

[Inactive, 2014/08/04 19:39:52]

Ok, I kept it as is.

     AttributeCollection::const_iterator end = attributes.end();
     for (AttributeCollection::const_iterator it = attributes.begin(); it != end; ++it) {
         if (RefPtrWillBeRawPtr<Attr> attrNode = findAttrNodeInList(*list, it->name()))