Implement getAttachedShaders

• Component: WebGL
• Status: RESOLVED FIXED

[Chris Marrin]

This array is returned from getAttachedShaders and from getProgramParameter when passing ATTACHED_SHADERS.

[Kenneth Russell]

Per recent WebGL spec updates, the WebGLObjectArray type has been removed in favor of returning an array of WebGLShader objects from getAttachedShaders.

getProgramParameter(ATTACHED_SHADERS) returns a long indicating the number of attached shaders, not the shaders themselves.

[Zhenyao Mo]

Created attachment 59977 [details]
patch

Please review this first.  Patch commit has to wait until chrome command buffer port implements this.

[Kenneth Russell]

Comment on attachment 59977 [details]
patch

This mostly looks good but there are a few issues.


WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:167
 +          return throwSyntaxError(exec);
This is too stringent. It's legal to supply extra arguments to a JavaScript function.

WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:170
 +      WebGLProgram* program = toWebGLProgram(exec->argument(0));
Double-check to see what happens when either null or a value which isn't a WebGLProgram is passed. Make sure this doesn't crash. Should probably raise a TypeError in some of these cases.


WebCore/html/canvas/WebGLRenderingContext.h:134
 +          Vector<WebGLShader*> getAttachedShaders(WebGLProgram*, ExceptionCode&);
After more thought I think the signature should be

bool getAttachedShaders(WebGLProgram*, Vector<WebGLShader*>&, ExceptionCode&);

so that additional errors can be signalled.


WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:224
 +          V8Proxy::setDOMException(SYNTAX_ERR);
This is also too stringent. Should be "if (args.Length() < 1)".


WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:173
 +      Vector<WebGLShader*> shaders = context->getAttachedShaders(program, ec);
If getAttachedShaders returns false, like it will if the context was lost, this binding should return null.


WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:231
 +      Vector<WebGLShader*> shaders = context->getAttachedShaders(program, ec);
Per above, this binding should return null if getAttachedShaders returns false.


WebCore/html/canvas/WebGLRenderingContext.cpp:1076
 +          ASSERT(count == numShaders);
It would be safer to change the signature of this function to return a boolean and return false in this case. This might happen for example if the context was spontaneously lost.


WebCore/html/canvas/WebGLRenderingContext.cpp:1079
 +              ASSERT(!shader);
I think this should return false for this case as well. I could imagine this could occur if the context were spontaneously lost.


WebCore/html/canvas/WebGLRenderingContext.cpp:1082
 +          delete []shaders;
Strange formatting. "delete[] shaders;".


WebCore/html/canvas/WebGLRenderingContext.cpp:1073
 +          unsigned int* shaders = new unsigned int(numShaders);
new unsigned int[numShaders];

[Zhenyao Mo]

(In reply to comment #3)
> (From update of attachment 59977 [details])
> This mostly looks good but there are a few issues.
> 
> 
> WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:167
>  +          return throwSyntaxError(exec);
> This is too stringent. It's legal to supply extra arguments to a JavaScript function.

Done.

> 
> WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:170
>  +      WebGLProgram* program = toWebGLProgram(exec->argument(0));
> Double-check to see what happens when either null or a value which isn't a WebGLProgram is passed. Make sure this doesn't crash. Should probably raise a TypeError in some of these cases.
toWebGLProgram will return null if the value isn't a WebGLProgram.  Tested, no crash.

> 
> 
> WebCore/html/canvas/WebGLRenderingContext.h:134
>  +          Vector<WebGLShader*> getAttachedShaders(WebGLProgram*, ExceptionCode&);
> After more thought I think the signature should be
> 
> bool getAttachedShaders(WebGLProgram*, Vector<WebGLShader*>&, ExceptionCode&);
> 
> so that additional errors can be signalled.
Done.

> 
> 
> WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:224
>  +          V8Proxy::setDOMException(SYNTAX_ERR);
> This is also too stringent. Should be "if (args.Length() < 1)".
Done.

> 
> 
> WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:173
>  +      Vector<WebGLShader*> shaders = context->getAttachedShaders(program, ec);
> If getAttachedShaders returns false, like it will if the context was lost, this binding should return null.
Done.

> 
> 
> WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:231
>  +      Vector<WebGLShader*> shaders = context->getAttachedShaders(program, ec);
> Per above, this binding should return null if getAttachedShaders returns false.
Done.

> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:1076
>  +          ASSERT(count == numShaders);
> It would be safer to change the signature of this function to return a boolean and return false in this case. This might happen for example if the context was spontaneously lost.
Done.

> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:1079
>  +              ASSERT(!shader);
> I think this should return false for this case as well. I could imagine this could occur if the context were spontaneously lost.
Done.

> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:1082
>  +          delete []shaders;
> Strange formatting. "delete[] shaders;".
Use OwnArrayPtr, so no delete[].

> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:1073
>  +          unsigned int* shaders = new unsigned int(numShaders);
> new unsigned int[numShaders];
Done with OwnArrayPtr.

[Zhenyao Mo]

Created attachment 60055 [details]
revised patch: responding to kbr's review

[Kenneth Russell]

Comment on attachment 60055 [details]
revised patch: responding to kbr's review

Looks good. Note that http://codereview.chromium.org/2805047 needs to land in Chromium before this.

[Dimitri Glazkov (Google)]

Comment on attachment 60055 [details]
revised patch: responding to kbr's review

ok.

[WebKit Commit Bot]

Comment on attachment 60055 [details]
revised patch: responding to kbr's review

Clearing flags on attachment: 60055

Committed r62158: <http://trac.webkit.org/changeset/62158>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.