Bug 64549 – Remove LegacyDefaultOptionalArguments flag from WebGL

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Mark Pilgrim (Google)]

As discussed in IRC, kbr wants to remove the LegacyDefaultOptionalArguments flag from WebGL IDL files. This will trigger the new, stricter type checking on missing required arguments (throwing TypeError instead of failing silently). I've adjusted several test cases that relied on the old, looser behavior.

[Mark Pilgrim (Google)]

Created attachment 100844 [details]
Patch

[Zhenyao Mo]

Should we discuss this on the public_webgl mailing list first?  As far as I know, opera already passes all the conformance tests as they are.  So changing this behavior and changing the tests should be discussed and agreed upon, IMO.

[Kenneth Russell]

Comment on attachment 100844 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100844&action=review

Generally looks good but needs a little more work on the test changes.

> LayoutTests/fast/canvas/webgl/bad-arguments-test.html:62
> +  if (argument == undefined) {
> +    shouldThrow("context.attachShader(argument)");
> +  } else {
> +    func("context.attachShader(argument)");
> +  }

It looks like this was fixed upstream in the Khronos repository, but differently. Could you please copy the associated code from https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/bad-arguments-test.html ?

> LayoutTests/fast/canvas/webgl/context-lost.html:136
> +        "gl.blendFunc(gl.ONE, gl.ONE)",
> +        "gl.blendFuncSeparate(gl.ONE, gl.ONE, gl.ONE, gl.ONE)",

Thanks for fixing this one. I've copied this fix into the Khronos repository.

> LayoutTests/fast/canvas/webgl/null-object-behaviour.html:-25
> -shouldGenerateGLError(context, context.INVALID_VALUE, "context.compileShader()");
> -shouldGenerateGLError(context, context.INVALID_VALUE, "context.linkProgram()");
> -shouldGenerateGLError(context, context.INVALID_VALUE, "context.attachShader()");

It looks like these have been fixed upstream, but differently. Could you please copy the associated code from https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/null-object-behaviour.html ?

> LayoutTests/fast/canvas/webgl/null-object-behaviour.html:-30
> -shouldGenerateGLError(context, context.INVALID_VALUE, "context.shaderSource()");

Same problem here.

[Kenneth Russell]

(In reply to comment #2)
> Should we discuss this on the public_webgl mailing list first?  As far as I know, opera already passes all the conformance tests as they are.  So changing this behavior and changing the tests should be discussed and agreed upon, IMO.

The changes to two out of the three tests are already in the Khronos repo. The fix to the lost context test is obvious and noncontroversial, and I've just committed it to the Khronos repo.

[Mark Pilgrim (Google)]

Created attachment 100858 [details]
Patch

[Kenneth Russell]

Comment on attachment 100858 [details]
Patch

Looks great. Thanks.

[WebKit Review Bot]

Comment on attachment 100858 [details]
Patch

Clearing flags on attachment: 100858

Committed r91053: <http://trac.webkit.org/changeset/91053>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.