[webkit-reviews] review denied: [Bug 51725] vertexAttribPointer should raise INVALID_OPERATION if stride/offset is not multiple of the type size : [Attachment 77646] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Zhenyao Mo <zmo at google.com>'s
request for review:
Bug 51725: vertexAttribPointer should raise INVALID_OPERATION if stride/offset
is not multiple of the type size
https://bugs.webkit.org/show_bug.cgi?id=51725

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

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=77646&action=review

The code changes look good and the test case certainly looks thorough, but the
number of test cases being run are massive and I don't think all of the
variants really increase the code coverage. In the WebGL test suite each pass
line will act as an independent test case so this single patch will drastically
increase the number of tests that we claim to run. I think you should look for
ways to reduce the number of variants tested per type. A few related and
unrelated comments.

> LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:60
> +	   "(" + count + ") " +

I don't think it's a good idea to include this count in the reported string.
Adding or removing any test cases later will then cause huge spurious changes
in the expected.txt file.

> LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:86
> +	 for (var off = 0; off < 4; ++off) {

The "4" here seems pretty arbitrary. Is there a strong reason for it?

> LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:87
> +	   for (var ss = 0; ss < info.bytesPerComponent; ++ss) {

How about a more descriptive name than "ss" like "offsetStep"?

> LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:89
> +	     for (var n = 0; n < 2; ++n) {

Testing both true and false values for the normalize argument doesn't really
improve the test coverage and doubles the number of lines in the expected.txt
file (making it harder to locate errors) so I'd suggest taking it out and just
always passing down false.

> LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:90
> +	       for (var st = 0; st < bytesPerElement * 2; ++st) {

Just use "var stride = 0; ..." here. Also, is there a reason to test all of
these variants up to stride = 2 * bytesPerElement? Does doing so really
increase the test coverage?

> LayoutTests/fast/canvas/webgl/gl-vertexattribpointer.html:114
> +		     gl, gl.NO_ERROR, "at stide limit",

stide -> stride