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.
Created attachment 136689 [details] Proposed move 1: Move all the functions, leaving only enclosingLayer on RenderObject.h.
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.
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...?
updateNewChildAfterInsertion sounds fine.
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*
Created attachment 136709 [details] Proposed change 2: Updated the function name.
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.
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!
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.
Considering that RenderObject has nothing to do with RenderLayer (it's a RenderBoxModel-only concept) this seems like a good idea on the surface. :)
Created attachment 138585 [details] Proposed change 3: Maybe better API.
Simon is your best reviewer here. You might try cornering him on #webkit.
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.