[webkit-reviews] review granted: [Bug 82569] [chromium] Ensure framebuffer exists at the start of beginDrawingFrame. : [Attachment 134493] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Michal Mocny
<mmocny at chromium.org>'s request for review:
Bug 82569: [chromium] Ensure framebuffer exists at the start of
beginDrawingFrame.
https://bugs.webkit.org/show_bug.cgi?id=82569

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

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


R=me. Could use some small cleanup before landing.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:168
> +    friend class LayerRendererGpuMemoryAllocationChangedCallbackAdapter;

this adapter confuses me. It adapts the call onSwapBuffersComplete() to
onSwapBuffersComplete(). Why doesn't LRC just implement
Extensions3DChromium::SwapBuffersCompleteCallbackCHROMIUM ?

>>> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:64
>>> +class FakeDrawableCCLayerImpl: public CCLayerImpl {
>> 
>> Not sure what these root layer changes are for...?
> 
> We assert that we do not have a null root layer in beginDrawingFrame.

This class appears to do nothing but expose the constructor (which is
protected). However there's a ::create() function that does exactly what you
want. Just make your test client have an OwnPtr<CCLayerImpl> and you can test
the real class instead of this somewhat silly fake

> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:71
> +    FakeLayerRendererChromiumClient() : m_setFullRootLayerDamageCount(0),
m_rootLayer(1) { }

one statement per line