Bug 51908

Created attachment 78748 [details] Patch

Attachment 78748 [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 Last 3072 characters of output: 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] Total errors found: 71 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

All the style errors are for gl function argument names. In this patch, I did not clean up WebGLGetInfo and related getLongParameter/getUnsignedLongParameter/getIntParameter. Will create another bug to clean this. Thanks in advance for reviewing this long and boring patch!

Attachment 78748 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7567002

Created attachment 78765 [details] Patch Fix two issues in 32bit build

Attachment 78765 [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 Last 3072 characters of output: 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] Total errors found: 71 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #3) > All the style errors are for gl function argument names. I don't understand what this means. Are you saying that this file isn't suppose to follow WebKit style for some reason? (If so, submit a patch to Tools/Scripts/webkitpy/style/checker.py to handle this.) It wasn't caught by the script but it looks like "pname" the parameter names in the declarations for "WebGLGetInfo getBooleanParameter(GC3Denum pname);" also add no information and should be removed.

(In reply to comment #7) > (In reply to comment #3) > > All the style errors are for gl function argument names. > > I don't understand what this means. Are you saying that this file isn't suppose to follow WebKit style for some reason? (If so, submit a patch to Tools/Scripts/webkitpy/style/checker.py to handle this.) This file and GraphicsContext3D defines a list of GL function signatures and I think their signatures should match the GL signatures in argument names. However, I don't think we should skip these two files totally as we have other functions in them and they should follow the rules. Fortunately these GL-like functions won't change often (hopefully after this patch they will never need to change again), so you won't see those style error messages again. > > It wasn't caught by the script but it looks like "pname" the parameter names in the declarations for "WebGLGetInfo getBooleanParameter(GC3Denum pname);" also add no information and should be removed. This patch didn't add/delete/modify any argument names. It's simply changing the types. I don't think we should clean any legacy style violations in this patch.

Create a bug for style cleanup: https://bugs.webkit.org/show_bug.cgi?id=52352

Comment on attachment 78765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78765&action=review Thanks for doing this tedious cleanup. It will make the code more maintainable. The only high-level issue is that I think GC3Dboolean should be typedefed to bool rather than unsigned char. Using 0 instead of false loses a significant amount of information and it's difficult to make the reverse transition, because some platforms will complain about converting 0/1 to a bool and some won't. There are probably ten or so places in this patch where this affects the code. > Source/WebCore/html/canvas/WebGLBuffer.cpp:131 Should these CheckedInt instances use GC3Dintptr as their template parameter? Theoretically they are supposed to handle 64-bit values on 64-bit platforms. > Source/WebCore/html/canvas/WebGLBuffer.cpp:132 > + if (!checkedArrayMax.valid() || checkedArrayMax.value() > static_cast<int32_t>(array->byteLength()) || !checkedBufferMax.valid() || checkedBufferMax.value() > m_byteLength) Should this static_cast to int32_t use one of the GC3D types, again like GC3Dintptr? > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:1910 > + return WebGLGetInfo(static_cast<unsigned long>(m_renderbufferBinding->getInternalFormat())); Might be a good idea to add a FIXME here even though you've already filed the cleanup bug to get rid of this cast to unsigned long. > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:1932 > + return WebGLGetInfo(static_cast<unsigned long>(m_renderbufferBinding->getInternalFormat())); Here too. > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2194 > + return WebGLGetInfo(static_cast<bool>(m_vertexAttribState[index].enabled)); FIXME here and below?

Committed r75741: <http://trac.webkit.org/changeset/75741>

For the records: 1) I didn't change the mapping of GC3Dboolean from unsigned char to bool as it turned out to be more complicated then I thought: getBooleanv function that takes a GLboolean* type, so it needs to be taken care of. However, I changed all the 0/1 cases in WebGLRenderingContext to true/false so the semantic meaning is not lost here. 2) CheckedInt takes int/long long, but does not take long. Will create a bug to fix this. Thanks again for the quick review for this huge ugly patch.

http://trac.webkit.org/changeset/75741 might have broken GTK Linux 64-bit Debug The following tests are not passing: editing/input/page-up-down-scrolls.html