(register|unregister)DynamicSubtreeNodeList should be called only for labels and...

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

Reviewed by Antti Koivisto.

Cleaned up invalidation code for dynamic node lists. It seems like the existing code was utterly confused
about the lifetime of node lists and caches within them. First, register/unregsiterDynamicSubtreeNodeList
are called for all dynamic node lists even though it's only useful for labels and region node lists since
it's a mechanism to allow node lists to be invalidated at a node to which the node list doesn't belong.

Second, some node lists had dedicated member functions on Node to explicitly invalidate caches in
parsedAttribute. However, this is redundant because invalidateNodeListsCacheAfterAttributeChanged should be
able to invalidate caches when the attribute value changes. This patch gets rid of the last instance of such
function in HTMLLabelElement.

And finally, this patch restricts the callers of DynamicSubtreeNodeList::invalidateCache to be member
functions of NodeListsNodeData (now friends of DynamicSubtreeNodeList) to allow futher refactoring.

* dom/DynamicNodeList.cpp:
(WebCore::DynamicSubtreeNodeList::DynamicSubtreeNodeList): Don't register the node list since it's only
useful for labels and region node lists.
(WebCore::DynamicSubtreeNodeList::~DynamicSubtreeNodeList): Ditto.
* dom/DynamicNodeList.h:
(DynamicSubtreeNodeList):
* dom/Node.cpp:
(WebCore):
(WebCore::Node::registerDynamicSubtreeNodeList): The comment about now we have to invalidate caches
when there had no caches is incorrect because registerDynamicSubtreeNodeList is called when a node list
is initially created. Also, if the tree scope didn't have any caches, then this is the first node list
to be added to the list, so there's no point in calling InvalidateCaches (no-op).
(WebCore::Node::unregisterDynamicSubtreeNodeList):
(WebCore::Node::invalidateNodeListsCacheAfterAttributeChanged): Take care of "for" content attribute.
Also remove the redundant call to removeNodeListCacheIfPossible since we only invalidates node lists
and never remove entries from NodeListsNodeData in this function.
(WebCore::Node::invalidateNodeListsCacheAfterChildrenChanged): The call to removeNodeListCacheIfPossible
is also redundant here. Also removed the invalidation of m_listsWithCaches since it's already done in
invalidateCaches via invalidateCachesThatDependOnAttributes.
(WebCore::NodeListsNodeData::invalidateCaches): Removed the invalidation of m_labelsNodeListCache. It's
done in invalidateCachesThatDependOnAttributes.
(WebCore::NodeListsNodeData::invalidateCachesThatDependOnAttributes): Invalidate
m_listsInvalidatedAtDocument, which is renamed from m_listsWithCaches.
(WebCore::NodeListsNodeData::isEmpty):
* dom/Node.h:
(Node):
* dom/NodeRareData.h:
(NodeListsNodeData):
* html/HTMLLabelElement.cpp:
(WebCore): Removed parseAttribute since the invalidation labels node list is now done by
invalidateNodeListsCacheAfterAttributeChanged and invalidateNodeListsCacheAfterChildrenChanged.
* html/HTMLLabelElement.h:
(HTMLLabelElement):
* html/LabelsNodeList.cpp:
(WebCore::LabelsNodeList::LabelsNodeList):
(WebCore::LabelsNodeList::~LabelsNodeList):

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

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 7194d97..840f6fa 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,60 @@
+2012-03-12  Ryosuke Niwa  <rniwa@webkit.org>
+
+        (register|unregister)DynamicSubtreeNodeList should be called only for labels and regions node lists
+        https://bugs.webkit.org/show_bug.cgi?id=80900
+
+        Reviewed by Antti Koivisto.
+
+        Cleaned up invalidation code for dynamic node lists. It seems like the existing code was utterly confused
+        about the lifetime of node lists and caches within them. First, register/unregsiterDynamicSubtreeNodeList
+        are called for all dynamic node lists even though it's only useful for labels and region node lists since
+        it's a mechanism to allow node lists to be invalidated at a node to which the node list doesn't belong.
+
+        Second, some node lists had dedicated member functions on Node to explicitly invalidate caches in
+        parsedAttribute. However, this is redundant because invalidateNodeListsCacheAfterAttributeChanged should be
+        able to invalidate caches when the attribute value changes. This patch gets rid of the last instance of such
+        function in HTMLLabelElement.
+
+        And finally, this patch restricts the callers of DynamicSubtreeNodeList::invalidateCache to be member
+        functions of NodeListsNodeData (now friends of DynamicSubtreeNodeList) to allow futher refactoring.
+
+        * dom/DynamicNodeList.cpp:
+        (WebCore::DynamicSubtreeNodeList::DynamicSubtreeNodeList): Don't register the node list since it's only
+        useful for labels and region node lists.
+        (WebCore::DynamicSubtreeNodeList::~DynamicSubtreeNodeList): Ditto.
+        * dom/DynamicNodeList.h:
+        (DynamicSubtreeNodeList):
+        * dom/Node.cpp:
+        (WebCore):
+        (WebCore::Node::registerDynamicSubtreeNodeList): The comment about now we have to invalidate caches
+        when there had no caches is incorrect because registerDynamicSubtreeNodeList is called when a node list
+        is initially created. Also, if the tree scope didn't have any caches, then this is the first node list
+        to be added to the list, so there's no point in calling InvalidateCaches (no-op).
+        (WebCore::Node::unregisterDynamicSubtreeNodeList):
+        (WebCore::Node::invalidateNodeListsCacheAfterAttributeChanged): Take care of "for" content attribute.
+        Also remove the redundant call to removeNodeListCacheIfPossible since we only invalidates node lists
+        and never remove entries from NodeListsNodeData in this function.
+        (WebCore::Node::invalidateNodeListsCacheAfterChildrenChanged): The call to removeNodeListCacheIfPossible
+        is also redundant here. Also removed the invalidation of m_listsWithCaches since it's already done in
+        invalidateCaches via invalidateCachesThatDependOnAttributes.
+        (WebCore::NodeListsNodeData::invalidateCaches): Removed the invalidation of m_labelsNodeListCache. It's
+        done in invalidateCachesThatDependOnAttributes.
+        (WebCore::NodeListsNodeData::invalidateCachesThatDependOnAttributes): Invalidate
+        m_listsInvalidatedAtDocument, which is renamed from m_listsWithCaches.
+        (WebCore::NodeListsNodeData::isEmpty):
+        * dom/Node.h:
+        (Node):
+        * dom/NodeRareData.h:
+        (NodeListsNodeData):
+        * html/HTMLLabelElement.cpp:
+        (WebCore): Removed parseAttribute since the invalidation labels node list is now done by
+        invalidateNodeListsCacheAfterAttributeChanged and invalidateNodeListsCacheAfterChildrenChanged.
+        * html/HTMLLabelElement.h:
+        (HTMLLabelElement):
+        * html/LabelsNodeList.cpp:
+        (WebCore::LabelsNodeList::LabelsNodeList):
+        (WebCore::LabelsNodeList::~LabelsNodeList):
+
 2012-03-14  Sheriff Bot  <webkit.review.bot@gmail.com>
 
         Unreviewed, rolling out r110565.

