Bug 80900 - (register|unregister)DynamicSubtreeNodeList should be called only for labels and regions node lists
Summary: (register|unregister)DynamicSubtreeNodeList should be called only for labels ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on: 81021
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-12 15:32 PDT by Ryosuke Niwa
Modified: 2012-03-14 20:52 PDT (History)
11 users (show)

See Also:


Attachments
cleanup (8.51 KB, patch)
2012-03-12 15:42 PDT, Ryosuke Niwa
kling: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (13.34 KB, patch)
2012-03-12 20:25 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Also fix the bug 80638 (14.43 KB, patch)
2012-03-13 00:02 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated for ToT (12.03 KB, patch)
2012-03-13 20:45 PDT, Ryosuke Niwa
kling: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-03-12 15:32:25 PDT
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();
Comment 1 Ryosuke Niwa 2012-03-12 15:42:07 PDT
Created attachment 131427 [details]
cleanup
Comment 2 Andreas Kling 2012-03-12 15:58:49 PDT
Comment on attachment 131427 [details]
cleanup

r=me.
Comment 3 WebKit Review Bot 2012-03-12 16:48:25 PDT
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
Comment 4 Ryosuke Niwa 2012-03-12 16:49:39 PDT
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 :(
Comment 5 Ryosuke Niwa 2012-03-12 18:38:57 PDT
Changing the scope of the bug a little bit.
Comment 6 Ryosuke Niwa 2012-03-12 20:25:40 PDT
Created attachment 131501 [details]
Patch
Comment 7 WebKit Review Bot 2012-03-12 23:26:17 PDT
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
Comment 8 Ryosuke Niwa 2012-03-12 23:49:08 PDT
Sigh... the bug 80638 is haunting me :(
Comment 9 Ryosuke Niwa 2012-03-13 00:02:04 PDT
Created attachment 131560 [details]
Also fix the bug 80638
Comment 10 Build Bot 2012-03-13 01:34:43 PDT
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
Comment 11 WebKit Review Bot 2012-03-13 01:37:49 PDT
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
Comment 12 Ryosuke Niwa 2012-03-13 10:34:25 PDT
ugh.... regions node lists were passing tests due to "redundant" cache invalidation magically updating otherwise stale node lists :-(
Comment 13 Ryosuke Niwa 2012-03-13 20:45:48 PDT
Created attachment 131777 [details]
Updated for ToT
Comment 14 Build Bot 2012-03-13 21:16:56 PDT
Comment on attachment 131777 [details]
Updated for ToT

Attachment 131777 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11948868
Comment 15 Ryosuke Niwa 2012-03-14 17:11:55 PDT
Committed in http://trac.webkit.org/changeset/110797.
Comment 16 Ryosuke Niwa 2012-03-14 20:20:35 PDT
Apparently I mangled patches together when I was landing a patch for https://bugs.webkit.org/show_bug.cgi?id=80706.
Comment 17 Andreas Kling 2012-03-14 20:35:30 PDT
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.
Comment 18 Ryosuke Niwa 2012-03-14 20:52:42 PDT
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.