[webkit-reviews] review denied: [Bug 75466] Remove style warning in GraphicsContext3DOpenGL.cpp : [Attachment 120923] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied ChangSeok Oh
<kevin.cs.oh at gmail.com>'s request for review:
Bug 75466: Remove style warning in GraphicsContext3DOpenGL.cpp
https://bugs.webkit.org/show_bug.cgi?id=75466

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

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=120923&action=review


> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:887
>      // FIXME: This is not implemented on desktop OpenGL. We need to have
ifdefs for the different GL variants

Nit: This comment is missing a period at the end of the line. Comments should
be full sentences as per the WebKit style guide.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:889
> +    // ::glReleaseShaderCompiler();

We shouldn't commit commented out code. Instead of fixing the style issue on
this line, I suggest we remove this line entirely because it's commented out
code and it's clear from the name of this method (whose name coincides with a
similarly named OpenGL call) that the implementation of this method will need
to inform OpenGL to deallocate its internal resources associated with the
shader compiler.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1276
> +	   // Let OpenGL handle these.

This comment is inane as it doesn't explain "why" we should use OpenGL handle
these cases. Either we should improve this comment or remove it.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1310
> +	   return "";

It is sufficient to call the default String() constructor here instead of the
C-string variant:

return String();

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1318
> +	       return "";

Ditto.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1321
> +	   GLchar* info = (GLchar*) fastMalloc(length);

Can we use OwnArrayPtr here? Then we can get rid of the fastFree() call on line
1326.