diff --git a/Source/WebCore/dom/ContainerNode.cpp b/Source/WebCore/dom/ContainerNode.cpp
index 4916eb1..14f29ae 100644 (file)
--- a/Source/WebCore/dom/ContainerNode.cpp
+++ b/Source/WebCore/dom/ContainerNode.cpp
@@ -55,7 +55,7 @@ typedef pair<RefPtr<Node>, unsigned> CallbackParameters;
 typedef pair<NodeCallback, CallbackParameters> CallbackInfo;
 typedef Vector<CallbackInfo> NodeCallbackQueue;
 
-typedef Vector<RefPtr<Node>, 1> NodeVector;
+typedef Vector<RefPtr<Node>, 11> NodeVector;
 static NodeCallbackQueue* s_postAttachCallbackQueue;
 
 static size_t s_attachDepth;

diff --git a/Source/WebCore/dom/DynamicNodeList.cpp b/Source/WebCore/dom/DynamicNodeList.cpp
index 65757b4..57cbee5 100644 (file)
--- a/Source/WebCore/dom/DynamicNodeList.cpp
+++ b/Source/WebCore/dom/DynamicNodeList.cpp
@@ -31,12 +31,10 @@ namespace WebCore {
 DynamicSubtreeNodeList::DynamicSubtreeNodeList(PassRefPtr<Node> node)
     : DynamicNodeList(node)
 {
-    rootNode()->registerDynamicSubtreeNodeList(this);
 }
 
 DynamicSubtreeNodeList::~DynamicSubtreeNodeList()
 {
-    rootNode()->unregisterDynamicSubtreeNodeList(this);
 }
 
 unsigned DynamicSubtreeNodeList::length() const

diff --git a/Source/WebCore/dom/DynamicNodeList.h b/Source/WebCore/dom/DynamicNodeList.h
index 7a0b84d..d8065f6 100644 (file)
--- a/Source/WebCore/dom/DynamicNodeList.h
+++ b/Source/WebCore/dom/DynamicNodeList.h
@@ -87,6 +87,9 @@ protected:
     DynamicSubtreeNodeList(PassRefPtr<Node> rootNode);
 
 private:
+    using DynamicNodeList::invalidateCache;
+    friend class NodeListsNodeData;
+
     Node* itemForwardsFromCurrent(Node* start, unsigned offset, int remainingOffset) const;
     Node* itemBackwardsFromCurrent(Node* start, unsigned offset, int remainingOffset) const;
 };

