[webkit-reviews] review granted: [Bug 45769] drawElements with count=0 and offset = 0 should not generate GL error or crash : [Attachment 67604] revised patch: responding to kbr's review

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Zhenyao Mo <zmo at google.com>'s
request for review:
Bug 45769: drawElements with count=0 and offset = 0 should not generate GL
error or crash
https://bugs.webkit.org/show_bug.cgi?id=45769

Attachment 67604: revised patch: responding to kbr's review
https://bugs.webkit.org/attachment.cgi?id=67604&action=review

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

Sorry for the long delay getting back to this review. I'm marking it r+ since I
think the logic is correct and this fix has been held up long enough, but
ideally we would add one more test simulating vertex attribute 0 and calling
drawElements with count==0. If you decide to add this test now, feel free to
add it upon commit, or otherwise please file a bug about adding the test.

> WebCore/html/canvas/WebGLRenderingContext.cpp:799
> +	       numElements /= sizeof(unsigned short);

This can cause numElements to go to 0 if the buffer only has one element.
However, I don't think that's a problem since it will only cause false to be
returned.

> WebCore/html/canvas/WebGLRenderingContext.cpp:830
> +	   return true;

This clause seems to be used when simulating vertex attribute 0. We should
really add a test for that.