[chromium] Ensure framebuffer exists at the start of beginDrawingFrame.
Created attachment 134491 [details] Patch
Comment on attachment 134491 [details] Patch If we ensure that a framebuffer exists before beginning to draw a frame, we need not have clients explicitly manage it, and we can be sure it is done on the right thread. This is also how it will work once it recreates itself of first use, so it is good practice. We need not discard a recreated framebuffer, because GpuMemoryManager will send another allocation suggesting to discard it.
Comment on attachment 134491 [details] Patch LGTM
Comment on attachment 134491 [details] Patch one sec, patch to remove public methods as they are no longer needed inc
Created attachment 134493 [details] Patch
Comment on attachment 134493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134493&action=review > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:64 > +class FakeDrawableCCLayerImpl: public CCLayerImpl { Not sure what these root layer changes are for...? > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:196 > + m_layerRendererChromium.beginDrawingFrame(); Where's the expectation that we called ensure?
Comment on attachment 134493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134493&action=review >> 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. >> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:196 >> + m_layerRendererChromium.beginDrawingFrame(); > > Where's the expectation that we called ensure? Right below, before this line we EXPECT that isFramebufferDiscarded returns true, after this line it returns false.
Comment on attachment 134493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134493&action=review Got it. Makes sense. Still lgtm. >>> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:196 >>> + m_layerRendererChromium.beginDrawingFrame(); >> >> Where's the expectation that we called ensure? > > Right below, before this line we EXPECT that isFramebufferDiscarded returns true, after this line it returns false. So you might want to put before this line expect no framebuffer
Comment on attachment 134493 [details] Patch 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
Comment on attachment 134493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134493&action=review >> 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 ? I assume you meant to type onMemoryAllocationChanged, and the main reason was to not need to expose extensions3dchromium in the header, and was copying the style of onSwap... I can make the change if that is preferred.
Right, the other one. I think it's fine for LayerRendererChromium to know about Extensions, the point of LRC is to implement GraphicsContext3D-based functionality.
I think also that the E3D callback model takes ownership of the callback ptr. We dont want G3CD deleting the lrc :)
Created attachment 134639 [details] Patch
Comment on attachment 134639 [details] Patch Clearing flags on attachment: 134639 Committed r112568: <http://trac.webkit.org/changeset/112568>
All reviewed patches have been landed. Closing bug.