Bug 24110 – cloneNode should call cloneElement and not the reverse

• Component: DOM
• Status: RESOLVED FIXED

[Julien Chaffraix]

Suggested as part of the review of bug23956 as it would remove a static_cast.

[Julien Chaffraix]

Created attachment 27908 [details]
Trivial change: move cloneNode into cloneElement, updated cloneElement signature and call sites

[Darin Adler]

Comment on attachment 27908 [details]
Trivial change: move cloneNode into cloneElement, updated cloneElement signature and call sites

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.

[Julien Chaffraix]

Created attachment 28355 [details]
Version updated that should address all Darin's comments

[Darin Adler]

Comment on attachment 28355 [details]
Version updated that should address all Darin's comments

r=me

[Julien Chaffraix]

Landed in r41621.