Bug 31172 - Implement getAttachedShaders
Summary: Implement getAttachedShaders
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-05 10:35 PST by Chris Marrin
Modified: 2010-06-29 19:28 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2009-11-05 10:35:11 PST
This array is returned from getAttachedShaders and from getProgramParameter when passing ATTACHED_SHADERS.
Comment 1 Kenneth Russell 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.
Comment 2 Zhenyao Mo 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.
Comment 3 Kenneth Russell 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];
Comment 4 Zhenyao Mo 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.
Comment 5 Zhenyao Mo 2010-06-29 14:31:46 PDT
Created attachment 60055 [details]
revised patch: responding to kbr's review
Comment 6 Kenneth Russell 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.
Comment 7 Dimitri Glazkov (Google) 2010-06-29 15:19:13 PDT
Comment on attachment 60055 [details]
revised patch: responding to kbr's review

ok.
Comment 8 WebKit Commit Bot 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>
Comment 9 WebKit Commit Bot 2010-06-29 19:28:45 PDT
All reviewed patches have been landed.  Closing bug.