[webkit-reviews] review granted: [Bug 126718] [WebGL] Have getProgramParameter return filtered results for ACTIVE_ATTRIBUTES and ACTIVE_UNIFORMS : [Attachment 220769] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Brent Fulgham <bfulgham at webkit.org>'s
request for review:
Bug 126718: [WebGL] Have getProgramParameter return filtered results for
ACTIVE_ATTRIBUTES and ACTIVE_UNIFORMS
https://bugs.webkit.org/show_bug.cgi?id=126718

Attachment 220769: Patch
https://bugs.webkit.org/attachment.cgi?id=220769&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=220769&action=review


> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2801
> +	   m_context->getNonBuiltinActiveSymbolCount(objectOrZero(program),
pname, &value);

Nit: Should it be BuiltIn (capital I)? Not a big deal to me.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:2
> + * Copyright (C) 2009, 2013 Apple Inc. All rights reserved.

2014

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1021
> +	   static bool isBuiltinName(const String& name)

Same here. Again, your call.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1025
> +	       if (name.startsWith("gl_") || name.startsWith("webgl_") ||
name.startsWith("_webgl_"))
> +		   return true;
> +	       return false;

Just return (name.startsWith...)

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1056
> +	       // Ignore the array components of these symbols for counting
purposes

Nit: period.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:2
> + * Copyright (C) 2010, 2013 Apple Inc. All rights reserved.

2014

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:327

> -    ShaderSourceEntry vertexEntry =
m_shaderSourceMap.find(vertexShader)->value;
> -    ShaderSourceEntry fragmentEntry =
m_shaderSourceMap.find(fragmentShader)->value;
> +    const ShaderSourceEntry& vertexEntry =
m_shaderSourceMap.find(vertexShader)->value;
> +    const ShaderSourceEntry& fragmentEntry =
m_shaderSourceMap.find(fragmentShader)->value;

Cool!

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:555

> +    // Everything went fine, reset our statistics
> +    m_shaderSymbolCount.release(); // Clear counts

Nit: two missing periods.

>
Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1241
> +    // Need to calculate statistics

Nit: periods. There are a few more cases in this function too.