Closed Bug 571076 Opened 12 years ago Closed 12 years ago

Implement GetAttachedShaders

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: bjacob)

Details

Attachments

(2 files, 2 obsolete files)

Should return a JS array of (nsI)WebGLShader.
Attached patch Implement GetAttachedShaders (obsolete) — Splinter Review
Patch; note that no existing conformance test covers this function at all. So the only thing that I checked, is that it compiles. Should learn some JS...
Attachment #450414 - Flags: review?(vladimir)
Sounds like I shouldn't review this until you create and add a test! :-)
hehehe :)
Attached patch patch to test getAttachedShaders (obsolete) — Splinter Review
Here's a patch against the program-test.html test, making it test getAttachedShaders. Do you think 1) it's enough testing and 2) that this test change would be OK to commit to khronos.org?
Attachment #450435 - Flags: review?(vladimir)
Nope, not nearly enough testing (also space after "if"); this should test at least --

 - getting attached shaders with a program with no shaders (empty list)
 - with a few shaders attached
 - removing a shader, then checking the list
 - removing all shaders, ensuring list is empty
 - maybe adding some shaders whose compilation purposely fails, and checking that they still exist in the list
This new version fixes an issue discovered while testing (see next comment): GetAttachedShaders was returning null object instead of empty list.

It also fixes an issue in LinkProgram: when the program doesn't have both shader types attached, the result was INVALID_OPERATION, but according to the OpenGL ES documentation, this shouldn't generate any GL error, it should simply let the program in a failed link status.
Attachment #450414 - Attachment is obsolete: true
Attachment #451077 - Flags: review?(vladimir)
Attachment #450414 - Flags: review?(vladimir)
This patch expands a lot the program-test. It allowed to catch the 2 issues mentioned in the previous comment.
Attachment #450435 - Attachment is obsolete: true
Attachment #451078 - Flags: review?(vladimir)
Attachment #450435 - Flags: review?(vladimir)
Comment on attachment 451078 [details] [diff] [review]
Expand the WebGL program test

Looks good, though a style comment on the tests.. right now the tests create a new program for each check and validate each step along the way.  That's probably fine, but it would also be ok (and maybe easier to follow) if you just used one program, attaching/detaching shaders along the way and validating that the set of attached shaders is consistent after each step.  Up to you which way you want to keep it, though.
Attachment #451078 - Flags: review?(vladimir) → review+
Attachment #451077 - Flags: review?(vladimir) → review+
My fear is that, since we store state in WebGLProgram, reusing a WebGLProgram object may lead to different behavior (if we have a bug). So I felt more comfortable creating a new WebGLProgram everytime. Is that reasonable?
http://hg.mozilla.org/mozilla-central/rev/a2ce1cefbf23
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.