[webkit-reviews] review denied: [Bug 74067] Unify willMoveToNewOwnerDocument() and didMoveToNewOwnerDocument() for better performance : [Attachment 120304] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied MORITA Hajime <morrita at google.com>'s
request for review:
Bug 74067: Unify willMoveToNewOwnerDocument() and didMoveToNewOwnerDocument()
for better performance
https://bugs.webkit.org/show_bug.cgi?id=74067

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

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


The EWS bot is failing on the patch, so I think you’ll need a new version. The
idea seems fine, but it can be done in a more-lightweight fashion.

> Source/WebCore/dom/Node.h:693
> +    virtual void didMoveToNewOwnerDocument(const NodeOwnerChange&);

It seems to me we should just pass the old document. The new document is easy
to get, just call document(), so there is no need to pass it. Having a named
structure just to pass two arguments also seems unnecessarily heavyweight.

> Source/WebCore/html/HTMLFormControlElement.h:123
> +    virtual void didMoveToNewOwnerDocument(const NodeOwnerChange&);

Please use OVERRIDE in all the didMoveToNewOwnerDocument function declarations.
This will catch if we get the argument list wrong in any of them.