Bug 93874

Summary: Add a was-inserted-into-tree notification to RenderObject
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Layout and RenderingAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric, esprehn, hyatt, inferno, leviw, mitz, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed change 1: Request for comment on the naming, introduce insertedIntoTree.
none
Proposed change 2: Fixed the Windows build by exposing enclosingRenderNamedFlowThread.
none
Patch for landing none

Description Julien Chaffraix 2012-08-13 11:36:38 PDT
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.
Comment 1 Julien Chaffraix 2012-08-13 13:09:26 PDT
Created attachment 158084 [details]
Proposed change 1: Request for comment on the naming, introduce insertedIntoTree.
Comment 2 Build Bot 2012-08-13 17:23:11 PDT
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
Comment 3 Julien Chaffraix 2012-08-14 10:18:32 PDT
Created attachment 158361 [details]
Proposed change 2: Fixed the Windows build by exposing enclosingRenderNamedFlowThread.
Comment 4 Eric Seidel (no email) 2012-08-14 12:23:37 PDT
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.
Comment 5 Eric Seidel (no email) 2012-08-14 12:24:54 PDT
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.
Comment 6 Julien Chaffraix 2012-08-14 18:50:02 PDT
(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.
Comment 7 Elliott Sprehn 2012-08-14 20:36:09 PDT
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())
Comment 8 Eric Seidel (no email) 2012-08-15 16:07:09 PDT
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. :)
Comment 9 Julien Chaffraix 2012-08-15 18:30:12 PDT
Created attachment 158679 [details]
Patch for landing
Comment 10 Julien Chaffraix 2012-08-15 18:31:54 PDT
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.
Comment 11 WebKit Review Bot 2012-08-15 19:35:45 PDT
Comment on attachment 158679 [details]
Patch for landing

Clearing flags on attachment: 158679

Committed r125737: <http://trac.webkit.org/changeset/125737>
Comment 12 WebKit Review Bot 2012-08-15 19:35:49 PDT
All reviewed patches have been landed.  Closing bug.