[webkit-reviews] review denied: [Bug 70989] Attribute and Uniform variable names need translation in shader : [Attachment 166789] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Dean Jackson <dino at apple.com>'s
request for review:
Bug 70989: Attribute and Uniform variable names need translation in shader
https://bugs.webkit.org/show_bug.cgi?id=70989

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=166789&action=review


review- only because of my mappedName buffer overrun concern. Otherwise looks
good, although I have various suggestions for improvements.

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:44
> +static bool getVariableInfo(const ShHandle compiler, ShShaderInfo
variableType, Vector<ANGLEShaderSymbol>& symbols)

The const in const ShHandle here has no practical effect and should be omitted.


> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:73
> +    OwnArrayPtr<char> nameBuffer = adoptArrayPtr(new char[maxNameLength]);
> +    OwnArrayPtr<char> mappedNameBuffer = adoptArrayPtr(new
char[maxMappedNameLength]);

A higher performance idiom for this is:

    Vector<char, 256> nameBuffer(maxNameLength);

Then you use nameBuffer.data().

Where you replace 256 with a number that is likely to be large enough to hold
most variable names but not so large that it makes the function use an
inappropriate amount of stack. That technique avoids heap allocation/free.

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:94
> +	   symbol.name = String::fromUTF8(nameBuffer.get(), nameLength);
> +	   symbol.mappedName = String::fromUTF8(mappedNameBuffer.get(),
std::min(maxMappedNameLength, nameLength));

This code will put a symbol containing null strings into the vector if there
are illegal UTF-8 sequences in either name. Can that happen? If so, what is the
right way to handle that? Leave out the symbol entirely? Are these strings
really UTF-8? I’m concerned about translating strings from UTF-8 to UTF-16 and
back if they aren’t actually UTF-8 strings. If they are ASCII then it’s more
efficient to treat them as Latin-1 than UTF-8; they’ll stay 8-bit strings and
not be encoded and decoded. If the characters can be arbitrary bytes that might
not be UTF-8 sequences, then we can either treat them as Latin-1, which
guarantees an accurate round trip and has no such concept as an illegal
sequence, or alternatively we might want to use something other than String to
store them.

I am puzzled by the std::min(maxMappedNameLength, nameLength) expression. What
is the guarantee that the mapped name did not overrun the buffer, if our code
is the code that’s clipping the mapped name's length to the maximum? If the
mapped name can come out of the shader functions with a longer length, then
mappedNameBuffer needs to be larger. This is non-obvious enough that it might
require a comment.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:974
> +	   VariableInfo(GC3Denum type, int size, String& mappedName)

Should be const String& rather than String&.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1000
> +	   ShaderVariableMap attribMap;

Can we spell out the word “attribute” instead of using the abbreviation
“attrib”?

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1003
> +	       , isValid(0)

Should be false rather than 0.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1011
> +    String getMappedVariableName(Platform3DObject program, ShShaderInfo
variableType, const String& name);

Normally we don’t name such functions with the word “get”. We reserve the word
“get” for functions that have no return value and use out arguments instead.
We’d typically just call this mappedVariableName.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:99
> +	   // We're only checking uniforms

Comments should answer the question “why” rather than restate what the code
does.

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:174
> +    for (Vector<ANGLEShaderSymbol>::iterator it = variables.begin(); it !=
variables.end(); ++it) {

Normally we use indexing, rather than iterators, to iterate vectors.

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:185
> +	   switch (it->symbolType) {
> +	   case SHADER_SYMBOL_TYPE_ATTRIBUTE:
> +	       entry.attribMap.set(it->name, variableInfo);
> +	       break;
> +	   case SHADER_SYMBOL_TYPE_UNIFORM:
> +	       entry.uniformMap.set(it->name, variableInfo);
> +	       break;
> +	   default:
> +	       ASSERT_NOT_REACHED();
> +	   }

I think we should have a ShaderSourceEntry member function that takes a symbol
type and returns a reference to the appropriate map. Then we can get rid of
this switch statement, and other ones like it in at least one other place. I’d
call the member function variableMap, and then it would be like this:

    entry.variableMap(it->symbolType).set(it->name, variableInfo);

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

> +#if PLATFORM(MAC) || PLATFORM(GTK) || PLATFORM(QT) || PLATFORM(EFL) ||
PLATFORM(BLACKBERRY)

This #if looks bad. Is there some way to avoid this long list of platforms?

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

> +    String mappedName = String(name);

There is no need for the String() around the word “name” here.

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

> +	   ShaderSourceEntry entry = result->second;

Would be more efficient to use a local variable of type const
ShaderSourceEntry& here.

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

> +	   if (variableType == SH_ACTIVE_ATTRIBUTES) {
> +	       variableResult = entry.attribMap.find(name);
> +	       if (variableResult == entry.attribMap.end())
> +		   continue;
> +	   } else {
> +	       variableResult = entry.uniformMap.find(name);
> +	       if (variableResult == entry.uniformMap.end())
> +		   continue;
> +	   }

It would be better factoring here to use something that points to either
attribMap or uniformMap, that way you could have find call and check against
continue also be part of the shared code. If you take my suggestion to add a
member function, then you get this:

    const ShaderVariableMap& variableMap =
result->second.variableMap(variableType);
    ShaderVariableMap::const_iterator variableEntry = variableMap.find(name);
    if (variableEntry != variableMap.end())
	return variableEntry->second.mappedName;

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

> +	   mappedName = variableResult->second.mappedName;
> +	   break;

I think using a return would be clearer here. Then we wouldn’t even need the
mappedName local variable.