There are multiple WPT failures in shadow-dom/slotchange.html and shadow-dom/slotchange-event.html because of this.
<rdar://problem/43871061>
Created attachment 348477 [details] WIP
Comment on attachment 348477 [details] WIP Attachment 348477 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9031498 New failing tests: imported/w3c/web-platform-tests/shadow-dom/slotchange.html
Created attachment 348486 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 348477 [details] WIP Attachment 348477 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9032135 New failing tests: imported/w3c/web-platform-tests/shadow-dom/slotchange.html
Created attachment 348490 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Created attachment 348583 [details] WIP2
Created attachment 348594 [details] WIP3
Attachment 348594 [details] did not pass style-queue: ERROR: Source/WebCore/dom/SlotAssignment.cpp:129: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 348697 [details] WIP4 Finally I have something satisfactory in terms of perf & behavior. Will clean this up, and add more test cases. We already have 53 test cases but still doesn't seem to cover all the edge cases.
Created attachment 348699 [details] WIP4 (rebased for trunk)
Attachment 348699 [details] did not pass style-queue: ERROR: Source/WebCore/dom/SlotAssignment.cpp:242: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 348709 [details] WIP5 This bug is absolutely insane... There are so many crazy edge cases I have to worry about for both insertion & removal.
Attachment 348709 [details] did not pass style-queue: ERROR: Source/WebCore/dom/SlotAssignment.cpp:261: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 348710 [details] WIP6 Done some cleanup. The patch looks much better! One more test case to fix and we should be good to go.
Created attachment 348711 [details] WIP6 (with code change)
Attachment 348711 [details] did not pass style-queue: ERROR: Source/WebCore/dom/SlotAssignment.cpp:246: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 348839 [details] Fixes the bug
Comment on attachment 348839 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=348839&action=review r=me > Source/WebCore/ChangeLog:10 > + Let us consider each scenairo separately. 'scenario' > Source/WebCore/ChangeLog:51 > + node insertion, removal, or rename but no longer has after the mutation. This patch also adds a slot mutation > + version number, m_slotMutationVersion, which is incremented whenever a node is about to be inserted or removed, > + and slot resolution version, m_slotResolutionVersion, which is set to the current slot mutation version number > + when the full slot resolution is triggered during slot mutations. They are used to avoid redundant tree traversals I like the versioning approach. > Source/WebCore/ChangeLog:74 > + While the DOM specification currently specifies the former beavhior, this patch implements the latter behavior 'behavior' > Source/WebCore/dom/SlotAssignment.cpp:181 > + if (!slot->element) // A previous invocation to resolveSlotsAfterSlotMutation during this removal has updated this slot. > + ASSERT(m_slotResolutionVersion == m_slotMutationVersion && !findSlotElement(shadowRoot, name)); > + else { Maybe just ASSERT(slot->element || (m_slotResolutionVersion == m_slotMutationVersion && !findSlotElement(shadowRoot, name))); A branch with nothing but assert doesn't look nice.
Comment on attachment 348839 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=348839&action=review > Source/WebCore/dom/SlotAssignment.cpp:175 > + bool elementWasRenamed = !oldParentOfRemovedTree; This is a rather magical way to signal the rename case.
(In reply to Antti Koivisto from comment #19) > Comment on attachment 348839 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348839&action=review > > r=me > > > Source/WebCore/ChangeLog:10 > > + Let us consider each scenairo separately. > > 'scenario' Fixed. > > Source/WebCore/ChangeLog:51 > > + node insertion, removal, or rename but no longer has after the mutation. This patch also adds a slot mutation > > + version number, m_slotMutationVersion, which is incremented whenever a node is about to be inserted or removed, > > + and slot resolution version, m_slotResolutionVersion, which is set to the current slot mutation version number > > + when the full slot resolution is triggered during slot mutations. They are used to avoid redundant tree traversals > > I like the versioning approach. Great! > > Source/WebCore/ChangeLog:74 > > + While the DOM specification currently specifies the former beavhior, this patch implements the latter behavior > > 'behavior' Fixed. > > Source/WebCore/dom/SlotAssignment.cpp:181 > > + if (!slot->element) // A previous invocation to resolveSlotsAfterSlotMutation during this removal has updated this slot. > > + ASSERT(m_slotResolutionVersion == m_slotMutationVersion && !findSlotElement(shadowRoot, name)); > > + else { > > Maybe just > > ASSERT(slot->element || (m_slotResolutionVersion == m_slotMutationVersion && > !findSlotElement(shadowRoot, name))); > > A branch with nothing but assert doesn't look nice. Sure. Fixed.
(In reply to Antti Koivisto from comment #20) > Comment on attachment 348839 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348839&action=review > > > Source/WebCore/dom/SlotAssignment.cpp:175 > > + bool elementWasRenamed = !oldParentOfRemovedTree; > > This is a rather magical way to signal the rename case. Renamed oldParentOfRemovedTree to oldParentOfRemovedTreeForRemoval as we discussed.
Committed r235650: <https://trac.webkit.org/changeset/235650>
*** Bug 190204 has been marked as a duplicate of this bug. ***
Mass move bugs into the DOM component.