| Summary: | Remove style warning in GraphicsContext3DOpenGL.cpp | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | ChangSeok Oh <kevin.cs.oh> | ||||||
| Component: | WebGL | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | dbates, kbr, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 75462 | ||||||||
| Attachments: |
|
||||||||
|
Description
ChangSeok Oh
2012-01-03 02:25:23 PST
Created attachment 120923 [details]
Patch
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 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. Created attachment 121076 [details]
patch
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 on attachment 121076 [details]
patch
Looks correct to me.
Comment on attachment 121076 [details] patch Clearing flags on attachment: 121076 Committed r104192: <http://trac.webkit.org/changeset/104192> All reviewed patches have been landed. Closing bug. 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. (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>. |