Bug 70981 – Implement new restrictions on uniform and attribute location lengths

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

The WebGL spec was just updated with new restrictions on the lengths of uniform and attribute locations. These restrictions need to be implemented.

[Kenneth Russell]

Created attachment 112634 [details]
Patch

[Stephen White]

Comment on attachment 112634 [details]
Patch

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

Looks good, but I'm leaving this for other ports and WebGL experts to have a look.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4216
> +    const unsigned MaxWebGLLocationLength = 256;

OCD nit:  According to the style guidelines, all variables should start with lower case, and there's no exception for const.  Also, it looks like there is already a start of a collection of constants in this file at line 14, so perhaps this should be moved there.  OTOH, it is kind of nice to be defined close to where it's used.  I couldn't really find a style guideline for that.  (Must wash hands again...)

> LayoutTests/fast/canvas/webgl/resources/webgl-test-utils.js:286
> +      indices[tbase + 0] = index + 0;
> +      indices[tbase + 1] = index + 1;
> +      indices[tbase + 2] = index + vertsAcross;
> +      indices[tbase + 3] = index + vertsAcross;

[After writing this, I realized this code all came from Khronos.  Feel free to upstream my comments, or not.]

Nit:  Since it's only positions, could this mesh be a tri-strip instead of triangles?  Might save on index buffer size.  If you don't mind adding some degenerate triangles, you could even get rid of the index buffer entirely by repeating a vertex at the end of each row and reversing direction.

> LayoutTests/fast/canvas/webgl/resources/webgl-test-utils.js:595
> +    //gl.deleteProgram(fragmentShader);
> +    //gl.deleteProgram(vertexShader);

Why are these commented out?  (blame Khronos)

[Zhenyao Mo]

Comment on attachment 112634 [details]
Patch

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

Looks good to me.

>> LayoutTests/fast/canvas/webgl/resources/webgl-test-utils.js:595

> 
> Why are these commented out?  (blame Khronos)

Those two lines are simply wrong and should just be deleted upstream.  That's the problem syncing with khronos: so many changes unrelated with the bug.

[Kenneth Russell]

Created attachment 112753 [details]
Patch

[Kenneth Russell]

Comment on attachment 112634 [details]
Patch

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

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4216
>> +    const unsigned MaxWebGLLocationLength = 256;
> 
> OCD nit:  According to the style guidelines, all variables should start with lower case, and there's no exception for const.  Also, it looks like there is already a start of a collection of constants in this file at line 14, so perhaps this should be moved there.  OTOH, it is kind of nice to be defined close to where it's used.  I couldn't really find a style guideline for that.  (Must wash hands again...)

You're right. When I wrote this I was thinking about the enum naming guidelines. Fixed. Leaving the const here as there are several other examples in this file of the constant being defined near its use.

>> LayoutTests/fast/canvas/webgl/resources/webgl-test-utils.js:286
>> +      indices[tbase + 3] = index + vertsAcross;
> 
> [After writing this, I realized this code all came from Khronos.  Feel free to upstream my comments, or not.]
> 
> Nit:  Since it's only positions, could this mesh be a tri-strip instead of triangles?  Might save on index buffer size.  If you don't mind adding some degenerate triangles, you could even get rid of the index buffer entirely by repeating a vertex at the end of each row and reversing direction.

I'll forward your comments on to Gregg, who's the author.

>>> LayoutTests/fast/canvas/webgl/resources/webgl-test-utils.js:595
>>> +    //gl.deleteProgram(vertexShader);
>> 
>> Why are these commented out?  (blame Khronos)
> 
> 

I've deleted these upstream and synced again.

[Chris Marrin]

Comment on attachment 112753 [details]
Patch

All looks good

[WebKit Review Bot]

Comment on attachment 112753 [details]
Patch

Clearing flags on attachment: 112753

Committed r98660: <http://trac.webkit.org/changeset/98660>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.