Bug 100923 – Free GL resources allocated by GraphicsContext3DEfl

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kalyan]

GraphicsContext3DEfl creates FBO's, textures and render buffer's, but doesn't free them.

[Kalyan]

Created attachment 171784 [details]
proposed-patch

[Kenneth Rohde Christiansen]

Comment on attachment 171784 [details]
proposed-patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171784&action=review

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:156
> +    if (m_renderStyle == RenderToCurrentGLContext || !m_private)
> +        return;
> +    makeContextCurrent();

We usually add a newline after return statements and before if/for/while etc sentenses unless the code is very relevant to it, like declaration of iterators

So newline after return here. I also think this return needs a comment ontop of the if.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:158
> +    glDeleteTextures(1, &m_texture);
> +    if (m_attrs.antialias) {

newline before if here

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:161
> +        if (m_attrs.stencil || m_attrs.depth)
> +            glDeleteRenderbuffers(1, &m_multisampleDepthStencilBuffer);

Newline before and after

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:166
> +            glDeleteRenderbuffers(1, &m_depthBuffer);

newline after this

[Kalyan]

Created attachment 171823 [details]
patch

Review Changes

[Kenneth Rohde Christiansen]

Comment on attachment 171823 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171823&action=review

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:155
> +    if (m_renderStyle == RenderToCurrentGLContext || !m_private)
> +        return;

Couldn't we have a comment here why no freeing is needed in that case?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:169
> +    } else if (m_attrs.stencil || m_attrs.depth) {
> +
> +#if USE(OPENGL_ES_2)

no newline here sorry.

Why does this have to be a separate "else if". You are chekcing with if's later down

[Kenneth Rohde Christiansen]

Comment on attachment 171823 [details]
patch

Let's just land this

[WebKit Review Bot]

Comment on attachment 171823 [details]
patch

Clearing flags on attachment: 171823

Committed r133162: <http://trac.webkit.org/changeset/133162>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.