[webkit-reviews] review granted: [Bug 55209] Remove support for Node::willRemove() : [Attachment 140661] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Hajime Morrita
<morrita at google.com>'s request for review:
Bug 55209: Remove support for Node::willRemove()
https://bugs.webkit.org/show_bug.cgi?id=55209

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=140661&action=review


> Source/WebCore/ChangeLog:9
> +	   o5% speedup on Dromaeo dom-modify.

o5%?

> Source/WebCore/dom/ContainerNode.h:97
> +    // Note that this cleanup is done by ContainerNode in most cases.
> +    // Only the Document class needs to take care of itself since
ContainerNode doesn't care about its lifetime.

I don't really understand this comment. I think it's better not to add it.

> Source/WebCore/dom/ContainerNodeAlgorithms.cpp:124
> +    if (m_owner->parentNode() == m_ownerParent)
> +	   toFrameOwnerElement(m_owner.get())->disconnectContentFrame();

I think this function should be inline to avoid incurring cost when
m_owner->parentNode() != m_ownerParent.

> Source/WebCore/dom/ContainerNodeAlgorithms.h:299
> +inline void ChildFrameDisconnector::collectDescendant(Node* root)

I don't think this function needs to be inline since it's a recursive function
anyway.
I'm almost certain you can safely move it to cpp file.
I suspect compilers would reject this function from being inlined.