Bug 126548 - [WebGL] Be safer about toggling OpenGL state by using a scoped object to control setting lifetime
Summary: [WebGL] Be safer about toggling OpenGL state by using a scoped object to cont...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-06 16:52 PST by Brent Fulgham
Modified: 2014-01-07 06:15 PST (History)
11 users (show)

See Also:


Attachments
Patch (23.67 KB, patch)
2014-01-06 16:59 PST, Brent Fulgham
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2014-01-06 16:52:24 PST
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.
Comment 1 Brent Fulgham 2014-01-06 16:59:37 PST
Created attachment 220471 [details]
Patch
Comment 2 WebKit Commit Bot 2014-01-06 17:02:14 PST
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.
Comment 3 Brent Fulgham 2014-01-06 17:04:28 PST
(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!
Comment 4 Brent Fulgham 2014-01-06 17:09:16 PST
(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.
Comment 5 Brent Fulgham 2014-01-06 17:45:38 PST
Committed r161389: <http://trac.webkit.org/changeset/161389>