Bug 94271

Summary: Introduce a will-be-removed-from-tree notification in 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, mihnea, mitz, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 94398    
Bug Blocks:    
Attachments:
Description Flags
Proposed implementation 1, welcome any comment on the naming.
none
Patch for landing
none
Patch for landing none

Description Julien Chaffraix 2012-08-16 17:09:48 PDT
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).
Comment 1 Julien Chaffraix 2012-08-16 17:28:05 PDT
Created attachment 158956 [details]
Proposed implementation 1, welcome any comment on the naming.
Comment 2 Elliott Sprehn 2012-08-16 17:58:10 PDT
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 3 Abhishek Arya 2012-08-16 20:57:33 PDT
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.
Comment 4 Mihnea Ovidenie 2012-08-17 00:41:00 PDT
(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 5 Abhishek Arya 2012-08-17 13:40:10 PDT
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 6 Julien Chaffraix 2012-08-17 13:42:09 PDT
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.
Comment 7 Julien Chaffraix 2012-08-17 13:43:00 PDT
Created attachment 159193 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-08-17 14:59:24 PDT
Comment on attachment 159193 [details]
Patch for landing

Clearing flags on attachment: 159193

Committed r125940: <http://trac.webkit.org/changeset/125940>
Comment 9 WebKit Review Bot 2012-08-17 14:59:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 WebKit Review Bot 2012-08-17 17:05:49 PDT
Re-opened since this is blocked by 94398
Comment 11 Julien Chaffraix 2012-08-17 18:51:33 PDT
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.
Comment 12 Julien Chaffraix 2012-08-20 10:00:14 PDT
(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).
Comment 13 Julien Chaffraix 2012-08-20 10:28:13 PDT
Created attachment 159467 [details]
Patch for landing
Comment 14 WebKit Review Bot 2012-08-20 11:52:40 PDT
Comment on attachment 159467 [details]
Patch for landing

Clearing flags on attachment: 159467

Committed r126048: <http://trac.webkit.org/changeset/126048>
Comment 15 WebKit Review Bot 2012-08-20 11:52:45 PDT
All reviewed patches have been landed.  Closing bug.