[webkit-reviews] review granted: [Bug 198578] Implement ParentNode.prototype.replaceChildren : [Attachment 400694] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tetsuharu Ohzeki
<tetsuharu.ohzeki at gmail.com>'s request for review:
Bug 198578: Implement ParentNode.prototype.replaceChildren
https://bugs.webkit.org/show_bug.cgi?id=198578

Attachment 400694: Patch

https://bugs.webkit.org/attachment.cgi?id=400694&action=review




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 400694
  --> https://bugs.webkit.org/attachment.cgi?id=400694
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.