This is a follow-up of bug 93874 that added the equivalent notification for insertion. This notification would help us properly invalidate on tree removal instead of relying on willBeDestroyed for most of the invalidations. willBeDestroyed will usually follow will-be-removed-from-tree but not always (there are cases where we move the renderer around, especially during splitting). This difference is likely the cause of some subtle bugs in rendering as we don't invalidate as expected in those cases, unless we implement some ad-hoc invalidation (f.e. markBoxForRelayoutAfterSplit in RenderBox).
Created attachment 158956 [details] Proposed implementation 1, welcome any comment on the naming.
Comment on attachment 158956 [details] Proposed implementation 1, welcome any comment on the naming. View in context: https://bugs.webkit.org/attachment.cgi?id=158956&action=review > Source/WebCore/rendering/RenderObjectChildList.cpp:88 > + if (!owner->documentBeingDestroyed() && fullRemove) Rename to notifyRenderer?
Comment on attachment 158956 [details] Proposed implementation 1, welcome any comment on the naming. View in context: https://bugs.webkit.org/attachment.cgi?id=158956&action=review We should make that notifyRenderer change as Elliot said in last comment. This matches http://trac.webkit.org/changeset/125737. > Source/WebCore/rendering/RenderObject.cpp:2416 > + if (isBox()) Can this if be moved to RenderBox::willBeRemovedFromTree ? I suspect the ordering of the regions calls here will cause problem. Lets cc mihnea@adobe.com to confirm.
(In reply to comment #3) > (From update of attachment 158956 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158956&action=review > > We should make that notifyRenderer change as Elliot said in last comment. This matches http://trac.webkit.org/changeset/125737. > > > Source/WebCore/rendering/RenderObject.cpp:2416 > > + if (isBox()) > > Can this if be moved to RenderBox::willBeRemovedFromTree ? I suspect the ordering of the regions calls here will cause problem. Lets cc mihnea@adobe.com to confirm. The calls to clear the render box region info and render object region style can be performed in any order, as they are removing information for the same render object from 2 different data structures, which serve different purposes.
Comment on attachment 158956 [details] Proposed implementation 1, welcome any comment on the naming. View in context: https://bugs.webkit.org/attachment.cgi?id=158956&action=review >>> Source/WebCore/rendering/RenderObject.cpp:2416 >>> + if (isBox()) >> >> Can this if be moved to RenderBox::willBeRemovedFromTree ? I suspect the ordering of the regions calls here will cause problem. Lets cc mihnea@adobe.com to confirm. > > The calls to clear the render box region info and render object region style can be performed in any order, as they are removing information for the same render object from 2 different data structures, which serve different purposes. Actually we shouldn't move to RenderBox since inRenderFlowThread() will be false most of the time. So, we should keep it the same way as it is.
Comment on attachment 158956 [details] Proposed implementation 1, welcome any comment on the naming. View in context: https://bugs.webkit.org/attachment.cgi?id=158956&action=review >> Source/WebCore/rendering/RenderObjectChildList.cpp:88 >> + if (!owner->documentBeingDestroyed() && fullRemove) > > Rename to notifyRenderer? Changed in the patch for landing. Thanks for the spotting that Elliot & Abhishek.
Created attachment 159193 [details] Patch for landing
Comment on attachment 159193 [details] Patch for landing Clearing flags on attachment: 159193 Committed r125940: <http://trac.webkit.org/changeset/125940>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 94398
The ASSERT in RenderObject::willBeRemoved was hit several times and the build bots caught that. It looks like we have the same issue as RenderObject::insertedIntoTree where the ASSERT would be nice but it would require fixing the rest of the code.
(In reply to comment #11) > The ASSERT in RenderObject::willBeRemoved was hit several times and the build bots caught that. It looks like we have the same issue as RenderObject::insertedIntoTree where the ASSERT would be nice but it would require fixing the rest of the code. Per discussion with Abhishek, I will just land a new patch without the ASSERT as it wasn't required as part of this change (it would have been nice to have though).
Created attachment 159467 [details] Patch for landing
Comment on attachment 159467 [details] Patch for landing Clearing flags on attachment: 159467 Committed r126048: <http://trac.webkit.org/changeset/126048>