Bug 80900 – (register|unregister)DynamicSubtreeNodeList should be called only for labels and regions node lists

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

I don't think we need registerDynamicSubtreeNodeList and unregisterDynamicSubtreeNodeList.

In particular, the following comment seems no longer valid because TreeScopeAdopter::moveTreeToNewScope clears node list caches whenever nodes are adopted into a new tree scope.
    // 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();

[Ryosuke Niwa]

Created attachment 131427 [details]
cleanup

[Andreas Kling]

Comment on attachment 131427 [details]
cleanup

r=me.

[WebKit Review Bot]

Comment on attachment 131427 [details]
cleanup

Attachment 131427 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11941228

New failing tests:
fast/forms/label/labels-remove-parent-label.html
fast/forms/label/labels-set-htmlFor-attribute.html
fast/forms/label/labels-remove-htmlFor-attribute.html
fast/regions/get-regions-by-content-node-vert-rl.html
fast/regions/get-regions-by-content-node-horiz-bt.html
fast/forms/label/labels-add-htmlFor-label.html
fast/regions/get-regions-by-content-node-vert-lr.html
fast/regions/get-regions-by-content-node-horiz-tb.html
fast/forms/label/labels-change-htmlFor-attribute.html
fast/forms/label/labels-add-parent-label.html
fast/forms/label/labels-remove-htmlFor-label.html
fast/regions/get-regions-by-content-node2.html

[Ryosuke Niwa]

Comment on attachment 131427 [details]
cleanup

Ugh... this isn't quite right because of LabelsNodeList and RegionNodeList. They need to be notified of changes at the document level instead of at the node it belongs :(

[Ryosuke Niwa]

Changing the scope of the bug a little bit.

[Ryosuke Niwa]

Created attachment 131501 [details]
Patch

[WebKit Review Bot]

Comment on attachment 131501 [details]
Patch

Attachment 131501 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11942360

New failing tests:
fast/regions/get-regions-by-content-node-vert-lr.html
fast/regions/get-regions-by-content-node-horiz-bt.html
fast/regions/get-regions-by-content-node-vert-rl.html
fast/regions/get-regions-by-content-node-horiz-tb.html
fast/regions/get-regions-by-content-node.html
fast/regions/get-regions-by-content-node2.html

[Ryosuke Niwa]

Sigh... the bug 80638 is haunting me :(

[Ryosuke Niwa]

Created attachment 131560 [details]
Also fix the bug 80638

[Build Bot]

Comment on attachment 131560 [details]
Also fix the bug 80638

Attachment 131560 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11942407

[WebKit Review Bot]

Comment on attachment 131560 [details]
Also fix the bug 80638

Attachment 131560 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11945371

New failing tests:
fast/regions/get-regions-by-content-node-vert-lr.html
fast/regions/get-regions-by-content-node-horiz-bt.html
fast/regions/get-regions-by-content-node-vert-rl.html
fast/regions/get-regions-by-content-node-horiz-tb.html
fast/regions/get-regions-by-content-node.html
fast/regions/get-regions-by-content-node2.html

[Ryosuke Niwa]

ugh.... regions node lists were passing tests due to "redundant" cache invalidation magically updating otherwise stale node lists :-(

[Ryosuke Niwa]

Created attachment 131777 [details]
Updated for ToT

[Build Bot]

Comment on attachment 131777 [details]
Updated for ToT

Attachment 131777 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11948868

[Ryosuke Niwa]

Committed in http://trac.webkit.org/changeset/110797.

[Ryosuke Niwa]

Apparently I mangled patches together when I was landing a patch for https://bugs.webkit.org/show_bug.cgi?id=80706.

[Andreas Kling]

Comment on attachment 131777 [details]
Updated for ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=131777&action=review

r=me but please remove references to region node lists from the code/changelog.

Bonus points for removing a parseAttribute() overload. :)

> Source/WebCore/dom/DynamicNodeList.h:90
> +    DynamicNodeList::invalidateCache;

Needs "using" to build on win, but you know that.

[Ryosuke Niwa]

The patch was "preemptively" landed in http://trac.webkit.org/changeset/110797 by accident. Fixed the reviewer name in http://trac.webkit.org/changeset/110817.