[webkit-reviews] review denied: [Bug 81823] [chromium] LayerRendererChromium should use GpuMemoryAllocationChanged callback to explicitly manage framebuffer. : [Attachment 133152] Patch

[bugzilla-daemon at webkit.org]

Adrienne Walker <enne at google.com> has denied Michal Mocny
<mmocny at chromium.org>'s request for review:
Bug 81823: [chromium] LayerRendererChromium should use
GpuMemoryAllocationChanged callback to explicitly manage framebuffer.
https://bugs.webkit.org/show_bug.cgi?id=81823

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

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


Looks good in general.	Here's a bunch of small things to fix, and then we can
land this.  Thanks so much for the test!

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1078
> +	 m_client->setFullRootLayerDamage();

Is this redundant? It looks like you already set the full root layer damage
during discardFramebuffer.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1110
> +    if (m_capabilities.usingDiscardFramebuffer) {

style nit: early out here, rather than indenting this whole block of code.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1127
> +    if (m_capabilities.usingDiscardFramebuffer) {

style nit: early out here too.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:169
> +    friend class LayerRendererGpuMemoryAllocationChangedCallbackAdapter;
> +    friend class ScopedEnsureFramebufferAllocation;
> +    void discardFramebuffer();
> +    void ensureFramebuffer();

I think you should just make these functions public for the sake of future
design, add an isFramebufferDiscarded() function, and remove the friend class. 
At some point, we'd like to make a CCRenderer interface that
LayerRendererChromium can implement, and I think that interface would end up
using these framebuffer functions directly.

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:75
> +    ScopedEnsureFramebufferAllocation
sefa(m_layerTreeHostImpl->layerRenderer());

style nit: please don't use abbreviations.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:94
> +    ScopedEnsureFramebufferAllocation
sefa(m_layerTreeHostImpl->layerRenderer());

style nit: please don't use abbreviations.

> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:53
> +    virtual const IntSize& viewportSize() const { static IntSize sz; return
sz; }

style nit: please don't use abbreviations.

> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:64
> +    LayerRendererChromiumTest(LayerRendererChromiumClient* lrcc,
PassRefPtr<GraphicsContext3D> context)

style nit: please don't use abbreviations.

> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:83
> +    LayerRendererChromiumTest lrc(&client, context.release());

style nit: please don't use abbreviations.