[webkit-reviews] review granted: [Bug 47501] Manage DrawingBuffer lifetime in GraphicsContext3D : [Attachment 70571] Patch using ref counting rather than weak pointers

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Marrin <cmarrin at apple.com>'s
request for review:
Bug 47501: Manage DrawingBuffer lifetime in GraphicsContext3D
https://bugs.webkit.org/show_bug.cgi?id=47501

Attachment 70571: Patch using ref counting rather than weak pointers
https://bugs.webkit.org/attachment.cgi?id=70571&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.