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

[bugzilla-daemon at webkit.org]

Max Vujovic <mvujovic at adobe.com> has canceled 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 163157: Patch
https://bugs.webkit.org/attachment.cgi?id=163157&action=review

------- Additional Comments from Max Vujovic <mvujovic at adobe.com>
Thanks for the review, Alex!

View in context: https://bugs.webkit.org/attachment.cgi?id=163157&action=review


>> Source/WebCore/ChangeLog:20
>> +		uniforms in the last validated vertex shader or fragment
shader. Uniform info includes
> 
> nit: [Typo] includes is written twice

Thanks.

>> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:118
>> +void ANGLEWebKitBridge::getUniformInfo(ShShaderType shaderType,
Vector<ANGLEUniformInfo> &infoVector)
> 
> I suppose this needs to use a plural version of getUniformInfo.

Yes, that would be better. I'll change it to "getUniforms". Then, I'll rename
the struct "ANGLEUniformInfo" to ANGLEShaderSymbol so we can reuse it for
attributes and not just uniforms.

>> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:123
>> +	if (numUniforms <= 0)
> 
> I don't know when ANGLE returns < 0, but I think we should just consider it a
validation error (ie. return false here and invalidate the shader in the
caller).

ANGLE should never return < 0 for the number of uniforms in the shader if we've
hooked things up correctly in WebKit. I'll make an ASSERT(numUniforms >= 0).
Then, I'll return early if there are no uniforms in the author's shader, which
is perfectly valid.

>> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:137
>> +	    info.name = nameBuffer.get();
> 
> Should you use the nameLength in the construction of the String here? There
are constructors that get the length as an argument. 
> Is ANGLE adding the null-char? 
> Is maxNameLength long enough to include the null-char?

> Should you use the nameLength in the construction of the String here? There
are constructors that get the length as an argument.
That's a nice optimization. I'll do that.

> Is ANGLE adding the null-char?
Yes it is.

> Is maxNameLength long enough to include the null-char?
Yes, it includes the null-char.

Thanks for checking :)

>> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:142
>> +int ANGLEWebKitBridge::getParameterValue(const ShHandle compiler,
ShShaderInfo parameterName)
> 
> I suppose you could keep this function as a file static one and maybe use the
inline keyword.

Will do.

>> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:149
>> +bool ANGLEWebKitBridge::isSamplerType(ShDataType dataType)
> 
> Would this look better on the ANGLEUniformInfo itself?

Great idea. Will do.

>> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:88

>> +	bool vertexShaderValid =
validator->validateShaderSource(originalVertexShader.utf8().data(),
SHADER_TYPE_VERTEX, m_validatedVertexShader, vertexShaderLog,
SH_ATTRIBUTES_UNIFORMS);
> 
> Do you want to add a comment about SH_ATTRIBUTES_UNIFORMS around
ANGLEWebKitBridge::getUniformInfo implementation? Others might use the function
and not understand why it's not working for them.

Another great idea. Will do.