[webkit-reviews] review denied: [Bug 71269] [chromium] Move resource-releasing logic into CCProxy and remove setNeedsCommit(without redraw) : [Attachment 113137] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Nat Duca <nduca at chromium.org>'s
request for review:
Bug 71269: [chromium] Move resource-releasing logic into CCProxy and remove
setNeedsCommit(without redraw)
https://bugs.webkit.org/show_bug.cgi?id=71269

Attachment 113137: Patch
https://bugs.webkit.org/attachment.cgi?id=113137&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=113137&action=review


The overflow looks good - I'm not too worried about the extra sync call. This
is missing a few bits, though, to ensure that we actually release all important
resources. I also think that we should leave setNeedsCommit in place for now.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:230
>      m_visible = visible;

does some higher level code avoid calling this when m_visible == visible or
should we check that?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:235
> +    m_proxy->setVisible(visible);

Could you add some comments here indicating what will happen on this call when
transitioning from true->false (i.e. that this will block for a while, etc)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:239
> +{

thread ASSERT() please

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:249
> +    m_proxy->setNeedsCommitThenRedraw();

We shouldn't actually need to redraw here. I think adding the 'thenRedraw' is
unnecessary here - the purpose here is that we need to push state to the impl
thread so it can make correct input routing decisions. I'd leave
setNeedsCommit() in as this is a legit use case for it.

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:193
> +	  
m_layerTreeHost->didBecomeInvisibleOnImplThread(m_layerTreeHostImpl.get());

probably need a scopedsetthread doohicky here to keep the thread assertions in
order.

you also need to call m_layerTreeHostImpl->setVisible() - that informs the
LayerRendererChromium of the visibility change so it releases the rendersurface
textures

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:263
> +   
m_layerTreeHost->didBecomeInvisibleOnImplThread(m_layerTreeHostImpl.get());

you also have to inform the m_layerTreeHostImpl of the visibility change -
that's what releases the render surface textures and calls
setVisibilityCHROMIUM() on the context to release shared memory segments, etc