[webkit-reviews] review denied: [Bug 24110] cloneNode should call cloneElement and not the reverse : [Attachment 27908] Trivial change: move cloneNode into cloneElement, updated cloneElement signature and call sites

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 24110: cloneNode should call cloneElement and not the reverse
https://bugs.webkit.org/show_bug.cgi?id=24110

Attachment 27908: Trivial change: move cloneNode into cloneElement, updated
cloneElement signature and call sites
https://bugs.webkit.org/attachment.cgi?id=27908&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks for doing this! I know I suggested it.

I do not like boolean arguments. I think they make call sites hard to read and
understand.

I think we should leave the cloneChildNodes logic inside the cloneNode
function, and then we would not need to add a boolean argument to cloneElement.


I also think that all the "shallow clone" comments at cloneElement call sites
make it clear that it should be renamed to shallowCloneElement or
cloneElementWithoutChildren, so we can remove those comments.

I think we should make the cloneNode virtual function override private, because
if someone has an Element* it's a waste to have them make a virtual function
call just to clone. Then, if we find callers that need a shallow clone, they
can call the current cloneElement (perhaps under its new clearer name) and if
we find callers that also want to clone the children, then I suggest we make a
new non-virtual cloneElement call that does clone children.