Bug 87718

Created attachment 144534 [details] patch

Comment on attachment 144534 [details] patch I'm ok with this, but I'd rather let mrobinson/kbr take a look since I don't usually review common ANGLE code.

Comment on attachment 144534 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=144534&action=review > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:78 > +#if USE(OPENGL_ES_2) > + m_fragmentCompiler = ShConstructCompiler(SH_FRAGMENT_SHADER, SH_WEBGL_SPEC, SH_ESSL_OUTPUT, &m_resources); > + m_vertexCompiler = ShConstructCompiler(SH_VERTEX_SHADER, SH_WEBGL_SPEC, SH_ESSL_OUTPUT, &m_resources); > +#else > m_fragmentCompiler = ShConstructCompiler(SH_FRAGMENT_SHADER, SH_WEBGL_SPEC, SH_GLSL_OUTPUT, &m_resources); > m_vertexCompiler = ShConstructCompiler(SH_VERTEX_SHADER, SH_WEBGL_SPEC, SH_GLSL_OUTPUT, &m_resources); > +#endif Perhaps this should be a runtime decision based on the result of isGLES2Compliant. Theoretically a compilation could support both OpenGL and OpenGL ES and I think this is the direction GTK+ is moving.

(In reply to comment #3) > (From update of attachment 144534 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144534&action=review > > > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:78 > > +#if USE(OPENGL_ES_2) > > + m_fragmentCompiler = ShConstructCompiler(SH_FRAGMENT_SHADER, SH_WEBGL_SPEC, SH_ESSL_OUTPUT, &m_resources); > > + m_vertexCompiler = ShConstructCompiler(SH_VERTEX_SHADER, SH_WEBGL_SPEC, SH_ESSL_OUTPUT, &m_resources); > > +#else > > m_fragmentCompiler = ShConstructCompiler(SH_FRAGMENT_SHADER, SH_WEBGL_SPEC, SH_GLSL_OUTPUT, &m_resources); > > m_vertexCompiler = ShConstructCompiler(SH_VERTEX_SHADER, SH_WEBGL_SPEC, SH_GLSL_OUTPUT, &m_resources); > > +#endif > > Perhaps this should be a runtime decision based on the result of isGLES2Compliant. Theoretically a compilation could support both OpenGL and OpenGL ES and I think this is the direction GTK+ is moving. You are right, the solution is better (and nicer) with isGLES2Compliant. I will do it!

Agreed, isGLES2Compliant is the better way to make this decision. To see how Chromium handles this, see GLES2DecoderImpl::InitializeShaderTranslator in src/gpu/command_buffer/service/gles2_cmd_decoder.cc and ShaderTranslator::Init and src/gpu/command_buffer/service/shader_translator.cc under http://src.chromium.org/viewvc/chrome/trunk/ .

Created attachment 144790 [details] work in progress patch Is it appropriate? If yes I will add the initializeShaderCompilers method to the build files. Otherwise please let me know how you imagine it.

Comment on attachment 144790 [details] work in progress patch View in context: https://bugs.webkit.org/attachment.cgi?id=144790&action=review I think this is the right direction, though has some style issues, as appropriate to a WIP patch :) > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:71 > + ShShaderOutput shader_output = (isGLES2 ? SH_ESSL_OUTPUT : SH_GLSL_OUTPUT); style. > Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:56 > + bool initializeShaderCompilers(bool isGLES2); This is kind of a boolean trap. I'd rather have an enum.

Created attachment 145581 [details] WIP should I to add my new function into some build file?

Comment on attachment 145581 [details] WIP Wouldn't this break ports that don't use GraphicsContext3DOpenGLCommon.cpp?

Comment on attachment 145581 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=145581&action=review > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:470 > + if (!(isGLES2Compliant() ? m_compiler.initializeShaderCompilers(SH_ESSL_OUTPUT) : m_compiler.initializeShaderCompilers(SH_GLSL_OUTPUT))) This is a slightly odd choice... I probably would have written: if (!m_compiler.initializeShaderCompilers(isGLES2Compliant() ? SH_ESSL_OUTPUT : SH_GLSL_OUTPUT)) return;

(In reply to comment #9) > (From update of attachment 145581 [details]) > Wouldn't this break ports that don't use GraphicsContext3DOpenGLCommon.cpp? It might break ports that support WebGL, use ANGLEWebKitBridge, but don't use GraphicsContext3DOpenGLCommon. I don't know which ports those might be (Qt?). Chromium doesn't use ANGLEWebKitBridge.

(In reply to comment #11) > (In reply to comment #9) > > (From update of attachment 145581 [details] [details]) > > Wouldn't this break ports that don't use GraphicsContext3DOpenGLCommon.cpp? > I don't really know. Does Mac use any of this stuff? Qt uses GC3DOpenGLCommon.

(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #9) > > > (From update of attachment 145581 [details] [details] [details]) > > > Wouldn't this break ports that don't use GraphicsContext3DOpenGLCommon.cpp? > > > I don't really know. Does Mac use any of this stuff? > Qt uses GC3DOpenGLCommon. The Mac port uses GC3DOpenGLCommon.

(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > (In reply to comment #9) > > > > (From update of attachment 145581 [details] [details] [details] [details]) > > > > Wouldn't this break ports that don't use GraphicsContext3DOpenGLCommon.cpp? > > > > > I don't really know. Does Mac use any of this stuff? > > Qt uses GC3DOpenGLCommon. > > The Mac port uses GC3DOpenGLCommon. OK then. This looks an ok direction to me, though Martin's comments are valid.

Created attachment 145764 [details] patch

Created attachment 145957 [details] patch updated the changelog because since yesterday the changelog is changed.

Comment on attachment 145957 [details] patch Attachment 145957 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12908084 New failing tests: css1/font_properties/font.html

Created attachment 146128 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

(In reply to comment #18) > Created an attachment (id=146128) [details] > Archive of layout-test-results from ec2-cr-linux-03 > > The attached test failures were seen while running run-webkit-tests on the chromium-ews. > Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick This looks flaky.

*** Bug 60617 has been marked as a duplicate of this bug. ***

Comment on attachment 145957 [details] patch This patch introduces a way to misuse the ANGLEWebKitBridge, by running validateShaderSource without calling initializeShaderCompilers first. I think it would be better for ANGLEWebKitBridge to take a ShShaderOutput argument in the constructor and then to call initializeShaderCompilers during the first call to validateShaderSource. At the very least validateShaderSource should ASSERT builtCompilers.

(In reply to comment #21) > At the very least validateShaderSource should ASSERT builtCompilers. By the way, apologies for not noticing this in an earlier review!

Created attachment 149280 [details] patch If I add a new argument to the ANGLEWebKitBridge constructor, the m_compiler (member of GraphicsContext3D class) must be initialized with the parameter at the GraphicsContext3D constructor ( on qt, this located at the GraphicsContext3DQt.cpp ). If I would like to initialize the m_compiler (ANGLEWebKitBridge) at the GC3D constructor, it looks like: GraphicsContext3D::GraphicsContext3D(....) : ..... , ..... #if (opengles) , m_compiler(SH_ESSL_OUTPUT) #else , m_compiler(SH_GLSL_OUTPUT) #endif , ... , ... So I could not initialize the m_compiler runtime (with isGles2Compliant()) only compiling time. [AFAIK] Because of this, I would rather add a new member ( m_shaderOutput) to ANGLEWebKitBridge, and it is initialized at its constructor by macro checking. Let me know how do you think if it not the best solution. :)

(In reply to comment #23) > Created an attachment (id=149280) [details] > patch > > If I add a new argument to the ANGLEWebKitBridge constructor, the m_compiler (member of GraphicsContext3D class) must be initialized with the parameter at the GraphicsContext3D constructor ( on qt, this located at the GraphicsContext3DQt.cpp ). > > If I would like to initialize the m_compiler (ANGLEWebKitBridge) at the GC3D constructor, it looks like: > > GraphicsContext3D::GraphicsContext3D(....) > : ..... > , ..... > #if (opengles) > , m_compiler(SH_ESSL_OUTPUT) > #else > , m_compiler(SH_GLSL_OUTPUT) > #endif > , ... > , ... > isGles2Compliant is not virtual, so it should be safe to call it during construction. Does: m_compile(isGles2Compliant() ? SH_ESSL_OUTPUT : SH_GLSL_OUTPUT) work?

Created attachment 149322 [details] patch

Comment on attachment 149322 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=149322&action=review > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:35 > +ANGLEWebKitBridge::ANGLEWebKitBridge(ShShaderOutput shaderOutput = SH_GLSL_OUTPUT) : Shouldn't the default argument be in the header?

Comment on attachment 149322 [details] patch Attachment 149322 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13090218

Comment on attachment 149322 [details] patch Patch doesn't compile, default argument is at the wrong place.

(In reply to comment #26) > (From update of attachment 149322 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149322&action=review > > > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:35 > > +ANGLEWebKitBridge::ANGLEWebKitBridge(ShShaderOutput shaderOutput = SH_GLSL_OUTPUT) : > > Shouldn't the default argument be in the header? Yes, you are right. Fixed.

Created attachment 149474 [details] patch

Comment on attachment 149474 [details] patch Clearing flags on attachment: 149474 Committed r121259: <http://trac.webkit.org/changeset/121259>

All reviewed patches have been landed. Closing bug.