[webkit-reviews] review denied: [Bug 91177] [chromium] Add 'self-managed' option to CCPrioritizedTexture to enable render-surface and canvas use cases. : [Attachment 152310] Patch

[bugzilla-daemon at webkit.org]

Adrienne Walker <enne at google.com> has denied Eric Penner
<epenner at chromium.org>'s request for review:
Bug 91177: [chromium] Add 'self-managed' option to CCPrioritizedTexture to
enable render-surface and canvas use cases.
https://bugs.webkit.org/show_bug.cgi?id=91177

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

------- Additional Comments from Adrienne Walker <enne at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=152310&action=review


Can you add asserts about things with backings not being self-managed textures,
e.g.
CCPrioritizedTextureManager::returnBacking/createBacking/destroyBacking/assertI
nvariants?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:500
> -void CCLayerTreeHost::prioritizeTextures(const LayerList& updateList)
> +void CCLayerTreeHost::prioritizeTextures(const LayerList& updateList,
size_t& memoryForRenderSurfaces)

This feels overloaded.	Can you split this into two functions? e.g. "size_t
calculateMemoryForRenderSurfaces()" and "void prioritizeTextures(const
LayerList&)" and maybe have paintContents add the managed textures?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:545
> +    // Create memory place-holder for surfaces (which are managed
independantly) but should
> +    // reduce memory available for contents.
> +    OwnPtr<CCPrioritizedTexture> surfacePlaceholder =
m_contentsTextureManager->createMemoryPlaceholder(memoryForRenderSurfaces);
> +   
surfacePlaceholder->setRequestPriority(CCPriorityCalculator::renderSurfacePrior
ity());

...and then maybe move this code into paintLayerContents. It's a little sketchy
to me that the surfacePlaceholder has such a transient lifetime; should it be
owned by the CCLTH?  If you create a self-managed texture, shouldn't the memory
available immediately go down and then go back up if and when it gets deleted?

> Source/WebKit/chromium/tests/CCPrioritizedTextureTest.cpp:324
> +

Boo random whitespace.