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.
Created attachment 158084 [details] Proposed change 1: Request for comment on the naming, introduce insertedIntoTree.
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
Created attachment 158361 [details] Proposed change 2: Fixed the Windows build by exposing enclosingRenderNamedFlowThread.
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.
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.
(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 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 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. :)
Created attachment 158679 [details] Patch for landing
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 on attachment 158679 [details] Patch for landing Clearing flags on attachment: 158679 Committed r125737: <http://trac.webkit.org/changeset/125737>
All reviewed patches have been landed. Closing bug.