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

Source/core/dom/Element.h

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Peter Kelly (pmk@post.com)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2003-2011, 2013, 2014 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
@@ skipping 121 matching lines @@
     bool fastHasAttribute(const QualifiedName&) const;
     const AtomicString& fastGetAttribute(const QualifiedName&) const;
 #if ENABLE(ASSERT)
     bool fastAttributeLookupAllowed(const QualifiedName&) const;
 #endif
 
 #ifdef DUMP_NODE_STATISTICS
     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;
 
     const AtomicString& getAttribute(const AtomicString& name) const;
     const AtomicString& getAttributeNS(const AtomicString& namespaceURI, const AtomicString& localName) const;
 
     void setAttribute(const AtomicString& name, const AtomicString& value, ExceptionState&);
     static bool parseAttributeName(QualifiedName&, const AtomicString& namespaceURI, const AtomicString& qualifiedName, ExceptionState&);
     void setAttributeNS(const AtomicString& namespaceURI, const AtomicString& qualifiedName, const AtomicString& value, ExceptionState&);
 
     bool isIdAttributeName(const QualifiedName&) const;
     const AtomicString& getIdAttribute() const;
     void setIdAttribute(const AtomicString&);
 
     const AtomicString& getNameAttribute() const;
     const AtomicString& getClassAttribute() const;
 
     bool shouldIgnoreAttributeCase() const;
 
     // Call this to get the value of the id attribute for style resolution purposes.
     // The value will already be lowercased if the document is in compatibility mode,
     // 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);
 
     int offsetLeft();
     int offsetTop();
     int offsetWidth();
     int offsetHeight();
 
     // FIXME: Replace uses of offsetParent in the platform with calls
@@ skipping 500 matching lines @@
 
 inline Element* Node::parentElement() const
 {
     ContainerNode* parent = parentNode();
     return parent && parent->isElementNode() ? toElement(parent) : 0;
 }
 
 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;
 }
 
 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 bool Element::hasAttributesWithoutUpdate() const
+inline AttributeCollection Element::attributesWithoutUpdate() const
 {
+    if (!elementData())
+        return AttributeCollection();
+    return elementData()->attributes();
+}
+
+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.

 }
 
 inline const AtomicString& Element::idForStyleResolution() const
 {
     ASSERT(hasID());
     return elementData()->idForStyleResolution();
 }
 
 inline bool Element::isIdAttributeName(const QualifiedName& attributeName) const
 {
@@ skipping 124 matching lines @@
     static PassRefPtrWillBeRawPtr<T> create(const QualifiedName&, Document&)
 #define DEFINE_ELEMENT_FACTORY_WITH_TAGNAME(T) \
     PassRefPtrWillBeRawPtr<T> T::create(const QualifiedName& tagName, Document& document) \
     { \
         return adoptRefWillBeNoop(new T(tagName, document)); \
     }
 
 } // namespace
 
 #endif