Right now we call dispatchSubtreeModifiedEvent in order to invalidate name node list. But this is semantically incorrect. We shouldn't invalidating this cache as a side effect of dispatching DOMSubtreeModified event.
I'm hitting some obscure JS assertion failure/crash in this refactoring when I run DRT on LayoutTests/fast/dom/Attr/child-nodes-cache.html
Created attachment 118287 [details] work in progress
class DynamicNodeList : public NodeList { public: struct Caches : RefCounted<Caches> { static PassRefPtr<Caches> create(); void reset(); unsigned cachedLength; Node* lastItem; // of course, it's always a raw pointer that troubles us. unsigned lastItemOffset; bool isLengthCacheValid : 1; bool isItemCacheValid : 1; protected: Caches();
It turns out that I can't even make them RefPtr due to the way node's life-cycle is managed :(
Created attachment 118299 [details] cleanup
An obvious follow up patch would be make invalidateNodeListsCacheAfterAttributeChanges take bit flags or a qualified name so that we only invalidate node lists that are affected. e.g. when adding/removing class names, we don't have to invalidate label node list.
Ping reviewers.
Could someone review this patch? My other work to improve the performance, etc... is blocked on this patch.
Comment on attachment 118299 [details] cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=118299&action=review What’s the story on test coverage? > Source/WebCore/dom/ContainerNode.cpp:846 > - document()->nodeChildrenChanged(this); > + document()->updateRangesAfterNodeChanges(this); It seems awkward to call changes to the children of a node “node changes”. The terminology here is getting less precise in a way that I think makes things more confusing. > Source/WebCore/dom/ContainerNode.cpp:848 > if (treeScope()->hasNodeListCaches()) > - notifyNodeListsChildrenChanged(); > + invalidateNodeListsCacheAfterNodeChanges(); Why does this need to be called in some cases where updateRangesAfterNodeChanges is not called? > Source/WebCore/dom/Node.cpp:1041 > + if (node->isAttributeNode()) > + data->nodeLists()->invalidateCaches(); > + else > + data->nodeLists()->invalidateCachesThatDependOnAttributes(); I think this needs a why comment. It’s quite confusing even if it’s backwards.
Comment on attachment 118299 [details] cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=118299&action=review >> Source/WebCore/dom/ContainerNode.cpp:846 >> + document()->updateRangesAfterNodeChanges(this); > > It seems awkward to call changes to the children of a node “node changes”. The terminology here is getting less precise in a way that I think makes things more confusing. Will change to updateRangesAfterChildrenChanged then. >> Source/WebCore/dom/ContainerNode.cpp:848 >> + invalidateNodeListsCacheAfterNodeChanges(); > > Why does this need to be called in some cases where updateRangesAfterNodeChanges is not called? That's actually a good point. Will test & make a change if appropriate. >> Source/WebCore/dom/Node.cpp:1041 >> + data->nodeLists()->invalidateCachesThatDependOnAttributes(); > > I think this needs a why comment. It’s quite confusing even if it’s backwards. Okay will do.
Thanks for the review! (In reply to comment #9) > (From update of attachment 118299 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=118299&action=review > > What’s the story on test coverage? There are quite few tests in LayoutTests/fast/dom. e.g. LayoutTests/fast/dom/Attr/child-nodes-cache.html Also, a lot of tests use childNodes, getElementById, etc... so this code is probably sufficiently exercised.
(In reply to comment #10) > (From update of attachment 118299 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=118299&action=review > > >> Source/WebCore/dom/ContainerNode.cpp:848 > >> + invalidateNodeListsCacheAfterNodeChanges(); > > > > Why does this need to be called in some cases where updateRangesAfterNodeChanges is not called? > > That's actually a good point. Will test & make a change if appropriate. Turns out this is tested by fast/dom/NodeList/invalidate-node-lists-when-parsing.html Now I wonder what makes Range special so that we don't have to update upon changes made by the parser...
Committed r102431: <http://trac.webkit.org/changeset/102431>