[webkit-reviews] review denied: [Bug 64549] Remove LegacyDefaultOptionalArguments flag from WebGL : [Attachment 100844] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Mark Pilgrim
<pilgrim at chromium.org>'s request for review:
Bug 64549: Remove LegacyDefaultOptionalArguments flag from WebGL
https://bugs.webkit.org/show_bug.cgi?id=64549

Attachment 100844: Patch
https://bugs.webkit.org/attachment.cgi?id=100844&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
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/conform
ance/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/conform
ance/null-object-behaviour.html ?

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

Same problem here.