[webkit-reviews] review denied: [Bug 30349] WebGL LayoutTests fail on buildbot : [Attachment 41960] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Oct 27 10:32:19 PDT 2009
Simon Fraser (smfr) <simon.fraser at apple.com> has denied Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 30349: WebGL LayoutTests fail on buildbot
https://bugs.webkit.org/show_bug.cgi?id=30349
Attachment 41960: Patch
https://bugs.webkit.org/attachment.cgi?id=41960&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> + (WebCore::CanvasRenderingContext3D::create):
> + (WebCore::CanvasRenderingContext3D::CanvasRenderingContext3D):
> + (WebCore::CanvasRenderingContext3D::beginPaint):
> + (WebCore::CanvasRenderingContext3D::endPaint):
> + (WebCore::CanvasRenderingContext3D::reshape):
> + (WebCore::CanvasRenderingContext3D::sizeInBytes):
> + (WebCore::CanvasRenderingContext3D::activeTexture):
> + (WebCore::CanvasRenderingContext3D::attachShader):
> + (WebCore::CanvasRenderingContext3D::bindAttribLocation):
> + (WebCore::CanvasRenderingContext3D::bindBuffer):
> + (WebCore::CanvasRenderingContext3D::bindFramebuffer):
> + (WebCore::CanvasRenderingContext3D::bindRenderbuffer):
> + (WebCore::CanvasRenderingContext3D::bindTexture):
> + (WebCore::CanvasRenderingContext3D::blendColor):
> + (WebCore::CanvasRenderingContext3D::blendEquation):
> + (WebCore::CanvasRenderingContext3D::blendEquationSeparate):
> + (WebCore::CanvasRenderingContext3D::blendFunc):
> + (WebCore::CanvasRenderingContext3D::blendFuncSeparate):
> + (WebCore::CanvasRenderingContext3D::bufferData):
> + (WebCore::CanvasRenderingContext3D::bufferSubData):
> + (WebCore::CanvasRenderingContext3D::checkFramebufferStatus):
> + (WebCore::CanvasRenderingContext3D::clear):
> + (WebCore::CanvasRenderingContext3D::clearColor):
> + (WebCore::CanvasRenderingContext3D::clearDepth):
> + (WebCore::CanvasRenderingContext3D::clearStencil):
> + (WebCore::CanvasRenderingContext3D::colorMask):
> + (WebCore::CanvasRenderingContext3D::compileShader):
> + (WebCore::CanvasRenderingContext3D::copyTexImage2D):
> + (WebCore::CanvasRenderingContext3D::copyTexSubImage2D):
> + (WebCore::CanvasRenderingContext3D::cullFace):
> + (WebCore::CanvasRenderingContext3D::depthFunc):
> + (WebCore::CanvasRenderingContext3D::depthMask):
> + (WebCore::CanvasRenderingContext3D::depthRange):
> + (WebCore::CanvasRenderingContext3D::detachShader):
> + (WebCore::CanvasRenderingContext3D::disable):
> + (WebCore::CanvasRenderingContext3D::disableVertexAttribArray):
> + (WebCore::CanvasRenderingContext3D::drawArrays):
> + (WebCore::CanvasRenderingContext3D::drawElements):
> + (WebCore::CanvasRenderingContext3D::enable):
> + (WebCore::CanvasRenderingContext3D::enableVertexAttribArray):
> + (WebCore::CanvasRenderingContext3D::finish):
> + (WebCore::CanvasRenderingContext3D::flush):
> + (WebCore::CanvasRenderingContext3D::framebufferRenderbuffer):
> + (WebCore::CanvasRenderingContext3D::framebufferTexture2D):
> + (WebCore::CanvasRenderingContext3D::frontFace):
> + (WebCore::CanvasRenderingContext3D::generateMipmap):
> + (WebCore::CanvasRenderingContext3D::getActiveAttrib):
> + (WebCore::CanvasRenderingContext3D::getActiveUniform):
> + (WebCore::CanvasRenderingContext3D::getAttribLocation):
> + (WebCore::CanvasRenderingContext3D::getBoolean):
> + (WebCore::CanvasRenderingContext3D::getBooleanv):
> + (WebCore::CanvasRenderingContext3D::getBufferParameteri):
> + (WebCore::CanvasRenderingContext3D::getBufferParameteriv):
> + (WebCore::CanvasRenderingContext3D::getError):
> + (WebCore::CanvasRenderingContext3D::getFloat):
> + (WebCore::CanvasRenderingContext3D::getFloatv):
> +
(WebCore::CanvasRenderingContext3D::getFramebufferAttachmentParameteri):
> +
(WebCore::CanvasRenderingContext3D::getFramebufferAttachmentParameteriv):
> + (WebCore::CanvasRenderingContext3D::getInteger):
> + (WebCore::CanvasRenderingContext3D::getIntegerv):
> + (WebCore::CanvasRenderingContext3D::getProgrami):
> + (WebCore::CanvasRenderingContext3D::getProgramiv):
> + (WebCore::CanvasRenderingContext3D::getProgramInfoLog):
> + (WebCore::CanvasRenderingContext3D::getRenderbufferParameteri):
> + (WebCore::CanvasRenderingContext3D::getRenderbufferParameteriv):
> + (WebCore::CanvasRenderingContext3D::getShaderi):
> + (WebCore::CanvasRenderingContext3D::getShaderiv):
> + (WebCore::CanvasRenderingContext3D::getShaderInfoLog):
> + (WebCore::CanvasRenderingContext3D::getShaderSource):
> + (WebCore::CanvasRenderingContext3D::getString):
> + (WebCore::CanvasRenderingContext3D::getTexParameterf):
> + (WebCore::CanvasRenderingContext3D::getTexParameterfv):
> + (WebCore::CanvasRenderingContext3D::getTexParameteri):
> + (WebCore::CanvasRenderingContext3D::getTexParameteriv):
> + (WebCore::CanvasRenderingContext3D::getUniformf):
> + (WebCore::CanvasRenderingContext3D::getUniformfv):
> + (WebCore::CanvasRenderingContext3D::getUniformi):
> + (WebCore::CanvasRenderingContext3D::getUniformiv):
> + (WebCore::CanvasRenderingContext3D::getUniformLocation):
> + (WebCore::CanvasRenderingContext3D::getVertexAttribf):
> + (WebCore::CanvasRenderingContext3D::getVertexAttribfv):
> + (WebCore::CanvasRenderingContext3D::getVertexAttribi):
> + (WebCore::CanvasRenderingContext3D::getVertexAttribiv):
> + (WebCore::CanvasRenderingContext3D::getVertexAttribOffset):
> + (WebCore::CanvasRenderingContext3D::hint):
> + (WebCore::CanvasRenderingContext3D::isBuffer):
> + (WebCore::CanvasRenderingContext3D::isEnabled):
> + (WebCore::CanvasRenderingContext3D::isFramebuffer):
> + (WebCore::CanvasRenderingContext3D::isProgram):
> + (WebCore::CanvasRenderingContext3D::isRenderbuffer):
> + (WebCore::CanvasRenderingContext3D::isShader):
> + (WebCore::CanvasRenderingContext3D::isTexture):
> + (WebCore::CanvasRenderingContext3D::lineWidth):
> + (WebCore::CanvasRenderingContext3D::linkProgram):
> + (WebCore::CanvasRenderingContext3D::pixelStorei):
> + (WebCore::CanvasRenderingContext3D::polygonOffset):
> + (WebCore::CanvasRenderingContext3D::readPixels):
> + (WebCore::CanvasRenderingContext3D::releaseShaderCompiler):
> + (WebCore::CanvasRenderingContext3D::renderbufferStorage):
> + (WebCore::CanvasRenderingContext3D::sampleCoverage):
> + (WebCore::CanvasRenderingContext3D::scissor):
> + (WebCore::CanvasRenderingContext3D::shaderSource):
> + (WebCore::CanvasRenderingContext3D::stencilFunc):
> + (WebCore::CanvasRenderingContext3D::stencilFuncSeparate):
> + (WebCore::CanvasRenderingContext3D::stencilMask):
> + (WebCore::CanvasRenderingContext3D::stencilMaskSeparate):
> + (WebCore::CanvasRenderingContext3D::stencilOp):
> + (WebCore::CanvasRenderingContext3D::stencilOpSeparate):
> + (WebCore::CanvasRenderingContext3D::texImage2D):
> + (WebCore::CanvasRenderingContext3D::texParameterf):
> + (WebCore::CanvasRenderingContext3D::texParameteri):
> + (WebCore::CanvasRenderingContext3D::texSubImage2D):
> + (WebCore::CanvasRenderingContext3D::uniform1f):
> + (WebCore::CanvasRenderingContext3D::uniform1fv):
> + (WebCore::CanvasRenderingContext3D::uniform1i):
> + (WebCore::CanvasRenderingContext3D::uniform1iv):
> + (WebCore::CanvasRenderingContext3D::uniform2f):
> + (WebCore::CanvasRenderingContext3D::uniform2fv):
> + (WebCore::CanvasRenderingContext3D::uniform2i):
> + (WebCore::CanvasRenderingContext3D::uniform2iv):
> + (WebCore::CanvasRenderingContext3D::uniform3f):
> + (WebCore::CanvasRenderingContext3D::uniform3fv):
> + (WebCore::CanvasRenderingContext3D::uniform3i):
> + (WebCore::CanvasRenderingContext3D::uniform3iv):
> + (WebCore::CanvasRenderingContext3D::uniform4f):
> + (WebCore::CanvasRenderingContext3D::uniform4fv):
> + (WebCore::CanvasRenderingContext3D::uniform4i):
> + (WebCore::CanvasRenderingContext3D::uniform4iv):
> + (WebCore::CanvasRenderingContext3D::uniformMatrix2fv):
> + (WebCore::CanvasRenderingContext3D::uniformMatrix3fv):
> + (WebCore::CanvasRenderingContext3D::uniformMatrix4fv):
> + (WebCore::CanvasRenderingContext3D::useProgram):
> + (WebCore::CanvasRenderingContext3D::validateProgram):
> + (WebCore::CanvasRenderingContext3D::vertexAttrib1f):
> + (WebCore::CanvasRenderingContext3D::vertexAttrib1fv):
> + (WebCore::CanvasRenderingContext3D::vertexAttrib2f):
> + (WebCore::CanvasRenderingContext3D::vertexAttrib2fv):
> + (WebCore::CanvasRenderingContext3D::vertexAttrib3f):
> + (WebCore::CanvasRenderingContext3D::vertexAttrib3fv):
> + (WebCore::CanvasRenderingContext3D::vertexAttrib4f):
> + (WebCore::CanvasRenderingContext3D::vertexAttrib4fv):
> + (WebCore::CanvasRenderingContext3D::vertexAttribPointer):
> + (WebCore::CanvasRenderingContext3D::viewport):
No need for this huge list.
> Index: WebCore/html/canvas/CanvasRenderingContext3D.cpp
> ===================================================================
> +CanvasRenderingContext3D::CanvasRenderingContext3D(HTMLCanvasElement*
canvas, GraphicsContext3D* context)
> : CanvasRenderingContext(canvas)
> + , m_context(context)
> , m_needsUpdate(true)
> , m_markedCanvasDirty(false)
> {
I think an ASSERT(context) would make the contract clearer.
> Index: WebCore/html/canvas/CanvasRenderingContext3D.h
> ===================================================================
>
> - GraphicsContext3D* graphicsContext3D() { return &m_context; }
> + GraphicsContext3D* graphicsContext3D() { return m_context.get(); }
As I mentioned in previous reviews, this getter should be "const".
> Index: WebCore/platform/graphics/mac/Canvas3DLayer.mm
> ===================================================================
> --- WebCore/platform/graphics/mac/Canvas3DLayer.mm (revision 50115)
> +++ WebCore/platform/graphics/mac/Canvas3DLayer.mm (working copy)
> @@ -50,6 +50,8 @@ using namespace WebCore;
>
> -(CGLPixelFormatObj)copyCGLPixelFormatForDisplayMask:(uint32_t)mask
> {
> + UNUSED_PARAM(mask);
> + /*
> CGLPixelFormatAttribute attribs[] =
> {
> (CGLPixelFormatAttribute) kCGLPFAAccelerated,
> @@ -63,6 +65,9 @@ using namespace WebCore;
>
> CGLChoosePixelFormat(attribs, &pixelFormatObj, &numPixelFormats);
> return pixelFormatObj;
> + */
Why are you commenting out this code? If it shouldn't be there, remove it.
> Index: WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp
> ===================================================================
> +static void setPixelFormat(CGLPixelFormatAttribute* attribs, int colorBits,
int depthBits,
> + bool accelerated, bool supersample, bool closest)
> +{
> + *attribs++ = kCGLPFAColorSize;
> + *attribs++ = static_cast<CGLPixelFormatAttribute>(colorBits);
> + *attribs++ = kCGLPFADepthSize;
> + *attribs++ = static_cast<CGLPixelFormatAttribute>(depthBits);
I don't like this at all. This method just blindly assumes that the caller has
allocated enough space.
You could use a Vector<CGLPixelFormatAttribute>, which would be much cleaner.
> @@ -176,7 +200,7 @@ void GraphicsContext3D::endPaint()
>
> void GraphicsContext3D::reshape(int width, int height)
> {
> - if (width == m_currentWidth && height == m_currentHeight)
> + if (width == m_currentWidth && height == m_currentHeight ||
!m_contextObj)
> return;
How do you ever get here with m_contextObj == 0 ?
> m_currentWidth = width;
> @@ -208,6 +232,9 @@ void GraphicsContext3D::reshape(int widt
>
> static inline void ensureContext(CGLContextObj context)
> {
> + if (!context)
> + return;
> +
> CGLContextObj currentContext = CGLGetCurrentContext();
> if (currentContext != context)
> CGLSetCurrentContext(context);
r- to clean up the setPixelFormat() stuff, but in general it looks ok.
More information about the webkit-reviews
mailing list