Bug 52352 – Style cleanup for WebGLRenderingContext

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

[Zhenyao Mo]

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

[David Levin]

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]

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]

(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]

Created attachment 79982 [details]
Patch

[WebKit Review Bot]

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]

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

(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]

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

[David Levin]

(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]

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