Bug 64549 - Remove LegacyDefaultOptionalArguments flag from WebGL
Summary: Remove LegacyDefaultOptionalArguments flag from WebGL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-14 12:36 PDT by Mark Pilgrim (Google)
Modified: 2011-07-14 23:33 PDT (History)
4 users (show)

See Also:


Attachments
Patch (10.42 KB, patch)
2011-07-14 12:44 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (10.16 KB, patch)
2011-07-14 14:02 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.