[webkit-reviews] review granted: [Bug 93871] [CSS Shaders] Remove direct texture access via u_texture : [Attachment 163251] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Max Vujovic <mvujovic at adobe.com>'s
request for review:
Bug 93871: [CSS Shaders] Remove direct texture access via u_texture
https://bugs.webkit.org/show_bug.cgi?id=93871

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

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


This patch is going to be helpful in my bug to map translated uniform and
attribute names to/from their original names! (e.g. when they are over 256
chars in length)

Just some small change requests.

> Source/WebCore/ChangeLog:13
> +	   Additionally, reject shaders with author-defined sampler uniforms.
When we implement texture 
> +	   parameters, we will allow shaders whose samplers are bound to valid
textures. We must not
> +	   allow OpenGL to give unbound samplers a default value of 0 because
that references the DOM
> +	   element texture, which should be inaccessible to the author's shader
code.

This probably shows my ignorance of GL, but can the author do texture2D(0,
vec2(10, 10)); or will ANGLE reject that?

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:35
> +inline static int getParameterValue(const ShHandle compiler, ShShaderInfo
parameterName)

I'm not sure this is the best name. To me "parameter" implies that it is a
value you pass into the compiler, whereas this is getting values produced by
the compiler. I can't think of a better name right now though. getCompilerValue
seems a bit weird.

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:130
> +    // There shouldn't be fewer than zero uniforms.

I don't think you need this comment - the assert and conditional explain it.

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:136
> +    // The max length for a uniform name should be at least one character
long.

Similarly here. The next line ASSERTs this comment. The difference here is that
we'll crash in release builds if length is < 0 due to later code. I think we
should guard against this.

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:145
> +	   // A uniform name should be at least one character long.

And again.

> Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.cpp:39
>
+CustomFilterCompiledProgram::CustomFilterCompiledProgram(PassRefPtr<GraphicsCo
ntext3D> context, const String& validatedVertexShader, const String&
validatedFragmentShader, bool mixEnabled)

I think "mixEnabled" could be named something more like
"allowedToAccessPrivateTexures" (although not that name, it's terrible!). I'm
looking for something that describes the meaning in the context of this
function. "mix" is a term that comes from the application of the shader, not
its construction.

> Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.cpp:133
> +	   // Only enable these internal symbols if the author is using the CSS
mix function.

Could you explain this in more detail, and maybe in reverse? For example, "when
the author is using the mix function in their CSS shader, they are then able to
access internal symbols representing the texture of the DOM element..." or
something :)