Bug 64549

Summary: Remove LegacyDefaultOptionalArguments flag from WebGL
Product: WebKit Reporter: Mark Pilgrim (Google) <pilgrim>
Component: WebGLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, kbr, webkit.review.bot, zmo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Mark Pilgrim (Google) 2011-07-14 12:36:43 PDT
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.
Comment 1 Mark Pilgrim (Google) 2011-07-14 12:44:18 PDT
Created attachment 100844 [details]
Patch
Comment 2 Zhenyao Mo 2011-07-14 13:51:53 PDT
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.
Comment 3 Kenneth Russell 2011-07-14 13:53:02 PDT
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.
Comment 4 Kenneth Russell 2011-07-14 13:55:56 PDT
(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.
Comment 5 Mark Pilgrim (Google) 2011-07-14 14:02:58 PDT
Created attachment 100858 [details]
Patch
Comment 6 Kenneth Russell 2011-07-14 14:12:02 PDT
Comment on attachment 100858 [details]
Patch

Looks great. Thanks.
Comment 7 WebKit Review Bot 2011-07-14 23:33:37 PDT
Comment on attachment 100858 [details]
Patch

Clearing flags on attachment: 100858

Committed r91053: <http://trac.webkit.org/changeset/91053>
Comment 8 WebKit Review Bot 2011-07-14 23:33:41 PDT
All reviewed patches have been landed.  Closing bug.