[webkit-reviews] review denied: [Bug 76248] Initial implementation of GraphicsContext3DOpenGLES.cpp : [Attachment 122537] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 16 09:22:21 PST 2012
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?
More information about the webkit-reviews
mailing list