Summary: | [GTK] Use an OpenGL < 3.0 compliant way to request the OpenGL version | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Miguel Gomez <magomez> | ||||||||
Component: | WebKitGTK | Assignee: | Miguel Gomez <magomez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bugs-noreply, commit-queue, mcatanzaro | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=165283 | ||||||||||
Attachments: |
|
Description
Miguel Gomez
2016-12-01 07:21:15 PST
Created attachment 295849 [details]
Patch
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? 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 Created attachment 295934 [details]
Patch
Created attachment 295937 [details]
Patch
Comment on attachment 295937 [details] Patch Clearing flags on attachment: 295937 Committed r209234: <http://trac.webkit.org/changeset/209234> All reviewed patches have been landed. Closing bug. Is there seriously no forwards-compatible way to check OpenGL version? What we committed looks pretty fragile. |