Bug 42907 - texture functions should gen INVALID_OPERATION if no texture is bound
Summary: texture functions should gen INVALID_OPERATION if no texture is bound
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-23 13:22 PDT by Gregg Tavares
Modified: 2010-08-10 13:45 PDT (History)
5 users (show)

See Also:


Attachments
patch (22.43 KB, patch)
2010-08-06 19:40 PDT, Zhenyao Mo
dglazkov: review+
zmo: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gregg Tavares 2010-07-23 13:22:21 PDT
As per the spec section 5.13.8, if no texture is bound all texture related functions should generate INVALID_OPERATION

eg

gl.bindTexture(gl.TEXTURE_2D, null)
gl.texImage2D(gl.TEXTURE_2D, ...)    <-  should generate INVALID_OPERATION

This conformance test has been updated to show the issue

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/null-object-behaviour.html
Comment 1 Zhenyao Mo 2010-08-06 19:40:06 PDT
Created attachment 63796 [details]
patch
Comment 2 Zhenyao Mo 2010-08-09 13:20:17 PDT
forget to mention: tests are synced with khronos.
Comment 3 Kenneth Russell 2010-08-10 00:32:32 PDT
Comment on attachment 63796 [details]
patch

The code changes look OK but there is an issue with the test:

LayoutTests/fast/canvas/webgl/null-object-behaviour.html:44
 +  shouldGenerateGLError(context, context.NO_ERROR, "context.bindTexture(context.TEXTURE_2D, undefined)");
I think it's a bug in the WebKit WebGL bindings that these accept "undefined" as argument. They should raise TypeError. Only null is a valid non-WebGLTexture argument. If this part of the test is removed then thumbs up from me.
Comment 4 Zhenyao Mo 2010-08-10 09:30:49 PDT
(In reply to comment #3)
> (From update of attachment 63796 [details])
> The code changes look OK but there is an issue with the test:
> 
> LayoutTests/fast/canvas/webgl/null-object-behaviour.html:44
>  +  shouldGenerateGLError(context, context.NO_ERROR, "context.bindTexture(context.TEXTURE_2D, undefined)");
> I think it's a bug in the WebKit WebGL bindings that these accept "undefined" as argument. They should raise TypeError. Only null is a valid non-WebGLTexture argument. If this part of the test is removed then thumbs up from me.

I'll fix the test on khronos and in the patch.
Comment 5 Gregg Tavares 2010-08-10 10:01:28 PDT
If undefined is supposed to generate type error then please change the test to check for type error, don't delete the test.

I ran into tons of no fun bugs in my own code because I was passing undefined unintentionally, misspellings of properties, here and there and WebGL was not complaining about it.  I wasn't sure if it was supposed to complain or not so I ended up wrapping the entire API with a wrapper that checks that no args I pass are undefined.

If those are supposed to reject undefined then we should test that they do.

Is this only for parameters that take null or also for parameters that take number or boolean as well?
Comment 6 Zhenyao Mo 2010-08-10 10:30:19 PDT
(In reply to comment #5)
> If undefined is supposed to generate type error then please change the test to check for type error, don't delete the test.

I will use null instead.

> 
> I ran into tons of no fun bugs in my own code because I was passing undefined unintentionally, misspellings of properties, here and there and WebGL was not complaining about it.  I wasn't sure if it was supposed to complain or not so I ended up wrapping the entire API with a wrapper that checks that no args I pass are undefined.

I think it should so I am planning a patch for it.  However, we need to run through the WebKit folks first because it effects more than just WebGL.

> 
> If those are supposed to reject undefined then we should test that they do.

Yes, "more/functions" in khronos conformance tests have a lot of tests for illegal input parameters.

> 
> Is this only for parameters that take null or also for parameters that take number or boolean as well?

The part of the js binding code I am touching will only check for parameters that take null.
Comment 7 Zhenyao Mo 2010-08-10 10:56:29 PDT
(In reply to comment #5)
> If undefined is supposed to generate type error then please change the test to check for type error, don't delete the test.

After another a look at the test, I realized it's a whole section of tests against "undefined" input.

I'll leave them out in the LayoutTests here and add them back with "shouldThrow" once we have the js binding patch in.

For now LayoutTests and khronos conformance tests will not be the same, but I see no way around it.

> 
> I ran into tons of no fun bugs in my own code because I was passing undefined unintentionally, misspellings of properties, here and there and WebGL was not complaining about it.  I wasn't sure if it was supposed to complain or not so I ended up wrapping the entire API with a wrapper that checks that no args I pass are undefined.
> 
> If those are supposed to reject undefined then we should test that they do.
> 
> Is this only for parameters that take null or also for parameters that take number or boolean as well?
Comment 8 Zhenyao Mo 2010-08-10 11:08:44 PDT
after more thinking, I feel we should check in the patch as it is.  The current test has a nice pattern that tests against 0, undefined, null.  We shouldn't break the pattern by removing part of the undefined section (there are already a few undefined in the test).

We should check them in, and later when we fix the js binding, we change all the 0 and undefined tests to shouldThrow.
Comment 9 Kenneth Russell 2010-08-10 11:36:47 PDT
(In reply to comment #8)
> after more thinking, I feel we should check in the patch as it is.  The current test has a nice pattern that tests against 0, undefined, null.  We shouldn't break the pattern by removing part of the undefined section (there are already a few undefined in the test).
> 
> We should check them in, and later when we fix the js binding, we change all the 0 and undefined tests to shouldThrow.

OK. As we've discussed separately, I was wrong that the tests which pass "undefined" should raise TypeError. Those should convert to null. Patch looks fine as is.
Comment 10 Dimitri Glazkov (Google) 2010-08-10 13:39:09 PDT
Comment on attachment 63796 [details]
patch

ok.
Comment 11 Zhenyao Mo 2010-08-10 13:45:16 PDT
Committed r65089: <http://trac.webkit.org/changeset/65089>