RESOLVED FIXED 31172
Implement getAttachedShaders
https://bugs.webkit.org/show_bug.cgi?id=31172
Summary Implement getAttachedShaders
Chris Marrin
Reported 2009-11-05 10:35:11 PST
This array is returned from getAttachedShaders and from getProgramParameter when passing ATTACHED_SHADERS.
Attachments
patch (16.32 KB, patch)
2010-06-28 19:22 PDT, Zhenyao Mo
no flags
revised patch: responding to kbr's review (17.16 KB, patch)
2010-06-29 14:31 PDT, Zhenyao Mo
no flags
Kenneth Russell
Comment 1 2010-06-08 12:11:45 PDT
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
Comment 2 2010-06-28 19:22:54 PDT
Created attachment 59977 [details] patch Please review this first. Patch commit has to wait until chrome command buffer port implements this.
Kenneth Russell
Comment 3 2010-06-29 11:37:44 PDT
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
Comment 4 2010-06-29 14:31:04 PDT
(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
Comment 5 2010-06-29 14:31:46 PDT
Created attachment 60055 [details] revised patch: responding to kbr's review
Kenneth Russell
Comment 6 2010-06-29 15:13:49 PDT
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 7 2010-06-29 15:19:13 PDT
Comment on attachment 60055 [details] revised patch: responding to kbr's review ok.
WebKit Commit Bot
Comment 8 2010-06-29 19:28:40 PDT
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
Comment 9 2010-06-29 19:28:45 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.