[webkit-reviews] review granted: [Bug 214116] Implement uniform* and getUniform for WebGL 2 types : [Attachment 404005] rebase

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted James Darpinian
<jdarpinian at chromium.org>'s request for review:
Bug 214116: Implement uniform* and getUniform for WebGL 2 types
https://bugs.webkit.org/show_bug.cgi?id=214116

Attachment 404005: rebase

https://bugs.webkit.org/attachment.cgi?id=404005&action=review




--- Comment #5 from Dean Jackson <dino at apple.com> ---
Comment on attachment 404005
  --> https://bugs.webkit.org/attachment.cgi?id=404005
rebase

View in context: https://bugs.webkit.org/attachment.cgi?id=404005&action=review

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1285
>> +	if (location->program() != m_currentProgram) {
> 
> Maybe we should add a bool validateUniformLocation(WebGLUniformLocation*,
const char* functionName).

Agreed.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1329
> +    m_context->uniform1uiv(location->location(), value.data(), srcOffset,
srcLength ? srcLength : (value.length() - srcOffset) / 1);

Maybe remove the "/1"?

Also, do we need to validate the case where srcLength is 0 and srcOffset is >
value.length?

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:3409
> +	       // Can't handle this type

Nit: End with a .

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:3471
> +	       // Can't handle this type

Nit: end with a .

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:6900
> +    GCGLsizei actualSize = size - srcOffset;
> +    if (srcLength > 0) {
> +	   if (srcLength > static_cast<GCGLuint>(actualSize)) {
> +	       synthesizeGLError(GraphicsContextGL::INVALID_VALUE,
functionName, "invalid srcOffset + srcLength");
> +	       return false;
> +	   }
> +	   actualSize = srcLength;

Oh, you did it here :)

> Source/WebCore/platform/graphics/GraphicsContextGL.h:1026
> +    virtual void getUniformuiv(PlatformGLObject program, GCGLint location,
GCGLuint* value) = 0;

Huh. I wonder how I missed this.