WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
revised patch: responding to kbr's review
(17.16 KB, patch)
2010-06-29 14:31 PDT
,
Zhenyao Mo
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug