Closed Bug 569714 Opened 14 years ago Closed 14 years ago

Pass webgl gl-get-calls test

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bjacob, Assigned: bjacob)

Details

Attachments

(3 files)

Note: this is NOT the same test as in bug 567499 !! Despite the similar name.

This patch makes this test fully pass, except for a bug in the test itself at the end:

    shouldBe('context.getParameter(context.SHADER_BINARY_FORMATS)', '[]');

There is no SHADER_BINARY_FORMATS constant in WebGL.
Assignee: nobody → bjacob
Status: NEW → ASSIGNED
Attachment #448835 - Flags: review?(vladimir)
Ah, so this is the one that was checked in by accident.  Ok; let's just do followup patches -- the changes aren't bad, just have smoe problems.

So, in the IDL, this currently looks like:

   void getParameter(in WebGLenum pname);

Where the return type is 'void' because we just do magic here and set the JS retval directly.  However, that was when we only returned simple types; now, we need to return, e.g., a nsIWebGLTexture in some cases.  So the easiest thing to do would be to have this return nsIVariant.

With a nsIVariant return, take a look at xpcom/ds/nsIVariant.idl; you'll need to create a new nsIWritableVariant and call the appropriate set function, and then set the retval to it.  We won't need any of the NativeCallContext stuff or anything like that any more in the function.

For returning arrays, use SetAsArray, e.g.:

  PRBool fourBools[4];

  ...->SetAsArray(nsIDataType::VTYPE_BOOL, nsnull, 4, reinterpret_cast<void *>(fourBoolds));

For returning webgl objects, you can use SetAsISupports(obj);
Summary: Pass Khrono's gl-get-calls test → Pass webgl gl-get-calls test
OK, here's the patch. I also fixed other pnames, and wrote to public_webgl about the ones that I really can't fix.
Attachment #450216 - Flags: review?(vladimir)
Comment on attachment 450216 [details] [diff] [review]
patch using nsIVariant

Looks good, with these fixes:

>+    nsCOMPtr<nsIWritableVariant> wrval = do_CreateInstance("@mozilla.org/variant;1");

Add a NS_ENSURE_SUCCESS(wrval, NS_ERROR_FAILURE); after this

>-        //case LOCAL_GL_IMPLEMENTATION_COLOR_READ_TYPE:
>-        //case LOCAL_GL_IMPLEMENTATION_COLOR_READ_FORMAT:
>+        case LOCAL_GL_IMPLEMENTATION_COLOR_READ_TYPE:
>+        case LOCAL_GL_IMPLEMENTATION_COLOR_READ_FORMAT:

These aren't valid enums on the desktop; leave them commented out until we figure out what to do with the whole set of 4.

> // javascript integer values. We just return them as double's and javascript doesn't care.

Can you fix the typo here, while you're touching this code? "double's" -> "doubles"

>-        case LOCAL_GL_COLOR_WRITEMASK: // 4 bools
>+        case LOCAL_GL_COLOR_WRITEMASK: // array 4 uint8's

These really are 4 booleans, even though you need to grab them as 4 uint8 values.

>+            realGLboolean gl_bv[4] = { 0 };
>+            gl->fGetBooleanv(pname, gl_bv);
>+            // it might be relatively safe to assume that realGLboolean is 1 byte,
>+            // but it's probably not entirely safe and this is not perf-critical anyway.
>+            PRUint8 pr_bv[4] = { gl_bv[0], gl_bv[1], gl_bv[2], gl_bv[3] };

^ PRBool, not PRUint8

>+            wrval->SetAsArray(nsIDataType::VTYPE_UINT8, nsnull,

VTYPE_BOOL, not UINT8.  You want these to be bools on the JS side, not 0/1 values.
Attachment #450216 - Flags: review?(vladimir) → review+
The new patch addresses your comments, except:

> Add a NS_ENSURE_SUCCESS(wrval, NS_ERROR_FAILURE); after this

This gave me a compiler error, but wrval!=nsnull worked.

>> +        PRUint8 pr_bv[4] = { gl_bv[0], gl_bv[1], gl_bv[2], gl_bv[3] };
>
> ^ PRBool, not PRUint8
>
>> +        wrval->SetAsArray(nsIDataType::VTYPE_UINT8, nsnull,
>
> VTYPE_BOOL, not UINT8.  You want these to be bools on the JS side, not
> 0/1values.

The WebGL spec really says Uint8Array for GL_COLOR_WRITEMASK. And the gl-get-calls really wants numbers, not bools. Are they wrong?
(carrying forward Vlad's r+)
Attachment #450350 - Flags: review+
(In reply to comment #6)
> The new patch addresses your comments, except:
> 
> > Add a NS_ENSURE_SUCCESS(wrval, NS_ERROR_FAILURE); after this
> 
> This gave me a compiler error, but wrval!=nsnull worked.

Sorry, my bad - NS_ENSURE_TRUE(wrval, NS_ERROR_FAILURE);

> 
> >> +        PRUint8 pr_bv[4] = { gl_bv[0], gl_bv[1], gl_bv[2], gl_bv[3] };
> >
> > ^ PRBool, not PRUint8
> >
> >> +        wrval->SetAsArray(nsIDataType::VTYPE_UINT8, nsnull,
> >
> > VTYPE_BOOL, not UINT8.  You want these to be bools on the JS side, not
> > 0/1values.
> 
> The WebGL spec really says Uint8Array for GL_COLOR_WRITEMASK. And the
> gl-get-calls really wants numbers, not bools. Are they wrong?

Hmm -- I think so, the GL spec specifies that these are booleans (though in C they're returned as 8-bit valus) -- the glColorMask entry point takes booleans, so I think it should be bools.  Send mail to the list?
ok, mail sent.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: