Bug 94271 – Introduce a will-be-removed-from-tree notification in RenderObject

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

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

[Julien Chaffraix]

Created attachment 158956 [details]
Proposed implementation 1, welcome any comment on the naming.

[Elliott Sprehn]

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?

[Abhishek Arya]

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 [e-mail] to confirm.

[Mihnea Ovidenie]

(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 [e-mail] 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.

[Abhishek Arya]

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 [e-mail] 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.

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 159193 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 159193 [details]
Patch for landing

Clearing flags on attachment: 159193

Committed r125940: <http://trac.webkit.org/changeset/125940>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 94398

[Julien Chaffraix]

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.

[Julien Chaffraix]

(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).

[Julien Chaffraix]

Created attachment 159467 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 159467 [details]
Patch for landing

Clearing flags on attachment: 159467

Committed r126048: <http://trac.webkit.org/changeset/126048>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.