Bug 52352

Summary:

Style cleanup for WebGLRenderingContext

Product:

WebKit

Reporter:

Zhenyao Mo <zmo>

Component:

WebGL

Assignee:

Zhenyao Mo <zmo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

enne, kbr, levin, webkit.review.bot, zmo

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	kbr: review+

Zhenyao Mo

Reported 2011-01-12 21:46:21 PST

remove unnecessary argument names in WebGLRenderingContext.h remove unnecessary whitespaces and empty lines

Attachments
Patch (23.05 KB, patch) 2011-01-24 15:19 PST, Zhenyao Mo	kbr: review+	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

David Levin

Comment 1 2011-01-14 01:42:23 PST

Here's the output from check-webkit-style on that previous patch: Source/WebCore/html/canvas/WebGLBuffer.h:44: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLBuffer.h:47: The parameter name "array" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLBuffer.h:48: The parameter name "array" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLBuffer.h:98: The parameter name "array" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLBuffer.h:100: The parameter name "array" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLUniformLocation.h:42: The parameter name "program" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLUniformLocation.h:49: The parameter name "program" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:71: The parameter name "ec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:73: The parameter name "ec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:74: The parameter name "ec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:75: The parameter name "ec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:76: The parameter name "ec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:77: The parameter name "ec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:84: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:133: The parameter name "ec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:134: The parameter name "ec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:151: The parameter name "ec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:198: The parameter name "image" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:200: The parameter name "canvas" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:202: The parameter name "video" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:213: The parameter name "image" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:215: The parameter name "canvas" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:217: The parameter name "video" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:219: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:221: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:221: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:222: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:224: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:224: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:225: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:227: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:227: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:228: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:230: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:230: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:231: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:233: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:233: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:234: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:236: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:236: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:237: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:239: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:239: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:240: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:242: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:242: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:243: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:244: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:244: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:245: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:246: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:246: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:247: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:248: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:248: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:255: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:258: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:261: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:264: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:471: The parameter name "image" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:564: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:565: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:566: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:566: The parameter name "v" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:566: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:567: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:568: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:568: The parameter name "v" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:568: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:577: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] If you felt it would be faster, you could probably hack up check-webkit-style for your our purposes to remove these names and write out the files again (but that may be more trouble than it is worth).

Kenneth Russell

Comment 2 2011-01-14 13:24:14 PST

In the OpenGL API (which WebGLRenderingContext and GraphicsContext3D mirror), the argument names provide a significant amount of information, and I think that removing them simply to satisfy a style guide is the wrong thing to do. I think it would be better to exempt these files' headers from this particular portion of check-webkit-style. The other style checks are still valuable for these files, though.

David Levin

Comment 3 2011-01-14 13:33:12 PST

(In reply to comment #2) > In the OpenGL API (which WebGLRenderingContext and GraphicsContext3D mirror), the argument names provide a significant amount of information, and I think that removing them simply to satisfy a style guide is the wrong thing to do. These are only parameter names that do not provide information (typically b/c they repeat the type name). If any of the items flagged provides information, then it should be left.

Zhenyao Mo

Comment 4 2011-01-24 15:19:20 PST

Created attachment 79982 [details] Patch

WebKit Review Bot

Comment 5 2011-01-24 15:22:29 PST

Attachment 79982 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/html/canvas/WebGLRenderingContext.h:568: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:570: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/canvas/WebGLRenderingContext.h:579: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Zhenyao Mo

Comment 6 2011-01-24 15:23:52 PST

This patch didn't touch any GL function signatures, because as kbr pointed out, they provide significant amount of information. However, I try to remove all the unnecessary parameter names in the help functions. Note that I didn't remove the "size" because in the same function, there are more than one GC3Dsizei-typed parameters. I am thinking maybe we should update the style-check rules to be more sophisticated: if a parameter name is part of the type name, but that type name appears more than once, then we should add a parameter name.

Kenneth Russell

Comment 7 2011-01-24 15:53:00 PST

Comment on attachment 79982 [details] Patch Looks like a reasonable incremental improvement. We should still consider whether we should exempt these files and GraphicsContext3D in the style checker from this particular style rule. Please coordinate with Chris Marrin on the landing of his patch for https://bugs.webkit.org/show_bug.cgi?id=53041 , since there may be conflicts in several files.

Zhenyao Mo

Comment 8 2011-01-24 16:06:03 PST

(In reply to comment #7) > (From update of attachment 79982 [details]) > Looks like a reasonable incremental improvement. We should still consider whether we should exempt these files and GraphicsContext3D in the style checker from this particular style rule. Please coordinate with Chris Marrin on the landing of his patch for https://bugs.webkit.org/show_bug.cgi?id=53041 , since there may be conflicts in several files. OK I'll wait until Chris lands since my patch is trivia.

Zhenyao Mo

Comment 9 2011-01-25 09:47:56 PST

Committed r76603: <http://trac.webkit.org/changeset/76603>

David Levin

Comment 10 2011-01-31 22:24:02 PST

(In reply to comment #6) > This patch didn't touch any GL function signatures, because as kbr pointed out, they provide significant amount of information. However, I try to remove all the unnecessary parameter names in the help functions. It looks like you got many of the important ones. Thanks! > Note that I didn't remove the "size" because in the same function, there are more than one GC3Dsizei-typed parameters. I am thinking maybe we should update the style-check rules to be more sophisticated: if a parameter name is part of the type name, but that type name appears more than once, then we should add a parameter name. Interesting idea. Want to try it? :) (it is pretty easy to modify the python code.)

Zhenyao Mo

Comment 11 2011-02-01 08:42:05 PST

(In reply to comment #10) > (In reply to comment #6) > > This patch didn't touch any GL function signatures, because as kbr pointed out, they provide significant amount of information. However, I try to remove all the unnecessary parameter names in the help functions. > > It looks like you got many of the important ones. Thanks! > > > Note that I didn't remove the "size" because in the same function, there are more than one GC3Dsizei-typed parameters. I am thinking maybe we should update the style-check rules to be more sophisticated: if a parameter name is part of the type name, but that type name appears more than once, then we should add a parameter name. > > Interesting idea. Want to try it? :) (it is pretty easy to modify the python code.) Actually I would suggest one more thing: if a parameter name is a single character, then maybe we shouldn't use the rule whether it is part of the type name. For example, size_t s, s is part of size_t, but there is no strong semantic connection. Yeah I could give it a try, only it has to wait until I get some more urgent GPU-related stuff done. I'll ping you for tips when I am ready to go.

Note You need to log in before you can comment on or make changes to this bug.