[chromium] LayerRendererChromium shouldn't be RefCounted
Created attachment 107397 [details] Patch
Comment on attachment 107397 [details] Patch I think this is a net positive change. I totally agree that OwnPtr<> reduces the chance of reference cycles, but I worry that making them raw pointers instead of RefPtr<>s increases the chance that a layer is holding onto a stale layer renderer pointer. Maybe this is a naive suggestion, but do you think it would be an improvement to pass a LayerRendererChromium* into the CCLayerImpl::draw() function to avoid having to store it on the layers themselves and thereby avoiding any chance that it's stale?
Comment on attachment 107397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107397&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:253 > + LayerRendererChromium* m_layerRenderer; Does the CCLayerImpl still need this pointer?
The most interesting lifecycle bit here is on lost context, which triggers a LayerRendererChromium recreation. Here's what happens then (in the CCSingleThreadProxy case which we use today, will be CCThreadProxy in the future): Initial state: LayerRendererChromium A, tree of CCLayerImpls that have (weak) pointers to A CCSingleThreadProxy detects a lost context and calls CCSingleThreadProxy::recreateContextIfNeeded() CCSingleThreadProxy::recreateContextIfNeeded() calls CCLayerTreeHostImpl::initializeLayerRenderer() CCLayerTreeHostImpl::initializeLayerRenderer() constructs a new LayerRendererChromium (B) with an initially empty CCLayerImpl tree CCLayerTreeHostImpl::initializeLayerRenderer() calls LayerRendererChromium::close() on A, which drops all rendersurface refs to CCLayerImpls CCLayerTreeHostImpl::initializeLayerRenderer() deletes the old LayerRendererChromium, which drops the last reference to its CCLayerImpl tree and destroys all CCLayerImpls (CCSingleThreadProxy::recreateContextIfNeeded() returns) CCSingleThreadProxy::compositeImmediately() calls commitIfTo, which calls the TreeSynchronizer, which constructs a new CCLayerImpl tree under LayerRendererChromium B LayerRendererChromium::drawLayers on B calls setLayerRendererRecursive() on all CCLayerImpls in the new tree and sets the LayerRendererChromium weak pointers Technically speaking we don't need LayerRendererChromium weak pointers on CCLayerImpls at all.
(In reply to comment #3) > (From update of attachment 107397 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107397&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:253 > > + LayerRendererChromium* m_layerRenderer; > > Does the CCLayerImpl still need this pointer? It doesn't. I can try to remove this as part of this patch if you'd like (it seems useful)
Looks like all 3 of us posted at the same time that CCLayerImpl weak pointer should go away :P. I'll kill it too and update this patch - hopefully that doesn't grow the thing too much.
Created attachment 107402 [details] remove LayerRendererChromium weak ptrs in favor of draw time parameter
Updated, PTAL.
(In reply to comment #8) > Updated, PTAL. This looks great. :)
Comment on attachment 107402 [details] remove LayerRendererChromium weak ptrs in favor of draw time parameter I was concerned that the dependency from the cc/ classes to LayerRendererChromium was a conceptual layering violation, but jamesr assures me that it's just that the LRC needs to be renamed to something like CCGLRendererImpl. This looks fine to me in that case.
Filed https://bugs.webkit.org/show_bug.cgi?id=68130 to track the poor name for LRC.
Committed r95135: <http://trac.webkit.org/changeset/95135>