Bug 74067 – Unify willMoveToNewOwnerDocument() and didMoveToNewOwnerDocument() for better performance

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

This change unify
    virtual void Node::willMoveToNewOwnerDocument();
    virtual void Node::didMoveToNewOwnerDocument();
into
    virtual void Node::didMoveToNewOwnerDocument(Document* oldDocment, Document* newDocument);

This is preparation for Bug 59816.
This also comes with a potential performance win because we kill two virtual function call down to one.

[Hajime Morrita]

Created attachment 118533 [details]
Patch

[Hajime Morrita]

Hi Dimitri, could you take a look at this?
This and blocking Bug 59816 pushes forward to make TreeScope first class citizen.

[WebKit Review Bot]

Comment on attachment 118533 [details]
Patch

Attachment 118533 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10831400

[Hajime Morrita]

The redness on cr-linux is apparently due to tree breakage at that time.

[Hajime Morrita]

Created attachment 118948 [details]
Patch

[Early Warning System Bot]

Comment on attachment 118948 [details]
Patch

Attachment 118948 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10847234

[Gustavo Noronha (kov)]

Comment on attachment 118948 [details]
Patch

Attachment 118948 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10846218

[Gyuyoung Kim]

Comment on attachment 118948 [details]
Patch

Attachment 118948 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10845219

[WebKit Review Bot]

Comment on attachment 118948 [details]
Patch

Attachment 118948 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10845218

[Hajime Morrita]

Created attachment 119137 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119137 [details]
Patch

Attachment 119137 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10874080

[Hajime Morrita]

Created attachment 120304 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 120304 [details]
Patch

Attachment 120304 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10998413

[Gyuyoung Kim]

Comment on attachment 120304 [details]
Patch

Attachment 120304 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10981480

[Early Warning System Bot]

Comment on attachment 120304 [details]
Patch

Attachment 120304 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10998418

[WebKit Review Bot]

Comment on attachment 120304 [details]
Patch

Attachment 120304 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10998416

[Darin Adler]

Comment on attachment 120304 [details]
Patch

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.

[Hajime Morrita]

Created attachment 120517 [details]
Patch

[Hajime Morrita]

Hi Darin, thanks you for taking look!
I updated the patch. Could you take another look?

> 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.
Right. The updated patch addresses this.

> 
> > 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.
Fixed.

[Darin Adler]

Comment on attachment 120517 [details]
Patch

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.

[Hajime Morrita]

Created attachment 120524 [details]
Patch

[Darin Adler]

Comment on attachment 120524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120524&action=review

Please fix the one thing I mentioned; otherwise this is ready to go.

> Source/WebCore/html/HTMLMediaElement.cpp:288
> +    if (m_readyState < HAVE_CURRENT_DATA && m_shouldDelayLoadEvent) {

We do not want to check m_readyState < HAVE_CURRENT_DATA here. If this somehow gets out of sync with m_shouldDelayLoadEvent it could result in us leaving a document with the wrong load event delay count. This logic should solely be based on the value of m_shouldDelayLoadEvent. The old code needed to check m_readyState because of a flaw in how the old code worked that is now fixed by your new function.

[Hajime Morrita]

Hi Darin, thanks for lighting review! I'll land this after addressing your point.

(In reply to comment #22)
> (From update of attachment 120524 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120524&action=review
> 
> Please fix the one thing I mentioned; otherwise this is ready to go.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:288
> > +    if (m_readyState < HAVE_CURRENT_DATA && m_shouldDelayLoadEvent) {
> 
> We do not want to check m_readyState < HAVE_CURRENT_DATA here. If this somehow gets out of sync with m_shouldDelayLoadEvent it could result in us leaving a document with the wrong load event delay count. This logic should solely be based on the value of m_shouldDelayLoadEvent. The old code needed to check m_readyState because of a flaw in how the old code worked that is now fixed by your new function.

[Hajime Morrita]

Created attachment 120526 [details]
Patch for landing

[Darin Adler]

Comment on attachment 120526 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=120526&action=review

> Source/WebCore/dom/Node.cpp:2511
> +#else
> +    UNUSED_PARAM(oldDocument);

Hmm, I just noticed that oldDocument is never used on either side of the if statement. That means that you should leave out the argument name altogether. Without that I think we’ll fail to compile when MUTATION_OBSERVERS is enabled.

> Source/WebCore/dom/StyledElement.cpp:185
> +    // Normally we would be concerned about reseting the parent of those declarations in StyledElement::didMoveToNewDocument().

I think it’s spelled resetting rather than reseting.

[WebKit Review Bot]

Comment on attachment 120526 [details]
Patch for landing

Clearing flags on attachment: 120526

Committed r103679: <http://trac.webkit.org/changeset/103679>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.