Bug 94631 – [chromium] Should be able to destroy a CCLayerTreeHost without manually setting the root layer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Should be able to destroy a CCLayerTreeHost without manually setting the root layer

[James Robinson]

Created attachment 159752 [details]
Patch

[Adrienne Walker]

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.

[James Robinson]

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)

[Adrienne Walker]

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.

[James Robinson]

"CCLayerTreeHost has a single owner" is much better, IMO.

[James Robinson]

Committed r126198: <http://trac.webkit.org/changeset/126198>