Bug 198578 – Implement ParentNode.prototype.replaceChildren

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

[Ryosuke Niwa]

Spec issue: https://github.com/whatwg/dom/issues/478
PR: https://github.com/whatwg/dom/pull/755

[Anne van Kesteren]

Latest PR at https://github.com/whatwg/dom/pull/851. I'm going to assume y'all are still interested.

[Chris Dumez]

(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.

[Tetsuharu Ohzeki [UTC+9]]

May I take this?

[Ryosuke Niwa]

(In reply to Tetsuharu Ohzeki from comment #3)
> May I take this?

You may!

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 400694 [details]
Patch

[Darin Adler]

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.

[Tetsuharu Ohzeki [UTC+9]]

(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.

[Darin Adler]

(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.

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 400735 [details]
Patch

I addressed the review comment.

[EWS]

Committed r262381: <https://trac.webkit.org/changeset/262381>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400735 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63833031>