diff --git a/Source/WebCore/dom/Node.cpp b/Source/WebCore/dom/Node.cpp
index 2c05437..91ea63e 100644 (file)
--- a/Source/WebCore/dom/Node.cpp
+++ b/Source/WebCore/dom/Node.cpp
@@ -940,22 +940,22 @@ static void removeNodeListCacheIfPossible(Node* node, NodeRareData* data)
     node->treeScope()->removeNodeListCache();
 }
 
+// FIXME: Move this function to Document
 void Node::registerDynamicSubtreeNodeList(DynamicSubtreeNodeList* list)
 {
+    ASSERT(isDocumentNode());
     NodeRareData* data = ensureRareData();
-    // We haven't been receiving notifications while there were no registered lists, so the cache is invalid now.
-    if (data->nodeLists() && (!treeScope() || !treeScope()->hasNodeListCaches()))
-        data->nodeLists()->invalidateCaches();
-
-    data->ensureNodeLists(this)->m_listsWithCaches.add(list);
+    data->ensureNodeLists(this)->m_listsInvalidatedAtDocument.add(list);
 }
 
+// FIXME: Move this function to Document
 void Node::unregisterDynamicSubtreeNodeList(DynamicSubtreeNodeList* list)
 {
+    ASSERT(isDocumentNode());
     ASSERT(hasRareData());
     ASSERT(rareData()->nodeLists());
     NodeRareData* data = rareData();
-    data->nodeLists()->m_listsWithCaches.remove(list);
+    data->nodeLists()->m_listsInvalidatedAtDocument.remove(list);
     removeNodeListCacheIfPossible(this, data);
 }
 
@@ -974,7 +974,8 @@ void Node::invalidateNodeListsCacheAfterAttributeChanged(const QualifiedName& at
         && attrName != itempropAttr
         && attrName != itemtypeAttr
 #endif
-        && attrName != nameAttr)
+        && attrName != nameAttr
+        && attrName != forAttr)
         return;
 
     if (!treeScope()->hasNodeListCaches())
@@ -989,7 +990,6 @@ void Node::invalidateNodeListsCacheAfterAttributeChanged(const QualifiedName& at
             continue;
 
         data->nodeLists()->invalidateCachesThatDependOnAttributes();
-        removeNodeListCacheIfPossible(node, data);
     }
 }
 
@@ -1008,27 +1008,9 @@ void Node::invalidateNodeListsCacheAfterChildrenChanged()
             continue;
 
         data->nodeLists()->invalidateCaches();
-
-        NodeListsNodeData::NodeListSet::iterator end = data->nodeLists()->m_listsWithCaches.end();
-        for (NodeListsNodeData::NodeListSet::iterator it = data->nodeLists()->m_listsWithCaches.begin(); it != end; ++it)
-            (*it)->invalidateCache();
-
-        removeNodeListCacheIfPossible(node, data);
     }
 }
 
