571076 - Implement GetAttachedShaders

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Should return a JS array of (nsI)WebGLShader.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Implement GetAttachedShaders

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...

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Sounds like I shouldn't review this until you create and add a test! :-)

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

hehehe :)

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: patch to test getAttachedShaders

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?

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Implement GetAttachedShaders, improved

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.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Expand the WebGL program test

This patch expands a lot the program-test. It allowed to catch the 2 issues mentioned in the previous comment.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

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?

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/a2ce1cefbf23