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

Source/core/dom/Element.h

Index: Source/core/dom/Element.h
diff --git a/Source/core/dom/Element.h b/Source/core/dom/Element.h
index 241698f723912cca3b6e56c9c4d27b3d77bb786b..1aff94f3a1222c4afbb4463d9be354d898a8c149 100644
--- a/Source/core/dom/Element.h
+++ b/Source/core/dom/Element.h
@@ -139,9 +139,6 @@ public:
     bool hasNamedNodeMap() const;
 #endif
     bool hasAttributes() const;
-    // This variant will not update the potentially invalid attributes. To be used when not interested
-    // in style attribute or one of the SVG animation attributes.
-    bool hasAttributesWithoutUpdate() const;
 
     bool hasAttribute(const AtomicString& name) const;
     bool hasAttributeNS(const AtomicString& namespaceURI, const AtomicString& localName) const;
@@ -167,10 +164,14 @@ public:
     // so this function is not suitable for non-style uses.
     const AtomicString& idForStyleResolution() const;
 
-    // Internal method that assumes the existence of attribute storage, one should use hasAttributes()
-    // before calling it. This is not a trivial getter and its return value should be cached for
-    // performance.
+    // This getter takes care of synchronizing all attributes before returning the
+    // AttributeCollection. If the Element has no attributes, an empty AttributeCollection
+    // will be returned. This is not a trivial getter and its return value should be cached
+    // for performance.
     AttributeCollection attributes() const;
+    // This variant will not update the potentially invalid attributes. To be used when not interested
+    // in style attribute or one of the SVG animation attributes.
+    AttributeCollection attributesWithoutUpdate() const;
 
     void scrollIntoView(bool alignToTop = true);
     void scrollIntoViewIfNeeded(bool centerIfNeeded = true);
@@ -691,14 +692,14 @@ inline Element* Node::parentElement() const
 inline bool Element::fastHasAttribute(const QualifiedName& name) const
 {
     ASSERT(fastAttributeLookupAllowed(name));
-    return elementData() && attributes().findIndex(name) != kNotFound;
+    return elementData() && elementData()->attributes().findIndex(name) != kNotFound;

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

I could call "return attributesWithoutUpdate().findIndex(name) != kNotFound;"

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

For these cases I definitely see the advantage of avoiding unnecessary logic,
I'd suggest leaving them as-is.

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

Ok, I kept them as is.

 }
 
 inline const AtomicString& Element::fastGetAttribute(const QualifiedName& name) const
 {
     ASSERT(fastAttributeLookupAllowed(name));
     if (elementData()) {
-        if (const Attribute* attribute = attributes().find(name))
+        if (const Attribute* attribute = elementData()->attributes().find(name))

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

I could call "return attributesWithoutUpdate().findIndex(name) != kNotFound;"

             return attribute->value();
     }
     return nullAtom;
@@ -706,12 +707,22 @@ inline const AtomicString& Element::fastGetAttribute(const QualifiedName& name)
 
 inline AttributeCollection Element::attributes() const
 {
-    ASSERT(elementData());
+    if (!elementData())

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

Is it really safe to check this before calling synchronize? I guess it's safe
for style, because that's stored in element data, but what about SVG stuff?
Couldn't this break if that state was elsewhere?

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

synchronizeAllAttributes() returns early if elementData() is null so it is safe.
I put the calls in the order to avoid incurring the call of
synchronizeAllAttributes() as it is not inline.

+        return AttributeCollection();
+    synchronizeAllAttributes();
+    return elementData()->attributes();
+}
+
+inline AttributeCollection Element::attributesWithoutUpdate() const
+{
+    if (!elementData())
+        return AttributeCollection();
     return elementData()->attributes();
 }
 
-inline bool Element::hasAttributesWithoutUpdate() const
+inline bool Element::hasAttributes() const
 {
+    synchronizeAllAttributes();
     return elementData() && !elementData()->attributes().isEmpty();

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

Can't this just be:

return !attributes().isEmpty();

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

Good point, yes. My original version of this patch didn't synchronize in
hasAttributes() so I couldn't do this. However, I noticed after this could cause
trouble for the style attribute so I put the synchronizeAllAttributes() call
back. I'll refactor as suggested.

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

Done.

 }