Bug 71269 - [chromium] Move resource-releasing logic into CCProxy and remove setNeedsCommit(without redraw)
Summary: [chromium] Move resource-releasing logic into CCProxy and remove setNeedsComm...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nat Duca
URL:
Keywords:
Depends on:
Blocks: 71267
  Show dependency treegraph
 
Reported: 2011-11-01 00:31 PDT by Nat Duca
Modified: 2011-11-01 19:18 PDT (History)
3 users (show)

See Also:


Attachments
Patch (11.25 KB, patch)
2011-11-01 00:31 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
. (19.59 KB, patch)
2011-11-01 14:58 PDT, Nat Duca
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nat Duca 2011-11-01 00:31:08 PDT
[chromium] Move resource-releasing logic into CCProxy and remove setNeedsCommit(without redraw)
Comment 1 Nat Duca 2011-11-01 00:31:29 PDT
Created attachment 113137 [details]
Patch
Comment 2 Nat Duca 2011-11-01 00:38:13 PDT
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. :'(
Comment 3 James Robinson 2011-11-01 14:00:11 PDT
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
Comment 4 Nat Duca 2011-11-01 14:58:31 PDT
Created attachment 113228 [details]
.
Comment 5 James Robinson 2011-11-01 15:18:52 PDT
Comment on attachment 113228 [details]
.

Looks great! Thanks
Comment 6 Nat Duca 2011-11-01 19:18:42 PDT
Committed r99033: <http://trac.webkit.org/changeset/99033>