-void Node::notifyLocalNodeListsLabelChanged()
-{
-    if (!hasRareData())
-        return;
-    NodeRareData* data = rareData();
-    if (!data->nodeLists())
-        return;
-
-    if (data->nodeLists()->m_labelsNodeListCache)
-        data->nodeLists()->m_labelsNodeListCache->invalidateCache();
-}
-
 void Node::removeCachedClassNodeList(ClassNodeList* list, const String& className)
 {
     ASSERT(rareData());
@@ -2307,8 +2289,6 @@ void Node::showTreeForThisAcrossFrame() const
 
 void NodeListsNodeData::invalidateCaches()
 {
-    if (m_labelsNodeListCache)
-        m_labelsNodeListCache->invalidateCache();
     TagNodeListCache::const_iterator tagCacheEnd = m_tagNodeListCache.end();
     for (TagNodeListCache::const_iterator it = m_tagNodeListCache.begin(); it != tagCacheEnd; ++it)
         it->second->invalidateCache();
@@ -2320,6 +2300,11 @@ void NodeListsNodeData::invalidateCaches()
 
 void NodeListsNodeData::invalidateCachesThatDependOnAttributes()
 {
+    // Used by labels and region node lists on document.
+    NodeListsNodeData::NodeListSet::iterator end = m_listsInvalidatedAtDocument.end();
+    for (NodeListsNodeData::NodeListSet::iterator it = m_listsInvalidatedAtDocument.begin(); it != end; ++it)
+        (*it)->invalidateCache();
+
     ClassNodeListCache::iterator classCacheEnd = m_classNodeListCache.end();
     for (ClassNodeListCache::iterator it = m_classNodeListCache.begin(); it != classCacheEnd; ++it)
         it->second->invalidateCache();
@@ -2339,7 +2324,7 @@ void NodeListsNodeData::invalidateCachesThatDependOnAttributes()
 
 bool NodeListsNodeData::isEmpty() const
 {
-    if (!m_listsWithCaches.isEmpty())
+    if (!m_listsInvalidatedAtDocument.isEmpty())
         return false;
 
     if (!m_tagNodeListCache.isEmpty())

diff --git a/Source/WebCore/dom/Node.h b/Source/WebCore/dom/Node.h
index 36c0c99..17cb60d 100644 (file)
--- a/Source/WebCore/dom/Node.h
+++ b/Source/WebCore/dom/Node.h
@@ -528,7 +528,6 @@ public:
     void unregisterDynamicSubtreeNodeList(DynamicSubtreeNodeList*);
     void invalidateNodeListsCacheAfterAttributeChanged(const QualifiedName&);
     void invalidateNodeListsCacheAfterChildrenChanged();
-    void notifyLocalNodeListsLabelChanged();
     void removeCachedClassNodeList(ClassNodeList*, const String&);
 
     void removeCachedNameNodeList(NameNodeList*, const String&);

diff --git a/Source/WebCore/dom/NodeRareData.h b/Source/WebCore/dom/NodeRareData.h
index 004f52a..fc17f2d 100644 (file)
--- a/Source/WebCore/dom/NodeRareData.h
+++ b/Source/WebCore/dom/NodeRareData.h
@@ -51,7 +51,7 @@ struct NodeListsNodeData {
     WTF_MAKE_NONCOPYABLE(NodeListsNodeData); WTF_MAKE_FAST_ALLOCATED;
 public:
     typedef HashSet<DynamicSubtreeNodeList*> NodeListSet;
-    NodeListSet m_listsWithCaches;
+    NodeListSet m_listsInvalidatedAtDocument;
 
     typedef HashMap<String, ClassNodeList*> ClassNodeListCache;
     ClassNodeListCache m_classNodeListCache;
@@ -76,7 +76,7 @@ public:
     {
         return adoptPtr(new NodeListsNodeData);
     }
-    
+
     void invalidateCaches();
     void invalidateCachesThatDependOnAttributes();
 

diff --git a/Source/WebCore/html/HTMLLabelElement.cpp b/Source/WebCore/html/HTMLLabelElement.cpp
index b4748d6..6cbd219 100644 (file)
--- a/Source/WebCore/html/HTMLLabelElement.cpp
+++ b/Source/WebCore/html/HTMLLabelElement.cpp
@@ -154,15 +154,4 @@ void HTMLLabelElement::accessKeyAction(bool sendMouseEvents)
         HTMLElement::accessKeyAction(sendMouseEvents);
 }
 
-void HTMLLabelElement::parseAttribute(Attribute* attribute)
-{
-    if (attribute->name() == forAttr) {
-        // htmlFor attribute change affects other nodes than this.
-        // Clear the caches to ensure that the labels caches are cleared.
-        if (document())
-            document()->notifyLocalNodeListsLabelChanged();
-    } else
-        HTMLElement::parseAttribute(attribute);
-}
-                
 } // namespace

diff --git a/Source/WebCore/html/HTMLLabelElement.h b/Source/WebCore/html/HTMLLabelElement.h
index 526379c..32a55e4 100644 (file)
--- a/Source/WebCore/html/HTMLLabelElement.h
+++ b/Source/WebCore/html/HTMLLabelElement.h
@@ -50,8 +50,6 @@ private:
     virtual void defaultEventHandler(Event*);
 
     void focus(bool restorePreviousSelection = true);
-
-    virtual void parseAttribute(Attribute*) OVERRIDE;
 };
 
 } //namespace

diff --git a/Source/WebCore/html/LabelsNodeList.cpp b/Source/WebCore/html/LabelsNodeList.cpp
index 5354c2d..5554c12 100644 (file)
--- a/Source/WebCore/html/LabelsNodeList.cpp
+++ b/Source/WebCore/html/LabelsNodeList.cpp
@@ -35,11 +35,13 @@ using namespace HTMLNames;
 LabelsNodeList::LabelsNodeList(Node* forNode )
     : DynamicSubtreeNodeList(forNode->document()) , m_forNode(forNode)
 {
+    m_forNode->document()->registerDynamicSubtreeNodeList(this);
 }
 
 LabelsNodeList::~LabelsNodeList()
 {
     m_forNode->removeCachedLabelsNodeList(this);
+    m_forNode->document()->unregisterDynamicSubtreeNodeList(this);
 } 
     
 bool LabelsNodeList::nodeMatches(Element* testNode) const