[webkit-reviews] review denied: [Bug 68866] CompositeEditCommand::prune should remove subtree at once : [Attachment 108786] refactoring

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 68866: CompositeEditCommand::prune should remove subtree at once
https://bugs.webkit.org/show_bug.cgi?id=68866

Attachment 108786: refactoring
https://bugs.webkit.org/attachment.cgi?id=108786&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=108786&action=review


review- because of the traverseNextSibling issue, and in part due to lack of
test

> Source/WebCore/ChangeLog:9
> +	   This reduces the number of node removals from O(n) to O(1) where n
is the depth of the tree.

Can we make a performance test demonstrating this fix? Ojan’s performance
harness typically makes that easy.

> Source/WebCore/editing/CompositeEditCommand.cpp:254
> +	       n = n->nextSibling();

I think you want:

    n = n->traverseNextSibling(node);

here, not just nextSibling.

> Source/WebCore/editing/CompositeEditCommand.cpp:277
> +void CompositeEditCommand::prune(PassRefPtr<Node> prpNode)

Normally we only use the "prp" naming if we are transferring ownership to a
local variable with a simpler name. Since we’re not doing that here, a prp
prefix is not necessary.

> Source/WebCore/editing/CompositeEditCommand.cpp:280
> +	   removeNode(highestNodeToRemove);

This should probably have a “release” in it.