NodeLists should not invalidate on irreleavnt attribute changes

https://bugs.webkit.org/show_bug.cgi?id=91277

Reviewed by Ojan Vafai.

Explicitely check the invalidation type and the changed attribute in NodeListNodeData::invalidateCaches
and ElementRareData::clearHTMLCollectionCaches to only invalidate node lists affected by the change.

Also merged invalidateNodeListsCacheAfterAttributeChanged and invalidateNodeListsCacheAfterChildrenChanged
as invalidateNodeListCachesInAncestors since they're almost identical after r122498.

In addition, moved shouldInvalidateNodeListForType from Document.cpp to DynamicNodeList.h and renamed it to
shouldInvalidateTypeOnAttributeChange since it needs to called in Node.cpp and ElementRareData.h.

* dom/Attr.cpp:
(WebCore::Attr::setValue):
(WebCore::Attr::childrenChanged):
* dom/ContainerNode.cpp:
(WebCore::ContainerNode::childrenChanged):
* dom/Document.cpp:
(WebCore::Document::registerNodeListCache): Calls isRootedAtDocument() instead of directly comparing
the value of NodeListRootType in order to prepare for the bug 80269.
(WebCore::Document::unregisterNodeListCache): Ditto.
(WebCore): shouldInvalidateNodeListForType is moved to DynamicNodeList.h
(WebCore::Document::shouldInvalidateNodeListCaches):
* dom/DynamicNodeList.h:
(DynamicNodeListCacheBase):
(WebCore::DynamicNodeListCacheBase::shouldInvalidateTypeOnAttributeChange): Moved from Document.cpp.
* dom/Element.cpp:
(WebCore::Element::attributeChanged):
* dom/ElementRareData.h:
(WebCore::ElementRareData::clearHTMLCollectionCaches): Takes const QualifiedName* to compare against
the invalidation type of HTML collections via shouldInvalidateTypeOnAttributeChange.
* dom/Node.cpp:
(WebCore::Node::invalidateNodeListCachesInAncestors): Merged invalidateNodeListCachesInAncestors and
invalidateNodeListsCacheAfterChildrenChanged. Also pass attrName to clearHTMLCollectionCaches.
(WebCore::NodeListsNodeData::invalidateCaches): Compares attrName against the invalidation type of
node lists via shouldInvalidateTypeOnAttributeChange.
(WebCore):
* dom/Node.h:
(Node):
* dom/NodeRareData.h:
(WebCore::NodeRareData::ensureNodeLists): Merged NodeRareData::createNodeLists.
(WebCore::NodeRareData::clearChildNodeListCache): Moved from Node.cpp.
(NodeRareData):
* html/HTMLCollection.h:
(HTMLCollectionCacheBase):

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

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index a2071a2..90490ba 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,53 @@
+2012-07-13  Ryosuke Niwa  <rniwa@webkit.org>
+
+        NodeLists should not invalidate on irreleavnt attribute changes
+        https://bugs.webkit.org/show_bug.cgi?id=91277
+
+        Reviewed by Ojan Vafai.
+
+        Explicitely check the invalidation type and the changed attribute in NodeListNodeData::invalidateCaches
+        and ElementRareData::clearHTMLCollectionCaches to only invalidate node lists affected by the change.
+
+        Also merged invalidateNodeListsCacheAfterAttributeChanged and invalidateNodeListsCacheAfterChildrenChanged
+        as invalidateNodeListCachesInAncestors since they're almost identical after r122498.
+
+        In addition, moved shouldInvalidateNodeListForType from Document.cpp to DynamicNodeList.h and renamed it to
+        shouldInvalidateTypeOnAttributeChange since it needs to called in Node.cpp and ElementRareData.h.
+
+        * dom/Attr.cpp:
+        (WebCore::Attr::setValue):
+        (WebCore::Attr::childrenChanged):
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::childrenChanged):
+        * dom/Document.cpp:
+        (WebCore::Document::registerNodeListCache): Calls isRootedAtDocument() instead of directly comparing
+        the value of NodeListRootType in order to prepare for the bug 80269.
+        (WebCore::Document::unregisterNodeListCache): Ditto.
+        (WebCore): shouldInvalidateNodeListForType is moved to DynamicNodeList.h
+        (WebCore::Document::shouldInvalidateNodeListCaches):
+        * dom/DynamicNodeList.h:
+        (DynamicNodeListCacheBase):
+        (WebCore::DynamicNodeListCacheBase::shouldInvalidateTypeOnAttributeChange): Moved from Document.cpp.
+        * dom/Element.cpp: 
+        (WebCore::Element::attributeChanged):
+        * dom/ElementRareData.h:
+        (WebCore::ElementRareData::clearHTMLCollectionCaches): Takes const QualifiedName* to compare against
+        the invalidation type of HTML collections via shouldInvalidateTypeOnAttributeChange.
+        * dom/Node.cpp:
+        (WebCore::Node::invalidateNodeListCachesInAncestors): Merged invalidateNodeListCachesInAncestors and
+        invalidateNodeListsCacheAfterChildrenChanged. Also pass attrName to clearHTMLCollectionCaches.
+        (WebCore::NodeListsNodeData::invalidateCaches): Compares attrName against the invalidation type of
+        node lists via shouldInvalidateTypeOnAttributeChange.
+        (WebCore):
+        * dom/Node.h:
+        (Node):
+        * dom/NodeRareData.h:
+        (WebCore::NodeRareData::ensureNodeLists): Merged NodeRareData::createNodeLists.
+        (WebCore::NodeRareData::clearChildNodeListCache): Moved from Node.cpp.
+        (NodeRareData):
+        * html/HTMLCollection.h:
+        (HTMLCollectionCacheBase):
+
 2012-07-13  Arpita Bahuguna  <arpitabahuguna@gmail.com>
 
         Refactor RenderTable to use the section's iteration functions.

