[chromium] Should be able to destroy a CCLayerTreeHost without manually setting the root layer
Created attachment 159752 [details] Patch
Comment on attachment 159752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159752&action=review > Source/WebCore/ChangeLog:12 > + In the depths of time when dinosaurs roamed the earth, LayerChromium and CCLayerTreeHost were both reference > + counted and there was a cycle between the root LayerChromium and CCLayerTreeHost. This required all users of > + CCLayerTreeHost to manually break the cycle by calling setRootLayer(0) before dropping their reference to the > + host. Nowadays, CCLayerTreeHost has single ownership and LayerChromiums only have a weak pointer to their host > + so we should just do this cleanup ourselves instead of imposing it on callers. This is a fanciful historical retcon. CCLayerTreeHost doesn't have single ownership, although it'd be nice if it did. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:117 > + if (m_rootLayer) > + m_rootLayer->setLayerTreeHost(0); I think you need to do this recursively or make LayerChromium not ref-counted so that you can be guaranteed of its destruction here (possibly clearing its pointer first so that the CCLayerTreeHost weak pointer is still valid during ~LayerChromium). This code probably works at this moment given that the only other owner of LayerChromiums is the render surface layer lists, which are only non-empty during CCLTH::updateLayers, but it seems a little dodgy.
What I meant is CCLayerTreeHost is OwnPtr<>, not RefCounted. Maybe I should rewrite that? LayerChromium::setLayerTreeHost() is recursive - it'll set the m_layerTreeHost to 0 for all layers under the root (including mask/replicas)
Comment on attachment 159752 [details] Patch (In reply to comment #3) > What I meant is CCLayerTreeHost is OwnPtr<>, not RefCounted. Maybe I should rewrite that? Yeah, I guess "has single ownership" (ambiguous "...of something else") vs. "has a single owner". I don't feel too strongly about it. > LayerChromium::setLayerTreeHost() is recursive - it'll set the m_layerTreeHost to 0 for all layers under the root (including mask/replicas) Oh, quite right. I was misremembering. This seems totally fine. R=me.
"CCLayerTreeHost has a single owner" is much better, IMO.
Committed r126198: <http://trac.webkit.org/changeset/126198>