[WebGL] GraphicsContext3D::compileShader stores return value of string conversion in const reference

[Arvid Nilsson]

How can this code possibly work reliably?

void GraphicsContext3D::compileShader(Platform3DObject shader)
{
    ASSERT(shader);
    makeContextCurrent();

    String translatedShaderSource =
m_extensions->getTranslatedShaderSourceANGLE(shader);

    if (!translatedShaderSource.length())
        return;

    int translatedShaderLength = translatedShaderSource.length();

>>> const CString& translatedShaderCString = translatedShaderSource.utf8();
    const char* translatedShaderPtr = translatedShaderCString.data();


Also, I think it should be using translatedShaderSource.latin1() instead of utf8(), since the ANGLEWebKitBridge uses the String::String(char*) constructor, which assumes char* is latin1. That should yield an 8 bit internal representation of the string, and the later call to String::latin1() will be faster, no conversion involved like there is with String::utf8().

[Arvid Nilsson]

Not a problem, the C++ standard says that the life of the const reference shall be prolonged until the end of the current scope.

The string length might have been an issue if the source was non-UTF8, but the source would probably not pass through ANGLE if it were.

[Arvid Nilsson]

Credit where credit is due: thanks to Leo Yang for showing me this part of the C++ standard =)

[Yong Li]

(In reply to comment #1)
> Not a problem, the C++ standard says that the life of the const reference shall be prolonged until the end of the current scope.
> 
> The string length might have been an issue if the source was non-UTF8, but the source would probably not pass through ANGLE if it were.

Are you sure?

>>> const CString& translatedShaderCString = translatedShaderSource.utf8();

Wouldn't the temp CString be destructed after this line?

Also we should use platform/blackberry/ReadOnlyLatin1String.cpp now, if we are sure it is latin1 string. string.latin1() also creates a new copy even the internal buffer is 8bit

[Arvid Nilsson]

(In reply to comment #3)
> (In reply to comment #1)
> > Not a problem, the C++ standard says that the life of the const reference shall be prolonged until the end of the current scope.
> > 
> > The string length might have been an issue if the source was non-UTF8, but the source would probably not pass through ANGLE if it were.
> 
> Are you sure?
> 
> >>> const CString& translatedShaderCString = translatedShaderSource.utf8();
> 
> Wouldn't the temp CString be destructed after this line?
> 
> Also we should use platform/blackberry/ReadOnlyLatin1String.cpp now, if we are sure it is latin1 string. string.latin1() also creates a new copy even the internal buffer is 8bit

I think the string code is suboptimal - utf8 sounds inappropriate for shader source. But unfortunately I named the bug after something that proved not to be an issue - I was thinking to create new bug for the string conversion stuff. Maybe rename bug is better.