diff --git a/Source/WebCore/dom/Attr.cpp b/Source/WebCore/dom/Attr.cpp
index de9e4ff..95c3fe0 100644 (file)
--- a/Source/WebCore/dom/Attr.cpp
+++ b/Source/WebCore/dom/Attr.cpp
@@ -119,7 +119,7 @@ void Attr::setValue(const AtomicString& value)
     createTextChild();
     m_ignoreChildrenChanged--;
 
-    invalidateNodeListsCacheAfterAttributeChanged(m_name, m_element);
+    invalidateNodeListCachesInAncestors(&m_name, m_element);
 }
 
 void Attr::setValue(const AtomicString& value, ExceptionCode&)
@@ -162,7 +162,7 @@ void Attr::childrenChanged(bool, Node*, Node*, int)
     if (m_ignoreChildrenChanged > 0)
         return;
 
-    invalidateNodeListsCacheAfterAttributeChanged(qualifiedName(), m_element);
+    invalidateNodeListCachesInAncestors(&qualifiedName(), m_element);
 
     // FIXME: We should include entity references in the value
 

diff --git a/Source/WebCore/dom/ContainerNode.cpp b/Source/WebCore/dom/ContainerNode.cpp
index c23a0b9..fd53a2f 100644 (file)
--- a/Source/WebCore/dom/ContainerNode.cpp
+++ b/Source/WebCore/dom/ContainerNode.cpp
@@ -680,7 +680,7 @@ void ContainerNode::childrenChanged(bool changedByParser, Node*, Node*, int chil
     document()->incDOMTreeVersion();
     if (!changedByParser && childCountDelta)
         document()->updateRangesAfterChildrenChanged(this);
-    invalidateNodeListsCacheAfterChildrenChanged();
+    invalidateNodeListCachesInAncestors();
 }
 
 void ContainerNode::cloneChildNodes(ContainerNode *clone)

diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
index b4fa8a2..64167fc 100644 (file)
--- a/Source/WebCore/dom/Document.cpp
+++ b/Source/WebCore/dom/Document.cpp
@@ -3871,7 +3871,7 @@ void Document::registerNodeListCache(DynamicNodeListCacheBase* list)
     if (list->type() != InvalidCollectionType)
         m_nodeListCounts[InvalidateOnIdNameAttrChange]++;
     m_nodeListCounts[list->invalidationType()]++;
-    if (list->rootType() == NodeListIsRootedAtDocument)
+    if (list->isRootedAtDocument())
         m_listsInvalidatedAtDocument.add(list);
 }
 
@@ -3880,45 +3880,17 @@ void Document::unregisterNodeListCache(DynamicNodeListCacheBase* list)
     if (list->type() != InvalidCollectionType)
         m_nodeListCounts[InvalidateOnIdNameAttrChange]--;
     m_nodeListCounts[list->invalidationType()]--;
-    if (list->rootType() == NodeListIsRootedAtDocument) {
+    if (list->isRootedAtDocument()) {
         ASSERT(m_listsInvalidatedAtDocument.contains(list));
         m_listsInvalidatedAtDocument.remove(list);
     }
 }
 
