[webkit-reviews] review denied: [Bug 76248] Initial implementation of GraphicsContext3DOpenGLES.cpp : [Attachment 122537] 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 122537: Patch
https://bugs.webkit.org/attachment.cgi?id=122537&action=review

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


I think this needs a bit of clean up, but it's looking pretty good so far!

> Source/WebCore/ChangeLog:13
> +	   No new tests required.

Might want to explain why there are no new tests required.

> Source/WebCore/ChangeLog:27
> +	   * platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:
> +	   (WebCore::GraphicsContext3D::readRenderingResults):
> +	   (WebCore::GraphicsContext3D::reshape):
> +	   (WebCore::GraphicsContext3D::prepareTexture):
> +	   (WebCore::GraphicsContext3D::bindFramebuffer):
> +	   (WebCore::GraphicsContext3D::copyTexImage2D):
> +	   (WebCore::GraphicsContext3D::copyTexSubImage2D):
> +	   (WebCore::GraphicsContext3D::getActiveUniform):
> +	   (WebCore::GraphicsContext3D::readPixels):
> +	   (WebCore::GraphicsContext3D::renderbufferStorage):
> +	   (WebCore::GraphicsContext3D::getIntegerv):
> +	   (WebCore::GraphicsContext3D::texImage2D):

Here it might be good to briefly explain how each method differs from it's
desktop GL counterpart.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:103
> +    // resize multisample FBO

Nit. Comments should begin with a capital letter and end with a period.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:107
> +    // resize regular FBO

Ditto.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:225
> +    // FIXME : We don't care about multisampleFBO currently.

Extra space after FIXME. Might want to open a bug about this now.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:240
> +    if (m_attrs.antialias && m_boundFBO == m_multisampleFBO)
> +	   notImplemented();
> +    ::glCopyTexImage2D(target, level, internalformat, x, y, width, height,
border);
> +    if (m_attrs.antialias && m_boundFBO == m_multisampleFBO)
> +	   notImplemented();

Why do you do this check twice? Perhaps you should return early if
multisampling is needed?

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:250
> +    if (m_attrs.antialias && m_boundFBO == m_multisampleFBO)
> +	   notImplemented();
> +    ::glCopyTexSubImage2D(target, level, xoffset, yoffset, x, y, width,
height);
> +    if (m_attrs.antialias && m_boundFBO == m_multisampleFBO)
> +	   notImplemented();

Ditto.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:281
> +    // FIXME : "[0]" is added at end of name in
WebGLRenderingContext::getActiveUniform()
> +    // when isGLES2Compliant is only false. I haven't found the reason.
> +    // So I add "[0]" here to avoid faling webgl conformance
test(get-active-test.html),
> +    // but if it makes something wrong later, remove following two lines.
> +    if (isGLES2Compliant() && info.size > 1 && !info.name.endsWith("[0]"))
> +	   info.name.append("[0]");

Extra space after the FIXME. Might want to investigate this a bit now to avoid
code churn.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:289
> +    // FIXME: remove the two glFlush calls when the driver bug is fixed,
i.e.,
> +    // all previous rendering calls should be done before reading pixels.

Certainly the driver bug wouldn't affect both OpenGLES drivers and for whatever
driver this work-around was added for. Can you just remove it?