[webkit-reviews] review denied: [Bug 56825] [Qt] fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html fails : [Attachment 86508] Fixes for context attribute handling.

[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 56825: [Qt]
fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html fails
https://bugs.webkit.org/show_bug.cgi?id=56825

Attachment 86508: Fixes for context attribute handling.
https://bugs.webkit.org/attachment.cgi?id=86508&action=review

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

I made general comments to get me started reviewing this :)

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:451
> +    if (!reshapeMainFbo(1, 1)) {

Why do we create the Fbo of size (1, 1) at this stage?

It seems reasonable to me to have an invalid painter state until
GraphicsContext3D::reshape() is called with a valid size. Or some
initialization of reshapeMainFbo() is needed in the constructor?

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:454
>	   LOG_ERROR("GraphicsContext3D: Wasn't able to create the main
framebuffer");
>	   m_contextValid = false;
>      }

Shouldn't this code be part of reshapeMainFbo()?
Logging the error and setting m_contextValid = false; seems reasonable to do in
the function if the allocation failed.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:482
> +    m_pixels = QImage(width, height, QImage::Format_ARGB32);

I got rid of m_pixels yesterday :)

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:494
> +    bindRenderbuffer(GraphicsContext3D::RENDERBUFFER, m_depthBuffer);

if !(m_attrs.depth) || QT_OPENGL_ES_2, genRenderbuffers() is not called. So I
guess we should not bind the buffer either(?).