-static ALWAYS_INLINE bool shouldInvalidateNodeListForType(NodeListInvalidationType type, const QualifiedName& attrName)
-{
-    switch (type) {
-    case InvalidateOnClassAttrChange:
-        return attrName == classAttr;
-    case InvalidateOnNameAttrChange:
-        return attrName == nameAttr;
-    case InvalidateOnIdNameAttrChange:
-        return attrName == idAttr || attrName == nameAttr;
-    case InvalidateOnForAttrChange:
-        return attrName == forAttr;
-    case InvalidateForFormControls:
-        return attrName == nameAttr || attrName == idAttr || attrName == forAttr || attrName == typeAttr;
-    case InvalidateOnHRefAttrChange:
-        return attrName == hrefAttr;
-    case InvalidateOnItemAttrChange:
-#if ENABLE(MICRODATA)
-        return attrName == itemscopeAttr || attrName == itempropAttr || attrName == itemtypeAttr;
-#endif // Intentionally fall through
-    case DoNotInvalidateOnAttributeChanges:
-        ASSERT_NOT_REACHED();
-        return false;
-    case InvalidateOnAnyAttrChange:
-        return true;
-    }
-    return false;
-}
-
 bool Document::shouldInvalidateNodeListCaches(const QualifiedName* attrName) const
 {
     if (attrName) {
         for (int type = DoNotInvalidateOnAttributeChanges + 1; type < numNodeListInvalidationTypes; type++) {
-            if (m_nodeListCounts[type] && shouldInvalidateNodeListForType(static_cast<NodeListInvalidationType>(type), *attrName))
+            if (m_nodeListCounts[type] && DynamicNodeListCacheBase::shouldInvalidateTypeOnAttributeChange(static_cast<NodeListInvalidationType>(type), *attrName))
                 return true;
         }
         return false;

diff --git a/Source/WebCore/dom/DynamicNodeList.h b/Source/WebCore/dom/DynamicNodeList.h
index b2fbe20..3343071 100644 (file)
--- a/Source/WebCore/dom/DynamicNodeList.h
+++ b/Source/WebCore/dom/DynamicNodeList.h
@@ -26,6 +26,7 @@
 
 #include "CollectionType.h"
 #include "Document.h"
+#include "HTMLNames.h"
 #include "NodeList.h"
 #include <wtf/Forward.h>
 #include <wtf/RefPtr.h>
@@ -57,13 +58,13 @@ public:
 
 public:
     ALWAYS_INLINE bool isRootedAtDocument() const { return m_rootedAtDocument; }
-    ALWAYS_INLINE bool shouldInvalidateOnAttributeChange() const { return m_invalidationType != DoNotInvalidateOnAttributeChanges; }
-    ALWAYS_INLINE NodeListRootType rootType() { return m_rootedAtDocument ? NodeListIsRootedAtDocument : NodeListIsRootedAtNode; }
     ALWAYS_INLINE NodeListInvalidationType invalidationType() const { return static_cast<NodeListInvalidationType>(m_invalidationType); }
     ALWAYS_INLINE CollectionType type() const { return static_cast<CollectionType>(m_collectionType); }
 
     void invalidateCache() const;
 
+    static bool shouldInvalidateTypeOnAttributeChange(NodeListInvalidationType, const QualifiedName&);
+
 protected:
     ALWAYS_INLINE bool isItemCacheValid() const { return m_isItemCacheValid; }
     ALWAYS_INLINE Node* cachedItem() const { return m_cachedItem; }
@@ -101,6 +102,34 @@ private:
     const unsigned m_collectionType : 5;
 };
 
+ALWAYS_INLINE bool DynamicNodeListCacheBase::shouldInvalidateTypeOnAttributeChange(NodeListInvalidationType type, const QualifiedName& attrName)
+{
+    switch (type) {
+    case InvalidateOnClassAttrChange:
+        return attrName == HTMLNames::classAttr;
+    case InvalidateOnNameAttrChange:
+        return attrName == HTMLNames::nameAttr;
+    case InvalidateOnIdNameAttrChange:
+        return attrName == HTMLNames::idAttr || attrName == HTMLNames::nameAttr;
+    case InvalidateOnForAttrChange:
+        return attrName == HTMLNames::forAttr;
+    case InvalidateForFormControls:
+        return attrName == HTMLNames::nameAttr || attrName == HTMLNames::idAttr || attrName == HTMLNames::forAttr || attrName == HTMLNames::typeAttr;
+    case InvalidateOnHRefAttrChange:
+        return attrName == HTMLNames::hrefAttr;
+    case InvalidateOnItemAttrChange:
+#if ENABLE(MICRODATA)
+        return attrName == HTMLNames::itemscopeAttr || attrName == HTMLNames::itempropAttr || attrName == HTMLNames::itemtypeAttr;
+#endif // Intentionally fall through
+    case DoNotInvalidateOnAttributeChanges:
+        ASSERT_NOT_REACHED();
+        return false;
+    case InvalidateOnAnyAttrChange:
+        return true;
+    }
+    return false;
+}
+
 class DynamicNodeList : public NodeList, public DynamicNodeListCacheBase {
 public:
     enum NodeListType {

diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
index 869c344..eb1733b 100644 (file)
--- a/Source/WebCore/dom/Element.cpp
+++ b/Source/WebCore/dom/Element.cpp
@@ -705,7 +705,7 @@ void Element::attributeChanged(const Attribute& attribute)
             setNeedsStyleRecalc();
     }
 
-    invalidateNodeListsCacheAfterAttributeChanged(attribute.name(), this);
+    invalidateNodeListCachesInAncestors(&attribute.name(), this);
 
     if (!AXObjectCache::accessibilityEnabled())
         return;

diff --git a/Source/WebCore/dom/ElementRareData.h b/Source/WebCore/dom/ElementRareData.h
index b17654f..07394eb 100644 (file)
--- a/Source/WebCore/dom/ElementRareData.h
+++ b/Source/WebCore/dom/ElementRareData.h
@@ -66,14 +66,18 @@ public:
         (*m_cachedCollections)[type - FirstNodeCollectionType] = 0;
     }
 
-    void clearHTMLCollectionCaches()
+    void clearHTMLCollectionCaches(const QualifiedName* attrName)
     {
         if (!m_cachedCollections)
             return;
 
+        bool shouldIgnoreType = !attrName || *attrName == HTMLNames::idAttr || *attrName == HTMLNames::nameAttr;
+
         for (unsigned i = 0; i < (*m_cachedCollections).size(); i++) {
-            if ((*m_cachedCollections)[i])
-                (*m_cachedCollections)[i]->invalidateCache();
+            if (HTMLCollection* collection = (*m_cachedCollections)[i]) {
+                if (shouldIgnoreType || DynamicNodeListCacheBase::shouldInvalidateTypeOnAttributeChange(collection->invalidationType(), *attrName))
+                    collection->invalidateCache();
+            }
         }
     }
 

diff --git a/Source/WebCore/dom/Node.cpp b/Source/WebCore/dom/Node.cpp
index 90c5fb3..7d2aa6f 100644 (file)
--- a/Source/WebCore/dom/Node.cpp
+++ b/Source/WebCore/dom/Node.cpp
@@ -962,40 +962,15 @@ unsigned Node::nodeIndex() const
     return count;
 }
 
