[webkit-reviews] review denied: [Bug 74067] Unify willMoveToNewOwnerDocument() and didMoveToNewOwnerDocument() for better performance : [Attachment 120517] 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 120517: Patch
https://bugs.webkit.org/attachment.cgi?id=120517&action=review

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


This is looking great. review- because of the HTMLInputElement being a bit too
messy.

Also, one last idea. I think didMoveToNewDocument is a fine name for this. I
don’t think we need to emphasize “owner document” just because the DOM does;
the old functions had awkward names, but no reason that we should keep those
awkward names.

> Source/WebCore/html/FormAssociatedElement.cpp:59
> +    if (element->fastHasAttribute(formAttr) && oldDocument)
> +	   oldDocument->unregisterFormElementWithFormAttribute(this);

Probably should check oldDocument first for better performance in the
new-element case.

> Source/WebCore/html/FormAssociatedElement.h:67
> +    void didMoveToNewOwnerDocument(Document* oldDocument) OVERRIDE;

This is not an override of a virtual function; I would not expect this to
compile at all!

> Source/WebCore/html/HTMLInputElement.cpp:1510
> +	   if (needsSuspensionCallback())

Since this function always calls needsSuspensionCallback at least once, it
would be better if it called it only once and stored it in a local variable.

> Source/WebCore/html/HTMLMediaElement.cpp:3401
> +void HTMLMediaElement::setShouldDelayLoadEvent(Document* doc, bool
shouldDelay)

Please don’t use the abbreviation “doc” in an argument name. The name document
would be better.

This new function is messy. It’s not good for the media element itself to
change its m_shouldDelayLoadEvent state when moving from one document to
another. Instead, only the count should be updated when the element moves from
document to the other and this function shouldn’t be called at all. I don’t
even think didMoveToNewOwnerDocument should look at m_readyState. It should
only look at m_shouldDelayLoadEvent.