This is part of https://bugs.webkit.org/show_bug.cgi?id=47379. Move creation of DrawingBuffer to GraphicsContext3D, keep a list of DrawingBuffers and manage their lifetime to avoid dangling pointers. DrawingBuffer for Mac is still not completely implemented and GraphicsContext3D still creates its own "drawing buffer" for use by WebGL.
Created attachment 70462 [details] Patch
Attachment 70462 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4324027
Created attachment 70464 [details] Patch fixing Chromium build
Attachment 70464 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4355019
Created attachment 70466 [details] Patch with more Chromium fixes and added m_context null checks to DrawingBuffer functions
Attachment 70466 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4343025
Created attachment 70468 [details] Patch
Created attachment 70469 [details] Patch adding removeDrawingBuffer calls and fixing more Chromium build problems
Created attachment 70473 [details] Patch adding protection against null m_context in DrawingBuffer ctors
Attachment 70469 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4369015
Attachment 70473 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4336025
Created attachment 70480 [details] Patch fixing another Chromium build issue
Created attachment 70483 [details] Patch fixing merge conflicts
Attachment 70483 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4304034
Created attachment 70537 [details] Patch
Attachment 70537 [details] did not build on mac: Build output: http://queues.webkit.org/results/4286037
Attachment 70537 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4289034
Created attachment 70542 [details] Patch gets rid of const on platformLayer() so Chromium builds
Created attachment 70571 [details] Patch using ref counting rather than weak pointers (please hold review)
Comment on attachment 70571 [details] Patch using ref counting rather than weak pointers (please hold review) View in context: https://bugs.webkit.org/attachment.cgi?id=70571&action=review > WebCore/platform/graphics/GraphicsContext3D.h:442 > + static PassRefPtr<GraphicsContext3D> create(Attributes attrs, HostWindow* hostWindow, RenderStyle renderStyle = RenderOffscreen); The argument names here are not helpful and should be omitted. > WebCore/platform/graphics/gpu/DrawingBuffer.cpp:42 > + return (drawingBuffer->m_context) ? drawingBuffer : 0; Should be drawingBuffer.release() rather than drawingBuffer. > WebCore/platform/graphics/gpu/DrawingBuffer.cpp:55 > + m_context = 0; Should use clear() to clear RefPtr rather than assignment to 0. > WebCore/platform/graphics/gpu/DrawingBuffer.h:65 > + // Clear all resources from this object, as well as context. Called when context is destroyed > + // to prevent invalid accesses to the resources. > + void clear(); The first line of the comment is indented incorrectly. > WebCore/platform/graphics/gpu/DrawingBuffer.h:88 > +protected: > + static PassRefPtr<DrawingBuffer> create(PassRefPtr<GraphicsContext3D>, const IntSize&); Why protected rather than private? > WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:64 > - return adoptRef(new SharedGraphicsContext3D(context.release(), solidFillShader.release(), texShader.release())); > + return adoptRef(new SharedGraphicsContext3D(context, solidFillShader.release(), texShader.release())); This need not be changed: A call to release() here is still a good idea. It will avoid a bit of reference count churn. > WebCore/platform/graphics/gpu/mac/DrawingBufferMac.mm:63 > + if (!m_context) > + return; > + > + clear(); Since the clear() function is already safe to call if m_context is 0, I suggest omitting the if statement and early return here. > WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:86 > - OwnPtr<GraphicsContext3D> context(new GraphicsContext3D(attrs, hostWindow, false)); > - return context->m_contextObj ? context.release() : 0; > + RefPtr<GraphicsContext3D> context = adoptRef(new GraphicsContext3D(attrs, hostWindow, false)); > + return context->m_contextObj ? context : 0; No reason to remove the release() call here. Including it reduces the reference count churn a bit.
(In reply to comment #20) > ... > > WebCore/platform/graphics/gpu/DrawingBuffer.h:88 > > +protected: > > + static PassRefPtr<DrawingBuffer> create(PassRefPtr<GraphicsContext3D>, const IntSize&); > > Why protected rather than private? This create method is used by GraphicsContext3D, which is a friend, so it has to be protected.
(In reply to comment #21) > (In reply to comment #20) > > ... > > > WebCore/platform/graphics/gpu/DrawingBuffer.h:88 > > > +protected: > > > + static PassRefPtr<DrawingBuffer> create(PassRefPtr<GraphicsContext3D>, const IntSize&); > > > > Why protected rather than private? > > This create method is used by GraphicsContext3D, which is a friend, so it has to be protected. I realize now that friends can access private members. So I will make this change. I'm not sure where I got the outdated notion that friends could only access protected members.
Landed in http://trac.webkit.org/changeset/69619
http://trac.webkit.org/changeset/69619 might have broken Chromium Win Release
Attachment 70571 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4392002