-void Node::invalidateNodeListsCacheAfterAttributeChanged(const QualifiedName& attrName, Element* attributeOwnerElement)
+void Node::invalidateNodeListCachesInAncestors(const QualifiedName* attrName, Element* attributeOwnerElement)
 {
-    if (hasRareData() && isAttributeNode()) {
-        NodeRareData* data = rareData();
-        ASSERT(!data->nodeLists());
-        data->clearChildNodeListCache();
-    }
+    if (hasRareData() && (!attrName || isAttributeNode()))
+        rareData()->clearChildNodeListCache();
 
     // Modifications to attributes that are not associated with an Element can't invalidate NodeList caches.
-    if (!attributeOwnerElement)
-        return;
-
-    if (!document()->shouldInvalidateNodeListCaches(&attrName))
+    if (attrName && !attributeOwnerElement)
         return;
 
-    document()->clearNodeListCaches();
-
-    for (Node* node = this; node; node = node->parentNode()) {
-        ASSERT(this == node || !node->isAttributeNode());
-        if (!node->hasRareData())
-            continue;
-        NodeRareData* data = node->rareData();
-        if (data->nodeLists())
-            data->nodeLists()->invalidateCaches(&attrName);
-        if (node->isElementNode())
-            static_cast<ElementRareData*>(data)->clearHTMLCollectionCaches();
-    }
-}
-
-void Node::invalidateNodeListsCacheAfterChildrenChanged()
-{
-    if (hasRareData())
-        rareData()->clearChildNodeListCache();
-
     if (!document()->shouldInvalidateNodeListCaches())
         return;
 
@@ -1006,9 +981,9 @@ void Node::invalidateNodeListsCacheAfterChildrenChanged()
             continue;
         NodeRareData* data = node->rareData();
         if (data->nodeLists())
-            data->nodeLists()->invalidateCaches();
+            data->nodeLists()->invalidateCaches(attrName);
         if (node->isElementNode())
-            static_cast<ElementRareData*>(data)->clearHTMLCollectionCaches();
+            static_cast<ElementRareData*>(data)->clearHTMLCollectionCaches(attrName);
     }
 }
 
