[webkit-reviews] review denied: [Bug 76248] Initial implementation of GraphicsContext3DOpenGLES.cpp : [Attachment 124068] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied ChangSeok Oh
<kevin.cs.oh at gmail.com>'s request for review:
Bug 76248: Initial implementation of GraphicsContext3DOpenGLES.cpp
https://bugs.webkit.org/show_bug.cgi?id=76248

Attachment 124068: Patch
https://bugs.webkit.org/attachment.cgi?id=124068&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=124068&action=review


Looking good. I've listed a few small things below.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:902
> +    void resolveMultisamplingIfNecessary(GC3Dint x, GC3Dint y, GC3Dsizei
width, GC3Dsizei height);

Better to pass an IntRect here.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:46
> +bool GraphicsContext3D::resizeFBOs(int width, int height)

Better to pass an const IntSize& here. reshapeFBOs might be a better name to
match the caller.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:74
> +	   bool supportPackedDepthStencilBuffer = false;
> +	   if (isGLES2Compliant())
> +	       supportPackedDepthStencilBuffer =
extensions->supports("GL_OES_packed_depth_stencil");
> +	   else
> +	       supportPackedDepthStencilBuffer =
extensions->supports("GL_EXT_packed_depth_stencil");
> +
> +	   if (supportPackedDepthStencilBuffer) {
> +	       if (isGLES2Compliant())
> +		   extensions->ensureEnabled("GL_OES_packed_depth_stencil");
> +	       else
> +		   extensions->ensureEnabled("GL_EXT_packed_depth_stencil");

Here it might be better to do something like this:

const const char* packedDepthStencilExtension = isGLES2Compliant() ?
"GL_OES_packed_depth_stencil" : "GL_EXT_packed_depth_stencil";
if (extensions->supports(packedDepthStencilExtension)) {
    extensions->ensureEnabled(packedDepthStencilExtension);

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:160

> +    if (m_attrs.antialias)
> +	   resolveMultisamplingIfNecessary(0, 0, m_currentWidth,
m_currentHeight);

It seems like this should be checking whether m_boundFBO == m_multisampleFBO,
but we need to ask the original author to answer that question, I think. Later
it might be good to suck more of the logic into
resolveMultisamplingIfNecessary. Perhaps it would be better as bool
resolveMultisamplingIfNecessaryAndBindSingleSampleFramebuffer() or something
like that. :)

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:204

> +	       std::swap(pixels[i], pixels[i + 2]); // Convert to BGRA

Nit: Missing a period at the end of this comment.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:59
> +    bool supportPackedDepthStencilBuffer = false;
> +    if (m_attrs.stencil || m_attrs.depth) {
> +	   // We don't allow the logic where stencil is required and depth is
not.
> +	   // See GraphicsContext3D::validateAttributes.
> +	   Extensions3D* extensions = getExtensions();
> +	   supportPackedDepthStencilBuffer =
extensions->supports("GL_OES_packed_depth_stencil");

This could simply be:

// We don't allow the logic where stencil is required and depth is not.
// See GraphicsContext3D::validateAttributes.
supportsPackedDepthStencilBuffer = (m_attrs.stencil || m_attrs.depth) &&
getExtensions()->->supports("GL_OES_packed_depth_stencil");

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:77
> +    if (!m_attrs.antialias && (m_attrs.stencil || m_attrs.depth)) {

Better to ASSERT(!m_attrs.antialias) instead of checking it when you know it's
always false.