Spec issue: https://github.com/whatwg/dom/issues/478 PR: https://github.com/whatwg/dom/pull/755
Latest PR at https://github.com/whatwg/dom/pull/851. I'm going to assume y'all are still interested.
(In reply to Anne van Kesteren from comment #1) > Latest PR at https://github.com/whatwg/dom/pull/851. I'm going to assume > y'all are still interested. Sure.
May I take this?
(In reply to Tetsuharu Ohzeki from comment #3) > May I take this? You may!
Created attachment 400694 [details] Patch
Comment on attachment 400694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400694&action=review > Source/WebCore/dom/ContainerNode.cpp:674 > +// https://dom.spec.whatwg.org/#concept-node-replace-all > +void ContainerNode::replaceAllChildrenNaively(Ref<Node>&& node) > +{ > + Ref<ContainerNode> protectedThis(*this); > + ChildListMutationScope mutation(*this); > + removeAllChildrenWithScriptAssertion(ChildChangeSource::API, DeferChildrenChanged::Yes); > + > + // we assume that we check preinsertion validity on the caller side. > + auto result = appendChildWithoutPreInsertionValidityCheck(node); > + ASSERT_UNUSED(result, !result.hasException()); > + > + rebuildSVGExtensionsElementsIfNecessary(); > + dispatchSubtreeModifiedEvent(); > +} I don’t see the value in factoring this out as a named function. The pre-condition that you must already have checked pre-insertion validity makes it hard to use this correctly. I suggest just including this code in ContainerNode::replaceChildren for now, and factoring it out only if we find we have a good way to share code with other call sites. I don’t understand why it is OK that this does not use executeNodeInsertionWithScriptAssertion.
(In reply to Darin Adler from comment #6) > I don’t see the value in factoring this out as a named function. The > pre-condition that you must already have checked pre-insertion validity > makes it hard to use this correctly. I suggest just including this code in > ContainerNode::replaceChildren for now, and factoring it out only if we find > we have a good way to share code with other call sites. Thank you review! Okay. I'll change like that. > I don’t understand why it is OK that this does not use > executeNodeInsertionWithScriptAssertion. As my understanding, ContainerNode::appendChildWithoutPreInsertionValidityCheck calls executeNodeInsertionWithScriptAssertion internally and appendChildWithoutPreInsertionValidityCheck has additional operations related to there is a parent.
(In reply to Tetsuharu Ohzeki from comment #7) > (In reply to Darin Adler from comment #6) > > I don’t understand why it is OK that this does not use > > executeNodeInsertionWithScriptAssertion. > > ContainerNode::appendChildWithoutPreInsertionValidityCheck calls > executeNodeInsertionWithScriptAssertion internally I see that now.
Created attachment 400735 [details] Patch I addressed the review comment.
Committed r262381: <https://trac.webkit.org/changeset/262381> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400735 [details].
<rdar://problem/63833031>