[webkit-reviews] review denied: [Bug 94036] [WebGL] program should not be able to link if a bad shader is attached : [Attachment 168122] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Roger Fong <roger_fong at apple.com>'s
request for review:
Bug 94036: [WebGL] program should not be able to link if a bad shader is
attached
https://bugs.webkit.org/show_bug.cgi?id=94036

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

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=168122&action=review


> Source/WebCore/html/canvas/WebGLShader.h:46
> +    bool isValid() const { return m_valid; }

I think this should be just valid(), but see below about avoiding this.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:677
> -    void compileShader(Platform3DObject);
> +    bool compileShader(Platform3DObject);

Unfortunately this change will require a fair amount of coordination with other
platforms, in particular Chromium (who have a bunch of proxies implementing
this API, some in the Chromium project).

Let's see if we can come up with another way to do this. ShaderSourceEntry
already has a "valid" flag that doesn't seem to be used.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:449

>      if (!translatedShaderSource.length())
> -	   return;
> +	   return false;

OK. Maybe we can do it here! If the validation failed, then get the entry in
the Shader Source map and set it to invalid. (In fact, why not set it
true/false for every shader?). Hmm... nope, then we'd still need to either
update GC3D to have API testing for valid shaders, or have linkProgram repeat
the test and communicate the failure.

How does Chrome pass this? Do they detach the shader if it fails validation?
I'll need to think about this for a moment.