Bug 165253 – [GTK] Use an OpenGL < 3.0 compliant way to request the OpenGL version

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Miguel Gomez]

We are currently using glGetIntegerv() with GL_MAJOR_VERSION and GL_MINOR_VERSION to get the opengl version, but this is not compatible with OpenGL < 3.0.

We need to use glGetString(GL_VERSION) instead, which is compatible with all versions (so far).

[Miguel Gomez]

Created attachment 295849 [details]
Patch

[Michael Catanzaro]

Comment on attachment 295849 [details]
Patch

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

> Source/WebCore/platform/graphics/GLContext.cpp:168
> +        // The version number always has the form "mayor.minor.release".

mayor -> major.

Nit, since I'm leaving other comments on this comment: vendor-specific with a hyphen.

Nit: If you want to leave a line break in the middle of the comment then you should leave an extra blank line as well. I'd prefer to write this comment on just two lines. Looks like it fits:

// Version string always begins with the version number, then a space and the vendor-
// specific info. The version number always has the form "major.minor.release".

Alternatively (less-preferred):

// Version string always begins with the version number, then a space and the vendor-
// specific info.
//
// The version number always has the form "major.minor.release".

> Source/WebCore/platform/graphics/GLContext.cpp:174
> +        versionStringComponents.at(0).split('.', versionDigits);
> +        m_version = versionDigits.at(0).toUInt() * 100 + versionDigits.at(1).toUInt() * 10;

Looks like it's going to crash if the version does not have a . character. I guess it will always exist, but is it guaranteed, or should we be robust to that?

[Carlos Garcia Campos]

Comment on attachment 295849 [details]
Patch

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

> Source/WebCore/platform/graphics/GLContext.cpp:167
> +        // Version string always begins with the version number, then a space and then vendor
> +        // specific info.

I think this is not always true, in case of OpenGL ES, the version string starts with "OpenGL ES", see https://www.khronos.org/opengles/sdk/1.1/docs/man/glGetString.xml.

>> Source/WebCore/platform/graphics/GLContext.cpp:168
>> +        // The version number always has the form "mayor.minor.release".
> 
> mayor -> major.
> 
> Nit, since I'm leaving other comments on this comment: vendor-specific with a hyphen.
> 
> Nit: If you want to leave a line break in the middle of the comment then you should leave an extra blank line as well. I'd prefer to write this comment on just two lines. Looks like it fits:
> 
> // Version string always begins with the version number, then a space and the vendor-
> // specific info. The version number always has the form "major.minor.release".
> 
> Alternatively (less-preferred):
> 
> // Version string always begins with the version number, then a space and the vendor-
> // specific info.
> //
> // The version number always has the form "major.minor.release".

And this is not true either, it could be major.minor.release or just major.minor see https://www.opengl.org/wiki/GLAPI/glGetString

[Miguel Gomez]

Created attachment 295934 [details]
Patch

[Miguel Gomez]

Created attachment 295937 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 295937 [details]
Patch

Clearing flags on attachment: 295937

Committed r209234: <http://trac.webkit.org/changeset/209234>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Is there seriously no forwards-compatible way to check OpenGL version? What we committed looks pretty fragile.