Summary: | [chromium] Should be able to destroy a CCLayerTreeHost without manually setting the root layer | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Robinson <jamesr> | ||||
Component: | New Bugs | Assignee: | James Robinson <jamesr> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cc-bugs, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 94174 | ||||||
Attachments: |
|
Description
James Robinson
2012-08-21 13:16:34 PDT
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> |