[webkit-reviews] review denied: [Bug 44309] Hook up ANGLE with chromium --in-process-webgl port : [Attachment 68767] revised 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 44309: Hook up ANGLE with chromium --in-process-webgl port
https://bugs.webkit.org/show_bug.cgi?id=44309

Attachment 68767: revised patch
https://bugs.webkit.org/attachment.cgi?id=68767&action=review

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

This mostly looks good; I'm marking it r- because of a few relatively minor
issues. Please correct me if I'm wrong about the need for the null checks, but
I'm quite sure strlen(NULL) crashes rather than returning 0.

The introduction of an ANGLE dependency on WebCore means that API changes to
the shader validator are going to be fairly difficult to handle. I'm going to
notify people about this.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:689
> +    int compileStatus;
> +    glGetShaderiv(shader, GL_COMPILE_STATUS, &compileStatus);
> +    // ASSERT that ANGLE generated GLSL will be accepted by OpenGL
> +    ASSERT(compileStatus == GL_TRUE);

It would be safer to handle the case that the OpenGL driver rejects the shader
for some reason. While it's unlikely, even though we're configuring the ANGLE
shader validator with the OpenGL driver's parameters like the maximum number of
uniforms, the OpenGL driver might count things differently than ANGLE and
therefore fail to compile the shader because it exceeds resource constraints.
(Although it's more likely that the OpenGL driver will eliminate unused
uniforms or varyings and therefore use fewer resources than ANGLE counts.) I
think this block is OK in debug mode but the whole thing should be in an
#ifndef NDEBUG.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:958
> +	       *value = static_cast<int>(entry.isValid);

Per above, I think this should delegate to glGetShaderiv if entry.isValid.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:962
> +		   *value = strlen(entry.log);

Guard against the case that entry.log is NULL.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:969
> +	       *value = strlen(entry.source);

Guard against the case that entry.source is NULL.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:987
> +	       WebString res = WebString::fromUTF8(entry.log,
strlen(entry.log));

Guard against the case that entry.log is NULL.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1014
> +	   WebString res = WebString::fromUTF8(entry.source,
strlen(entry.source));

Guard against the case that entry.source is NULL.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1152
> +	   glShaderSource(shader, 1, &string, &length);

The else clause should be an error condition, and synthesize either
INVALID_VALUE or INVALID_OPERATION (probably the latter).

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1358
> +    getIntegerv(0x8DFB, // MAX_VERTEX_UNIFORM_VECTORS

Please define "const int MAX_VERTEX_UNIFORM_VECTORS=0x8DFB" at the top of the
file and use it here and in getIntegerv.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1361
> +    getIntegerv(0x8DFC, // MAX_VARYING_VECTORS

Same here.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1370
> +    getIntegerv(0x8DFD, // MAX_FRAGMENT_UNIFORM_VECTORS

Same here.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.h:330
> +    class ShaderSourceEntry {

If all members are supposed to be public then this should be a struct, not a
class. Note that a struct can have constructors and destructors.