Bug 75466 – Remove style warning in GraphicsContext3DOpenGL.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[ChangSeok Oh]

This is trivial bug to remove webkit style warnings in GraphicsContext3DOpenGL.cpp

[ChangSeok Oh]

Created attachment 120923 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 120923 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120923&action=review

style-only changes are kinda looked down upon.  BUt there is some minor cleanup you could do here at the same time which seems worth doing.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1291
> +        (*value) = getShaderInfoLog(shader).length();

Why the () around *value?

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1315
> +    if (entry.isValid) {
> +        GLint length;

If you're going to re-indent this, you might as well make it use early-return instead.

[Daniel Bates]

Comment on attachment 120923 [details]
Patch

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.

[ChangSeok Oh]

Created attachment 121076 [details]
patch

[ChangSeok Oh]

Comment on attachment 120923 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120923&action=review

Thanks for 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.

Done.

>> 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.

Removed glReleaseShaderCompiler() and added notImplemented() newly.

>> 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.

Removed.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1291
>> +        (*value) = getShaderInfoLog(shader).length();
> 
> Why the () around *value?

Removed ().

>> 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();

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1315
>> +        GLint length;
> 
> If you're going to re-indent this, you might as well make it use early-return instead.

Done.

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

Done.

>> 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.

It looks no problem. So I did.

[Kenneth Russell]

Comment on attachment 121076 [details]
patch

Looks correct to me.

[WebKit Review Bot]

Comment on attachment 121076 [details]
patch

Clearing flags on attachment: 121076

Committed r104192: <http://trac.webkit.org/changeset/104192>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

Comment on attachment 121076 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121076&action=review

I know this was already reviewed and landed, but I noticed a correctness issue.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1251
> +    OwnArrayPtr<GLchar> info = adoptArrayPtr(static_cast<GLchar*>(fastMalloc(length)));

This is incorrect when GLOBAL_FASTMALLOC_NEW is disabled. We should be using operator new[] instead of fastMalloc() here since OwnArrayPtr ultimately uses operator delete[] on destruction.

[Daniel Bates]

(In reply to comment #9)
> [...]
> > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1251
> > +    OwnArrayPtr<GLchar> info = adoptArrayPtr(static_cast<GLchar*>(fastMalloc(length)));
> 
> This is incorrect when GLOBAL_FASTMALLOC_NEW is disabled. We should be using operator new[] instead of fastMalloc() here since OwnArrayPtr ultimately uses operator delete[] on destruction.

Fixed in <http://trac.webkit.org/changeset/104395>.