@@ -2249,14 +2224,16 @@ void NodeListsNodeData::invalidateCaches(const QualifiedName* attrName)
 {
     NodeListAtomicNameCacheMap::const_iterator atomicNameCacheEnd = m_atomicNameCaches.end();
     for (NodeListAtomicNameCacheMap::const_iterator it = m_atomicNameCaches.begin(); it != atomicNameCacheEnd; ++it) {
-        if (!attrName || it->second->shouldInvalidateOnAttributeChange())
-            it->second->invalidateCache();
+        DynamicNodeList* list = it->second;
+        if (!attrName || DynamicNodeListCacheBase::shouldInvalidateTypeOnAttributeChange(list->invalidationType(), *attrName))
+            list->invalidateCache();
     }
 
     NodeListNameCacheMap::const_iterator nameCacheEnd = m_nameCaches.end();
     for (NodeListNameCacheMap::const_iterator it = m_nameCaches.begin(); it != nameCacheEnd; ++it) {
-        if (!attrName || it->second->shouldInvalidateOnAttributeChange())
-            it->second->invalidateCache();
+        DynamicNodeList* list = it->second;
+        if (!attrName || DynamicNodeListCacheBase::shouldInvalidateTypeOnAttributeChange(list->invalidationType(), *attrName))
+            list->invalidateCache();
     }
 
     if (!attrName)
@@ -2772,17 +2749,6 @@ void Node::setItemType(const String& value)
 
 #endif
 
-void NodeRareData::createNodeLists()
-{
-    setNodeLists(NodeListsNodeData::create());
-}
-
-void NodeRareData::clearChildNodeListCache()
-{
-    if (m_childNodeList)
-        m_childNodeList->invalidateCache();
-}
-
 // It's important not to inline removedLastRef, because we don't want to inline the code to
 // delete a Node at each deref call site.
 void Node::removedLastRef()

diff --git a/Source/WebCore/dom/Node.h b/Source/WebCore/dom/Node.h
index 93a66d9..c9a28f4 100644 (file)
--- a/Source/WebCore/dom/Node.h
+++ b/Source/WebCore/dom/Node.h
@@ -558,8 +558,7 @@ public:
     void showTreeForThisAcrossFrame() const;
 #endif
 
-    void invalidateNodeListsCacheAfterAttributeChanged(const QualifiedName&, Element* attributeOwnerElement);
-    void invalidateNodeListsCacheAfterChildrenChanged();
+    void invalidateNodeListCachesInAncestors(const QualifiedName* attrName = 0, Element* attributeOwnerElement = 0);
     NodeListsNodeData* nodeLists();
     void removeCachedChildNodeList();
 

diff --git a/Source/WebCore/dom/NodeRareData.h b/Source/WebCore/dom/NodeRareData.h
index 1750810..0bdc3e8 100644 (file)
--- a/Source/WebCore/dom/NodeRareData.h
+++ b/Source/WebCore/dom/NodeRareData.h
@@ -222,10 +222,14 @@ public:
     NodeListsNodeData* ensureNodeLists()
     {
         if (!m_nodeLists)
-            createNodeLists();
+            setNodeLists(NodeListsNodeData::create());
         return m_nodeLists.get();
     }
-    void clearChildNodeListCache();
+    void clearChildNodeListCache()
+    {
+        if (m_childNodeList)
+            m_childNodeList->invalidateCache();
+    }
 
     ChildNodeList* childNodeList() const { return m_childNodeList; }
     void setChildNodeList(ChildNodeList* list) { m_childNodeList = list; }
@@ -344,7 +348,6 @@ protected:
     void setNeedsFocusAppearanceUpdateSoonAfterAttach(bool needs) { m_needsFocusAppearanceUpdateSoonAfterAttach = needs; }
 
 private:
-    void createNodeLists();
 
     TreeScope* m_treeScope;
     OwnPtr<NodeListsNodeData> m_nodeLists;

diff --git a/Source/WebCore/html/HTMLCollection.h b/Source/WebCore/html/HTMLCollection.h
index 8e5a4f6..1330a1b 100644 (file)
--- a/Source/WebCore/html/HTMLCollection.h
+++ b/Source/WebCore/html/HTMLCollection.h
@@ -63,7 +63,6 @@ protected:
 
 private:
     using DynamicNodeListCacheBase::isRootedAtDocument;
-    using DynamicNodeListCacheBase::shouldInvalidateOnAttributeChange;
     using DynamicNodeListCacheBase::setItemCache;
 
     mutable NodeCacheMap m_idCache;