Bug 75466 - Remove style warning in GraphicsContext3DOpenGL.cpp
Summary: Remove style warning in GraphicsContext3DOpenGL.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 75462
  Show dependency treegraph
 
Reported: 2012-01-03 02:25 PST by ChangSeok Oh
Modified: 2012-01-07 21:31 PST (History)
3 users (show)

See Also:


Attachments
Patch (5.28 KB, patch)
2012-01-03 02:44 PST, ChangSeok Oh
dbates: review-
dbates: commit-queue-
Details | Formatted Diff | Diff
patch (6.99 KB, patch)
2012-01-03 23:57 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ChangSeok Oh 2012-01-03 02:25:23 PST
This is trivial bug to remove webkit style warnings in GraphicsContext3DOpenGL.cpp
Comment 1 ChangSeok Oh 2012-01-03 02:44:29 PST
Created attachment 120923 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-01-03 17:42:45 PST
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.
Comment 3 Daniel Bates 2012-01-03 18:20:35 PST
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.
Comment 4 ChangSeok Oh 2012-01-03 23:57:02 PST
Created attachment 121076 [details]
patch
Comment 5 ChangSeok Oh 2012-01-04 00:03:32 PST
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.
Comment 6 Kenneth Russell 2012-01-05 10:27:13 PST
Comment on attachment 121076 [details]
patch

Looks correct to me.
Comment 7 WebKit Review Bot 2012-01-05 12:02:35 PST
Comment on attachment 121076 [details]
patch

Clearing flags on attachment: 121076

Committed r104192: <http://trac.webkit.org/changeset/104192>
Comment 8 WebKit Review Bot 2012-01-05 12:02:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Daniel Bates 2012-01-07 18:18:49 PST
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.
Comment 10 Daniel Bates 2012-01-07 21:31:31 PST
(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>.