Bug 126548 – [WebGL] Be safer about toggling OpenGL state by using a scoped object to control setting lifetime

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Brent Fulgham]

The current WebGL code has a number of places where the same code snippets are used:

    GLboolean isScissorEnabled = ::glIsEnabled(GL_SCISSOR_TEST);
    ::glDisable(GL_SCISSOR_TEST);

[... other code ... ]

    if (isScissorEnabled)
        ::glEnable(GL_SCISSOR_TEST);


These lines are repeated for GL_DITHER and other flags. Sometimes there is a considerable amount of code between the disabling (or enabling) of a flag, and returning it to its initial state before leaving the method. If early returns are added to any of this code, improper OpenGL state will be retained in the context, resulting in improper rendering.

This patch adds a new helper class "TemporaryOpenGLSetting" that does the following:

1. Ensures the setting is in the proper state upon construction.
2. Returns the setting to its original state when the object goes out of scope.

This logic is based on our TemporaryChange<> template (though we do not need a template in this instance, since the glEnable/glDisable logic does not support anything besides a GLenum argument.

[Brent Fulgham]

Created attachment 220471 [details]
Patch

[WebKit Commit Bot]

Attachment 220471 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformBlackBerry.cmake', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/PlatformGTK.cmake', u'Source/WebCore/PlatformNix.cmake', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp', u'Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp', u'Source/WebCore/platform/graphics/opengl/TemporaryOpenGLSetting.cpp', u'Source/WebCore/platform/graphics/opengl/TemporaryOpenGLSetting.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/graphics/opengl/TemporaryOpenGLSetting.cpp:56:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/TemporaryOpenGLSetting.cpp:56:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:301:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:302:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:301:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:302:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:301:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:302:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:301:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:302:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 10 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

(In reply to comment #2)
> Attachment 220471 [details] did not pass style-queue:
> ERROR: Source/WebCore/platform/graphics/opengl/TemporaryOpenGLSetting.cpp:56:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
> ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:301:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
> ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:301:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]

style-bot is going crazy.  Which is it, style-bot? Should the method implementation (which should have 0 spaces) actually have 8 or 12 spaces?

Buggy!

[Brent Fulgham]

(In reply to comment #2)
> Attachment 220471 [details] did not pass style-queue:
> ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:301:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
> ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:302:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
> ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:301:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
> ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:302:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]

Filed Bug 126554 about this craziness.

[Brent Fulgham]

Committed r161389: <http://trac.webkit.org/changeset/161389>