Add iterator object to iterate efficiently over an Element's attributes

Source/core/dom/ElementData.h

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
+ * Copyright (C) 2014 Apple Inc. All rights reserved.

[esprehn, 2014/05/28 20:03:54]

Do we really need to add Apple copyrights?

[Inactive, 2014/05/29 15:14:01]

I don't know what the exact rules are but it seems (to me) like the right thing
to do since I am porting over once of their patches.

FYI, I have done this so far when porting patches over to Blink.

[Inactive, 2014/05/29 15:17:59]

Also, this CL seems to hint that I am not the only one doing this:
https://codereview.chromium.org/305653007/

  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
  * copyright notice, this list of conditions and the following disclaimer
  * in the documentation and/or other materials provided with the
@@ skipping 22 matching lines @@
 #include "core/dom/SpaceSplitString.h"
 #include "wtf/text/AtomicString.h"
 
 namespace WebCore {
 
 class Attr;
 class ShareableElementData;
 class StylePropertySet;
 class UniqueElementData;
 
+class AttributeConstIterator {
+public:
+    AttributeConstIterator(const Attribute* array, unsigned index)
+        : m_array(array)
+        , m_index(index)
+    { }
+
+    const Attribute* operator*() const { return &m_array[m_index]; }
+    const Attribute* operator->() const { return &m_array[m_index]; }
+    AttributeConstIterator& operator++() { ++m_index; return *this; }
+
+    bool operator==(const AttributeConstIterator& other) const { return m_index == other.m_index; }
+    bool operator!=(const AttributeConstIterator& other) const { return !(*this == other); }
+
+    unsigned index() const { return m_index; }
+
+private:
+    const Attribute* m_array;
+    unsigned m_index;
+};
+
+class AttributeIteratorAccessor {

[esprehn, 2014/05/28 20:03:54]

This class seems unnecessary.

[Inactive, 2014/05/29 15:14:01]

The main reason for this class (besides avoiding multiple calls to isUnique() as
explained below), is that this will let us use range iterators when we
eventually start using C++11 (http://www.stroustrup.com/C++11FAQ.html#for):
for (const Attribute& attribute : attributesIterator())

For the range-for statement to work, the object needs to have begin() and end()
methods defines (i.e. not attributesBegin() / attributesEnd()). Defining those
on ElementData may be a bit confusing. Defining those on Element would
definitely be confusing as it would not be obvious the iterators are for the
Element's attributes.

+public:
+    AttributeIteratorAccessor(const Attribute* array, unsigned size)
+        : m_array(array)
+        , m_size(size)
+    { }
+
+    AttributeConstIterator begin() const { return AttributeConstIterator(m_array, 0); }
+    AttributeConstIterator end() const { return AttributeConstIterator(m_array, m_size); }
+
+    unsigned size() const { return m_size; }
+
+private:
+    const Attribute* m_array;
+    unsigned m_size;
+};
+
 // ElementData represents very common, but not necessarily unique to an element,
 // data such as attributes, inline style, and parsed class names and ids.
 class ElementData : public RefCounted<ElementData> {
     WTF_MAKE_FAST_ALLOCATED;
 public:
     // Override RefCounted's deref() to ensure operator delete is called on
     // the appropriate subclass type.
     void deref();
 
     void clearClass() const { m_classNames.clear(); }
     void setClass(const AtomicString& className, bool shouldFoldCase) const { m_classNames.set(className, shouldFoldCase); }
     const SpaceSplitString& classNames() const { return m_classNames; }
 
     const AtomicString& idForStyleResolution() const { return m_idForStyleResolution; }
     void setIdForStyleResolution(const AtomicString& newId) const { m_idForStyleResolution = newId; }
 
     const StylePropertySet* inlineStyle() const { return m_inlineStyle.get(); }
 
     const StylePropertySet* presentationAttributeStyle() const;
 
     // This is not a trivial getter and its return value should be cached for performance.
     size_t length() const;
     bool isEmpty() const { return !length(); }
 
+    AttributeIteratorAccessor attributesIterator() const;

[esprehn, 2014/05/28 20:03:54]

Can we just add attributesBegin() and attributesEnd() instead? Why do we need
this class?

[Inactive, 2014/05/29 15:14:01]

If we do this then we will call ElementData::isUnique() twice, once in
attributesBegin() and once in attributesEnd(), right? Instead of now calling it
only once when calling attributesData().

[Inactive, 2014/05/30 13:16:03]

Note that we had issues before with callers not caching attributeCount() before
the loop and thus calling ElementData::isUnique() unnecessarily for every loop
iteration. The proposal at the time was to introduce iterators to avoid this.
However, if attributesEnd() hides a call to ElementData::isUnique(), then the
same type of error is also possible if the caller fails to cache the end
iterator before the loop.

+
     const Attribute& attributeItem(unsigned index) const;
     const Attribute* getAttributeItem(const QualifiedName&) const;
     size_t getAttributeItemIndex(const QualifiedName&, bool shouldIgnoreCase = false) const;
     size_t getAttributeItemIndex(const AtomicString& name, bool shouldIgnoreAttributeCase) const;
     size_t getAttrIndex(Attr*) const;
 
     bool hasID() const { return !m_idForStyleResolution.isNull(); }
     bool hasClass() const { return !m_classNames.isNull(); }
 
     bool isEquivalent(const ElementData* other) const;
@@ skipping 116 matching lines @@
 
 inline const Attribute* ElementData::attributeBase() const
 {
     if (m_isUnique)
         return static_cast<const UniqueElementData*>(this)->m_attributeVector.begin();
     return static_cast<const ShareableElementData*>(this)->m_attributeArray;
 }
 
 inline size_t ElementData::getAttributeItemIndex(const QualifiedName& name, bool shouldIgnoreCase) const
 {
-    const Attribute* begin = attributeBase();
-    // Cache length for performance as ElementData::length() contains a conditional branch.
-    unsigned length = this->length();
-    for (unsigned i = 0; i < length; ++i) {
-        const Attribute& attribute = begin[i];
-        if (attribute.name().matchesPossiblyIgnoringCase(name, shouldIgnoreCase))
-            return i;
+    AttributeIteratorAccessor attributes = attributesIterator();
+    AttributeConstIterator end = attributes.end();
+    for (AttributeConstIterator it = attributes.begin(); it != end; ++it) {
+        if (it->name().matchesPossiblyIgnoringCase(name, shouldIgnoreCase))
+            return it.index();
     }
     return kNotFound;
 }
 
 // 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 size_t ElementData::getAttributeItemIndex(const AtomicString& name, bool shouldIgnoreAttributeCase) const
 {
-    // Cache length for performance as ElementData::length() contains a conditional branch.
-    unsigned length = this->length();
     bool doSlowCheck = shouldIgnoreAttributeCase;
 
     // Optimize for the case where the attribute exists and its name exactly matches.
-    const Attribute* begin = attributeBase();
-    for (unsigned i = 0; i < length; ++i) {
-        const Attribute& attribute = begin[i];
+    AttributeIteratorAccessor attributes = attributesIterator();
+    AttributeConstIterator end = attributes.end();
+    for (AttributeConstIterator it = attributes.begin(); it != end; ++it) {
         // FIXME: Why check the prefix? Namespaces should be all that matter.
         // Most attributes (all of HTML and CSS) have no namespace.
-        if (!attribute.name().hasPrefix()) {
-            if (name == attribute.localName())
-                return i;
+        if (!it->name().hasPrefix()) {
+            if (name == it->localName())
+                return it.index();
         } else {
             doSlowCheck = true;
         }
     }
 
     if (doSlowCheck)
         return getAttributeItemIndexSlowCase(name, shouldIgnoreAttributeCase);
     return kNotFound;
 }
 
+inline AttributeIteratorAccessor ElementData::attributesIterator() const
+{
+    if (isUnique()) {
+        const Vector<Attribute, 4>& attributeVector = static_cast<const UniqueElementData*>(this)->m_attributeVector;
+        return AttributeIteratorAccessor(attributeVector.data(), attributeVector.size());
+    }
+    return AttributeIteratorAccessor(static_cast<const ShareableElementData*>(this)->m_attributeArray, m_arraySize);
+}
+
 inline const Attribute* ElementData::getAttributeItem(const QualifiedName& name) const
 {
-    const Attribute* begin = attributeBase();
-    unsigned length = this->length();
-    for (unsigned i = 0; i < length; ++i) {
-        const Attribute& attribute = begin[i];
-        if (attribute.name().matches(name))
-            return &attribute;
+    AttributeIteratorAccessor attributes = attributesIterator();
+    AttributeConstIterator end = attributes.end();
+    for (AttributeConstIterator it = attributes.begin(); it != end; ++it) {
+        if (it->name().matches(name))
+            return *it;
     }
     return 0;
 }
 
 inline const Attribute& ElementData::attributeItem(unsigned index) const
 {
     RELEASE_ASSERT(index < length());
     ASSERT(attributeBase() + index);
     return *(attributeBase() + index);
 }
 
 inline void UniqueElementData::addAttribute(const QualifiedName& attributeName, const AtomicString& value)
 {
     m_attributeVector.append(Attribute(attributeName, value));
 }
 
 inline void UniqueElementData::removeAttribute(size_t index)
 {
     m_attributeVector.remove(index);
 }
 
 inline Attribute& UniqueElementData::attributeItem(unsigned index)
 {
     return m_attributeVector.at(index);
 }
 
 } // namespace WebCore
 
 #endif // ElementData_h