From fe7d2301ca401861bd671014d237ad0ce8367c59 Mon Sep 17 00:00:00 2001 From: "rniwa@webkit.org" Date: Thu, 15 Mar 2012 00:10:55 +0000 Subject: [PATCH] (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): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@110797 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- Source/WebCore/ChangeLog | 57 ++++++++++++++++++++++++++++++++ Source/WebCore/dom/ContainerNode.cpp | 2 +- Source/WebCore/dom/DynamicNodeList.cpp | 2 -- Source/WebCore/dom/DynamicNodeList.h | 3 ++ Source/WebCore/dom/Node.cpp | 43 ++++++++---------------- Source/WebCore/dom/Node.h | 1 - Source/WebCore/dom/NodeRareData.h | 4 +-- Source/WebCore/html/HTMLLabelElement.cpp | 11 ------ Source/WebCore/html/HTMLLabelElement.h | 2 -- Source/WebCore/html/LabelsNodeList.cpp | 2 ++ 10 files changed, 79 insertions(+), 48 deletions(-) diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index 7194d97..840f6fa 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,3 +1,60 @@ +2012-03-12 Ryosuke Niwa + + (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 Unreviewed, rolling out r110565. diff --git a/Source/WebCore/dom/ContainerNode.cpp b/Source/WebCore/dom/ContainerNode.cpp index 4916eb1..14f29ae 100644 --- a/Source/WebCore/dom/ContainerNode.cpp +++ b/Source/WebCore/dom/ContainerNode.cpp @@ -55,7 +55,7 @@ typedef pair, unsigned> CallbackParameters; typedef pair CallbackInfo; typedef Vector NodeCallbackQueue; -typedef Vector, 1> NodeVector; +typedef Vector, 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 --- a/Source/WebCore/dom/DynamicNodeList.cpp +++ b/Source/WebCore/dom/DynamicNodeList.cpp @@ -31,12 +31,10 @@ namespace WebCore { DynamicSubtreeNodeList::DynamicSubtreeNodeList(PassRefPtr 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 --- a/Source/WebCore/dom/DynamicNodeList.h +++ b/Source/WebCore/dom/DynamicNodeList.h @@ -87,6 +87,9 @@ protected: DynamicSubtreeNodeList(PassRefPtr 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 --- 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 --- 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 --- 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 NodeListSet; - NodeListSet m_listsWithCaches; + NodeListSet m_listsInvalidatedAtDocument; typedef HashMap 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 --- 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 --- 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 --- 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 -- 2.7.4