Bug 52352 - Style cleanup for WebGLRenderingContext
Summary: Style cleanup for WebGLRenderingContext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-12 21:46 PST by Zhenyao Mo
Modified: 2011-02-01 08:42 PST (History)
5 users (show)

See Also:


Attachments
Patch (23.05 KB, patch)
2011-01-24 15:19 PST, Zhenyao Mo
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhenyao Mo 2011-01-12 21:46:21 PST
remove unnecessary argument names in WebGLRenderingContext.h
remove unnecessary whitespaces and empty lines
Comment 1 David Levin 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).
Comment 2 Kenneth Russell 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.
Comment 3 David Levin 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.
Comment 4 Zhenyao Mo 2011-01-24 15:19:20 PST
Created attachment 79982 [details]
Patch
Comment 5 WebKit Review Bot 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.
Comment 6 Zhenyao Mo 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.
Comment 7 Kenneth Russell 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.
Comment 8 Zhenyao Mo 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.
Comment 9 Zhenyao Mo 2011-01-25 09:47:56 PST
Committed r76603: <http://trac.webkit.org/changeset/76603>
Comment 10 David Levin 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.)
Comment 11 Zhenyao Mo 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.