Created attachment 404029 [details] Patch

Created attachment 404159 [details] Patch

Comment on attachment 404159 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404159&action=review > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1355 > + m_context->vertexAttribI4i(index, x, y, z, w); Do we need to confirm index <= m_maxVertexAttribs? > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1382 > + if (index >= m_maxVertexAttribs) { > + synthesizeGLError(GraphicsContextGL::INVALID_VALUE, "vertexAttribI4iv", "index out of range"); > + return; ... because you do here. > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1386 > + memcpy(m_vertexAttribValue[index].iValue, data, sizeof(m_vertexAttribValue[index].iValue)); Do we need to memset(0) the destination values before copying? What if the destination value is larger than the incoming array?

Comment on attachment 404159 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404159&action=review Great work James - LGTM overall with Dean's review feedback addressed. One small point. > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1357 > + m_vertexAttribValue[index].type = GraphicsContextGL::INT; Would this be a good time to #ifdef out - or remove entirely - the m_vertexAttribValue cache? It was removed from Chromium's WebGL implementation a while ago in favor of calling glGetVertexAttribfv, glGetVertexAttribIiv or glGetVertexAttribIuiv directly. That would indirectly address some of the other review feedback.

Comment on attachment 404159 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404159&action=review >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1355 >> + m_context->vertexAttribI4i(index, x, y, z, w); > > Do we need to confirm index <= m_maxVertexAttribs? We could, but we don't need to because ANGLE validates it again. It's only necessary when writing to the m_vertexAttribValue cache below. >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1357 >> + m_vertexAttribValue[index].type = GraphicsContextGL::INT; > > Would this be a good time to #ifdef out - or remove entirely - the m_vertexAttribValue cache? It was removed from Chromium's WebGL implementation a while ago in favor of calling glGetVertexAttribfv, glGetVertexAttribIiv or glGetVertexAttribIuiv directly. That would indirectly address some of the other review feedback. It's tied in with the stuff that emulates vertex attrib 0 and I didn't want to mess around with that in case it's still necessary somewhere. >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1382 >> + return; > > ... because you do here. We do need to check the index before writing to m_vertexAttribValue. This one generates the GL error directly, while the other one lets ANGLE do it. It all works as is, but if you want me to make them consistent let me know which way you prefer. >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1386 >> + memcpy(m_vertexAttribValue[index].iValue, data, sizeof(m_vertexAttribValue[index].iValue)); > > Do we need to memset(0) the destination values before copying? What if the destination value is larger than the incoming array? The size of the incoming array is validated above.

Tools/Scripts/svn-apply failed to apply attachment 404159 [details] to trunk. Please resolve the conflicts and upload a new patch.

Created attachment 404291 [details] rebase

Created attachment 404295 [details] mark reviewed

Comment on attachment 404295 [details] mark reviewed Attempting cq+ as a WebKit committer.

Committed r264381: <https://trac.webkit.org/changeset/264381> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404295 [details].

<rdar://problem/65570741>