Bug 71269 – [chromium] Move resource-releasing logic into CCProxy and remove setNeedsCommit(without redraw)

• Component: New Bugs
• Status: RESOLVED FIXED

[Nat Duca]

[chromium] Move resource-releasing logic into CCProxy and remove setNeedsCommit(without redraw)

[Nat Duca]

Created attachment 113137 [details]
Patch

[Nat Duca]

This code moves setVisible handling into the CCProxy.

Note that setVisible(false) needs to be blocking from perspective of the CCLayerTreeHost because the didBecomeInvisibleOnImplThread needs access to both threads' data structures. :'(

[James Robinson]

Comment on attachment 113137 [details]
Patch

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

[Nat Duca]

Created attachment 113228 [details]
.

[James Robinson]

Comment on attachment 113228 [details]
.

Looks great! Thanks

[Nat Duca]

Committed r99033: <http://trac.webkit.org/changeset/99033>