Summary: | drawElements/drawArrays should validate input parameters according to GLES2 spec | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kenneth Russell <kbr> | ||||||
Component: | WebGL | Assignee: | Zhenyao Mo <zmo> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cmarrin, commit-queue, oliver, zmo | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Kenneth Russell
2010-05-06 15:43:31 PDT
Several cases should return INVALID_VALUE instead of INVALID_OPERATION. Created attachment 58315 [details]
patch
Comment on attachment 58315 [details]
patch
Mostly looks good. A few issues.
WebCore/html/canvas/WebGLRenderingContext.cpp:824
+ if (!validateDrawMode(mode))
From recent emails on the public-webgl mailing list it sounds like they may want to strictly specify the order in which errors are returned if the inputs would generate multiple errors. For example, INVALID_ENUM would be preferred over INVALID_VALUE. Because of this, even though it may mean doing some redundant validation, I think we should remove the isGLES2Compliant() check here and always validate the draw mode.
LayoutTests/fast/canvas/webgl/resources/webgl-test.js:110
+ testFailed(evalStr + " expected: " + getGLErrorAsString(ctx, glError) + ". Was " + getGLErrorAsString(ctx, err) + ".");
Can you say here "expected GL error:" and ". Generated: " for clarity?
LayoutTests/fast/canvas/webgl/resources/webgl-test.js:112
+ testPassed(evalStr + " was expected value: " + getGLErrorAsString(ctx, glError) + ".");
Can you say here " generated expected GL error: " for clarity?
LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds.html:44
+ shouldGenerateGLError(context, context.INVALID_ENUM, "context.drawElements(0x0009, 3, context.UNSIGNED_INT, 0)"); // GL_POLYGON
You should use UNSIGNED_BYTE here to make sure you're producing the error for the first enum error and not the second.
Created attachment 58421 [details]
revised patch: responding to kbr's review
Comment on attachment 58421 [details]
revised patch: responding to kbr's review
Looks good.
Comment on attachment 58421 [details]
revised patch: responding to kbr's review
ok.
Comment on attachment 58421 [details] revised patch: responding to kbr's review Clearing flags on attachment: 58421 Committed r61017: <http://trac.webkit.org/changeset/61017> All reviewed patches have been landed. Closing bug. |