[webkit-reviews] review denied: [Bug 57530] [Qt] GraphicsContext3D internal buffers are not freed : [Attachment 88264] QString::fromUtf8()

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Jarkko Sakkinen
<jarkko.j.sakkinen at gmail.com>'s request for review:
Bug 57530: [Qt] GraphicsContext3D internal buffers are not freed
https://bugs.webkit.org/show_bug.cgi?id=57530

Attachment 88264: QString::fromUtf8()
https://bugs.webkit.org/attachment.cgi?id=88264&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=88264&action=review

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:455
> +    m_glWidget->makeCurrent();
> +    if (m_glWidget->isValid()) {
> +	   ::glDeleteTextures(1, &m_texture);
> +	   deleteRenderbuffers(1, &m_depthBuffer);
> +	   deleteFramebuffers(1, &m_canvasFbo);
> +    }

Please resubmit the patch with just this.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:597
> +	   void* addr =
m_glWidget->context()->getProcAddress(QString::fromUtf8(nameWithExt.utf8().data
()));

Actually, looking at it, shouldn't it be QString(nameWithExt)?
I don't see why we should pass by UTF-8 to convert for WTF::String to QString.

You should commit this separately. It seems unrelated with the resource
cleaning.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:665
> +    if ((width == m_currentWidth && height == m_currentHeight) ||
(!m_internal))

No problem with that but you can commit that separately, with the above change,
in a code cleaning patch.