Bug 83697 – Move RenderObject's layer manipulation functions to RenderLayer

• Component: Layout and Rendering
• Status: RESOLVED WONTFIX

[Julien Chaffraix]

Currently layer manipulation functions (addLayers, removeLayers...) are attached to RenderObject as they are called on any RenderObject and update its subtree, which may include some RenderBoxModelObject with RenderLayer (not reflected by the name unfortunately). However this artificially exposes RenderLayer on an object that shouldn't know about it.

All the layer's manipulation functions requires a RenderLayer and could thus be moved to RenderLayer.

Patch forthcoming.

[Julien Chaffraix]

Created attachment 136689 [details]
Proposed move 1: Move all the functions, leaving only enclosingLayer on RenderObject.h.

[Simon Fraser (smfr)]

Comment on attachment 136689 [details]
Proposed move 1: Move all the functions, leaving only enclosingLayer on RenderObject.h.

View in context: https://bugs.webkit.org/attachment.cgi?id=136689&action=review

I like this in general. Does it add any additional computational complexity?

> Source/WebCore/rendering/RenderObjectChildList.cpp:499
> +void RenderObjectChildList::updateNewChildAfterFullAppendInsert(RenderObject* owner, RenderObject* newChild)

I don't know what a "FullAppendInsert" is.

[Julien Chaffraix]

Comment on attachment 136689 [details]
Proposed move 1: Move all the functions, leaving only enclosingLayer on RenderObject.h.

View in context: https://bugs.webkit.org/attachment.cgi?id=136689&action=review

> I like this in general. Does it add any additional computational complexity?

It shouldn't. All the functions already require a RenderLayer so we are not going to extra lengths to get it. The moved code should have the same complexity.

>> Source/WebCore/rendering/RenderObjectChildList.cpp:499
>> +void RenderObjectChildList::updateNewChildAfterFullAppendInsert(RenderObject* owner, RenderObject* newChild)
> 
> I don't know what a "FullAppendInsert" is.

This is a concept of appendChildNode / insertChildNode but I agree it's super confusing. The code used to differentiate between full insertion / appending vs just moving a node. This difference looks like it was removed.

How about just updateNewChildAfterInsertion...?

[Simon Fraser (smfr)]

updateNewChildAfterInsertion sounds fine.

[Julien Chaffraix]

Comment on attachment 136689 [details]
Proposed move 1: Move all the functions, leaving only enclosingLayer on RenderObject.h.

View in context: https://bugs.webkit.org/attachment.cgi?id=136689&action=review

I will upload a new patch with the new name in a few.

>>> Source/WebCore/rendering/RenderObjectChildList.cpp:499
>>> +void RenderObjectChildList::updateNewChildAfterFullAppendInsert(RenderObject* owner, RenderObject* newChild)
>> 
>> I don't know what a "FullAppendInsert" is.
> 
> This is a concept of appendChildNode / insertChildNode but I agree it's super confusing. The code used to differentiate between full insertion / appending vs just moving a node. This difference looks like it was removed.
> 
> How about just updateNewChildAfterInsertion...?

Actually scratch what I wrote, the difference is still around. It's just that we are using RenderObject pointers as boolean arguments all over. *sigh*

[Julien Chaffraix]

Created attachment 136709 [details]
Proposed change 2: Updated the function name.

[Simon Fraser (smfr)]

Comment on attachment 136709 [details]
Proposed change 2: Updated the function name.

View in context: https://bugs.webkit.org/attachment.cgi?id=136709&action=review

> Source/WebCore/rendering/RenderLayer.cpp:1356
> +            beforeLayer = findNextLayer(subtreeRoot->parent(), subtreeRoot);
> +        addChild(toRenderBoxModelObject(currentObject)->layer(), beforeLayer);

I'd prefer a blank line before the addChild

> Source/WebCore/rendering/RenderLayer.cpp:1363
> +void RenderLayer::removeLayersFromSubtree(RenderObject* subtreeRoot)

I have a general unease with all of these methods that pass a RenderObject. There's some implicit assumption that the RO passed in has a layer that is related to |this| in some way. e.g. here, it's assumed that subtreeRoot has a layer that is a child of the current layer. If not, bad things can happen.

This issue also existed to some extent in the old code, but by moving the code, you've made the methods look more "API"-like on RenderLayer, providing a false sense of security.

[Julien Chaffraix]

Comment on attachment 136709 [details]
Proposed change 2: Updated the function name.

View in context: https://bugs.webkit.org/attachment.cgi?id=136709&action=review

>> Source/WebCore/rendering/RenderLayer.cpp:1363
>> +void RenderLayer::removeLayersFromSubtree(RenderObject* subtreeRoot)
> 
> I have a general unease with all of these methods that pass a RenderObject. There's some implicit assumption that the RO passed in has a layer that is related to |this| in some way. e.g. here, it's assumed that subtreeRoot has a layer that is a child of the current layer. If not, bad things can happen.
> 
> This issue also existed to some extent in the old code, but by moving the code, you've made the methods look more "API"-like on RenderLayer, providing a false sense of security.

That's a good point, I did not really want to modify the existing functions as part of this change. But it's likely time for us to rethink those as they were (and still are) clunky...

The current code works more or less always the same:
* find |subtreeRoot|'s enclosing layer.
* call the right function on the previous layer as all the functions assumes that all top layers in |subtreeRoot|'s subtree are child of the enclosing layer.

We could encapsulate that into some static helper methods (like RenderLayer::removeLayersFromSubtree(RenderObject*)). This would do a bit more tree traversal but would ensure no bad calls can be made.

Any better design welcome!

[Julien Chaffraix]

Just a heads-up, the static helpers look a lot more horrible, even if we get added tightening, I don't think it's worth that cost.

[Eric Seidel (no email)]

Considering that RenderObject has nothing to do with RenderLayer (it's a RenderBoxModel-only concept) this seems like a good idea on the surface. :)

[Julien Chaffraix]

Created attachment 138585 [details]
Proposed change 3: Maybe better API.

[Eric Seidel (no email)]

Simon is your best reviewer here.  You might try cornering him on #webkit.

[Julien Chaffraix]

Comment on attachment 138585 [details]
Proposed change 3: Maybe better API.

The patch has rotten and I don't think I will come around to update it.