Bug 93874 – Add a was-inserted-into-tree notification to RenderObject

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

Currently we have a situation where post-insertion tasks can be done in 2 places: in addChild or in RenderObjectChild low-level insertion methods (insertChildNode or appendChildNode).

One downside of using addChild to do post-insertion work is that:
* addChild is not guaranteed to be called due to the complex tree patching it does (generating anonymous wrappers, splitting flows, ...).
* addChild is called on the parent, never on the actual inserted child.

The low-level insertion methods have thus become a dumping ground for post-insertion tasks that should be run most of the time, leading to code duplication between the 2 methods.

Looking at the code, we could remove a lot of the complexity by adding a was-inserted-into-tree signal similar to Node::insertedInto. This would enable us to consolidate our post-insertion code in the different renderers.

[Julien Chaffraix]

Created attachment 158084 [details]
Proposed change 1: Request for comment on the naming, introduce insertedIntoTree.

[Build Bot]

Comment on attachment 158084 [details]
Proposed change 1: Request for comment on the naming, introduce insertedIntoTree.

Attachment 158084 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13493345

[Julien Chaffraix]

Created attachment 158361 [details]
Proposed change 2: Fixed the Windows build by exposing enclosingRenderNamedFlowThread.

[Eric Seidel (no email)]

My largest concern when reviewing this will be that there is some nod to the perf implications of this change.  Some microbenchmark which adds/removes a bunch of renderers would help show that this added virtual method is not a huge perf hit.

[Eric Seidel (no email)]

I suspect you don't necessarily need to write a new microbenchmark, one of the ones in PerformanceTests or dglazkov's performance-tests is likely to do a bunch of renderer addition/removal.  I expect one of them is to slow down with this, the question is how much, and how much we believe it to be representative of more common scenarios.

[Julien Chaffraix]

(In reply to comment #4)
> My largest concern when reviewing this will be that there is some nod to the perf implications of this change.  Some microbenchmark which adds/removes a bunch of renderers would help show that this added virtual method is not a huge perf hit.

Unless I am missing something, this change removes one virtual function call from the common path as it replaces the calls to isListItem() and isRenderRegion() (both virtuals) with one call to insertedIntoTree. For RenderListItem and RenderRegion, my guess would be that it's also 1 virtual function call as the compiler knows the parent function to call.

Doing some archeology, we used to have one more call there due to RenderQuote. Also the patch adding the isRenderRegion call (bug 66142) didn't seem to raise any performance comment. This only gives circumstantial evidence that it's not hot or that we have no perf coverage though.

[Elliott Sprehn]

Comment on attachment 158361 [details]
Proposed change 2: Fixed the Windows build by exposing enclosingRenderNamedFlowThread.

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

> Source/WebCore/rendering/RenderObject.cpp:2367
> +{

We should ASSERT(isRooted()) here and then fix all the bugs where we insert into detached subtrees (I know generated content is one of them).

> Source/WebCore/rendering/RenderQuote.cpp:57
> +    // renderers before going into the main render tree.

This issue is fixed once we can ASSERT(isRooted())

[Eric Seidel (no email)]

Comment on attachment 158361 [details]
Proposed change 2: Fixed the Windows build by exposing enclosingRenderNamedFlowThread.

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

LGTM.

> Source/WebCore/rendering/RenderQuote.cpp:53
> +void RenderQuote::insertedIntoTree()

It seems we can remove this, no?  This just needs to be a comment, not an implementation.

> Source/WebCore/rendering/RenderQuote.h:47
> +    virtual void insertedIntoTree() OVERRIDE;

Please remove. :)

[Julien Chaffraix]

Created attachment 158679 [details]
Patch for landing

[Julien Chaffraix]

Comment on attachment 158361 [details]
Proposed change 2: Fixed the Windows build by exposing enclosingRenderNamedFlowThread.

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

>> Source/WebCore/rendering/RenderObject.cpp:2367
>> +{
> 
> We should ASSERT(isRooted()) here and then fix all the bugs where we insert into detached subtrees (I know generated content is one of them).

Added a FIXME to track enabling this (super useful) ASSERT.

>> Source/WebCore/rendering/RenderQuote.h:47
>> +    virtual void insertedIntoTree() OVERRIDE;
> 
> Please remove. :)

Removed and put the comment here instead.

[WebKit Review Bot]

Comment on attachment 158679 [details]
Patch for landing

Clearing flags on attachment: 158679

Committed r125737: <http://trac.webkit.org/changeset/125737>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.