[webkit-reviews] review granted: [Bug 74028] It's semantically incorrect to call notifyNodeListsAttributeChanged in dispatchSubtreeModifiedEvent : [Attachment 118299] cleanup

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 74028: It's semantically incorrect to call notifyNodeListsAttributeChanged
in dispatchSubtreeModifiedEvent
https://bugs.webkit.org/show_bug.cgi?id=74028

Attachment 118299: cleanup
https://bugs.webkit.org/attachment.cgi?id=118299&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.