Bug 93871

Created attachment 163154 [details] Patch Alex, can you take a look at this?

Attachment 163154 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1 Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:46: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:47: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:48: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:63: The parameter name "shaderType" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:65: The parameter name "dataType" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 5 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 163157 [details] Patch Fixed style issues.

Comment on attachment 163157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163157&action=review Looks good overall. I just had some small comments. > Source/WebCore/ChangeLog:20 > + uniforms in the last validated vertex shader or fragment shader. Uniform info includes nit: [Typo] includes is written twice > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:118 > +void ANGLEWebKitBridge::getUniformInfo(ShShaderType shaderType, Vector<ANGLEUniformInfo> &infoVector) I suppose this needs to use a plural version of getUniformInfo. > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:123 > + if (numUniforms <= 0) I don't know when ANGLE returns < 0, but I think we should just consider it a validation error (ie. return false here and invalidate the shader in the caller). > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:137 > + info.name = nameBuffer.get(); Should you use the nameLength in the construction of the String here? There are constructors that get the length as an argument. Is ANGLE adding the null-char? Is maxNameLength long enough to include the null-char? > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:142 > +int ANGLEWebKitBridge::getParameterValue(const ShHandle compiler, ShShaderInfo parameterName) I suppose you could keep this function as a file static one and maybe use the inline keyword. > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:149 > +bool ANGLEWebKitBridge::isSamplerType(ShDataType dataType) Would this look better on the ANGLEUniformInfo itself? > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:88 > + bool vertexShaderValid = validator->validateShaderSource(originalVertexShader.utf8().data(), SHADER_TYPE_VERTEX, m_validatedVertexShader, vertexShaderLog, SH_ATTRIBUTES_UNIFORMS); Do you want to add a comment about SH_ATTRIBUTES_UNIFORMS around ANGLEWebKitBridge::getUniformInfo implementation? Others might use the function and not understand why it's not working for them.

Comment on attachment 163157 [details] Patch Thanks for the review, Alex! View in context: https://bugs.webkit.org/attachment.cgi?id=163157&action=review >> Source/WebCore/ChangeLog:20 >> + uniforms in the last validated vertex shader or fragment shader. Uniform info includes > > nit: [Typo] includes is written twice Thanks. >> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:118 >> +void ANGLEWebKitBridge::getUniformInfo(ShShaderType shaderType, Vector<ANGLEUniformInfo> &infoVector) > > I suppose this needs to use a plural version of getUniformInfo. Yes, that would be better. I'll change it to "getUniforms". Then, I'll rename the struct "ANGLEUniformInfo" to ANGLEShaderSymbol so we can reuse it for attributes and not just uniforms. >> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:123 >> + if (numUniforms <= 0) > > I don't know when ANGLE returns < 0, but I think we should just consider it a validation error (ie. return false here and invalidate the shader in the caller). ANGLE should never return < 0 for the number of uniforms in the shader if we've hooked things up correctly in WebKit. I'll make an ASSERT(numUniforms >= 0). Then, I'll return early if there are no uniforms in the author's shader, which is perfectly valid. >> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:137 >> + info.name = nameBuffer.get(); > > Should you use the nameLength in the construction of the String here? There are constructors that get the length as an argument. > Is ANGLE adding the null-char? > Is maxNameLength long enough to include the null-char? > Should you use the nameLength in the construction of the String here? There are constructors that get the length as an argument. That's a nice optimization. I'll do that. > Is ANGLE adding the null-char? Yes it is. > Is maxNameLength long enough to include the null-char? Yes, it includes the null-char. Thanks for checking :) >> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:142 >> +int ANGLEWebKitBridge::getParameterValue(const ShHandle compiler, ShShaderInfo parameterName) > > I suppose you could keep this function as a file static one and maybe use the inline keyword. Will do. >> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:149 >> +bool ANGLEWebKitBridge::isSamplerType(ShDataType dataType) > > Would this look better on the ANGLEUniformInfo itself? Great idea. Will do. >> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:88 >> + bool vertexShaderValid = validator->validateShaderSource(originalVertexShader.utf8().data(), SHADER_TYPE_VERTEX, m_validatedVertexShader, vertexShaderLog, SH_ATTRIBUTES_UNIFORMS); > > Do you want to add a comment about SH_ATTRIBUTES_UNIFORMS around ANGLEWebKitBridge::getUniformInfo implementation? Others might use the function and not understand why it's not working for them. Another great idea. Will do.

Created attachment 163251 [details] Patch Updated patch from Alex's informal review.

Comment on attachment 163251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163251&action=review This patch is going to be helpful in my bug to map translated uniform and attribute names to/from their original names! (e.g. when they are over 256 chars in length) Just some small change requests. > Source/WebCore/ChangeLog:13 > + Additionally, reject shaders with author-defined sampler uniforms. When we implement texture > + parameters, we will allow shaders whose samplers are bound to valid textures. We must not > + allow OpenGL to give unbound samplers a default value of 0 because that references the DOM > + element texture, which should be inaccessible to the author's shader code. This probably shows my ignorance of GL, but can the author do texture2D(0, vec2(10, 10)); or will ANGLE reject that? > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:35 > +inline static int getParameterValue(const ShHandle compiler, ShShaderInfo parameterName) I'm not sure this is the best name. To me "parameter" implies that it is a value you pass into the compiler, whereas this is getting values produced by the compiler. I can't think of a better name right now though. getCompilerValue seems a bit weird. > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:130 > + // There shouldn't be fewer than zero uniforms. I don't think you need this comment - the assert and conditional explain it. > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:136 > + // The max length for a uniform name should be at least one character long. Similarly here. The next line ASSERTs this comment. The difference here is that we'll crash in release builds if length is < 0 due to later code. I think we should guard against this. > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:145 > + // A uniform name should be at least one character long. And again. > Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.cpp:39 > +CustomFilterCompiledProgram::CustomFilterCompiledProgram(PassRefPtr<GraphicsContext3D> context, const String& validatedVertexShader, const String& validatedFragmentShader, bool mixEnabled) I think "mixEnabled" could be named something more like "allowedToAccessPrivateTexures" (although not that name, it's terrible!). I'm looking for something that describes the meaning in the context of this function. "mix" is a term that comes from the application of the shader, not its construction. > Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.cpp:133 > + // Only enable these internal symbols if the author is using the CSS mix function. Could you explain this in more detail, and maybe in reverse? For example, "when the author is using the mix function in their CSS shader, they are then able to access internal symbols representing the texture of the DOM element..." or something :)

(In reply to comment #7) > This probably shows my ignorance of GL, but can the author do texture2D(0, vec2(10, 10)); or will ANGLE reject that? Of course I meant vec2(0.5, 0.5) :)

Created attachment 163501 [details] Patch Thanks for the review, Dean! (In reply to comment #7)> (From update of attachment 163251 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163251&action=review > > This patch is going to be helpful in my bug to map translated uniform and attribute names to/from their original names! (e.g. when they are over 256 chars in length) > Awesome! > Just some small change requests. > > > Source/WebCore/ChangeLog:13 > > + Additionally, reject shaders with author-defined sampler uniforms. When we implement texture > > + parameters, we will allow shaders whose samplers are bound to valid textures. We must not > > + allow OpenGL to give unbound samplers a default value of 0 because that references the DOM > > + element texture, which should be inaccessible to the author's shader code. > > This probably shows my ignorance of GL, but can the author do texture2D(0, vec2(10, 10)); or will ANGLE reject that? > Good question :). Thankfully, ANGLE will reject that since it expects a sampler2D for the first argument, but here we've provided an int. We'll get an error like "No matching function for call to texture2D(int, vec2)". We are also fortunate because samplers are not valid l-values in GLSL, meaning an author can't change their value in the shader. > > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:35 > > +inline static int getParameterValue(const ShHandle compiler, ShShaderInfo parameterName) > > I'm not sure this is the best name. To me "parameter" implies that it is a value you pass into the compiler, whereas this is getting values produced by the compiler. I can't think of a better name right now though. getCompilerValue seems a bit weird. > Right. Let's call it "getValidationResultValue" since we're getting info related to the result of last validation (e.g. the number of uniforms found). > > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:130 > > + // There shouldn't be fewer than zero uniforms. > > I don't think you need this comment - the assert and conditional explain it. > Alex had a good idea about this- I've changed ANGLEWebKitBridge::getUniforms to return a bool. It returns false if an unexpected error occurred in ANGLE. Otherwise, it returns true. This way, we don't validate the shader if something went wrong with ANGLE, and we also don't crash. Most importantly, we don't miss an author-defined sampler and proceed, since that could be a security issue. I've removed all of the ASSERTs and their corresponding comments since we return false in those cases now. I've also added a comment about the return value of getUniforms() above its prototype. > > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:136 > > + // The max length for a uniform name should be at least one character long. > > Similarly here. The next line ASSERTs this comment. The difference here is that we'll crash in release builds if length is < 0 due to later code. I think we should guard against this. > See above. > > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:145 > > + // A uniform name should be at least one character long. > > And again. > See above. > > Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.cpp:39 > > +CustomFilterCompiledProgram::CustomFilterCompiledProgram(PassRefPtr<GraphicsContext3D> context, const String& validatedVertexShader, const String& validatedFragmentShader, bool mixEnabled) > > I think "mixEnabled" could be named something more like "allowedToAccessPrivateTexures" (although not that name, it's terrible!). I'm looking for something that describes the meaning in the context of this function. "mix" is a term that comes from the application of the shader, not its construction. > Yes, mixEnabled is an unfortunate name. Based on our IRC chat, I've made a enum instead: enum CustomFilterProgramType { PROGRAM_TYPE_NO_ELEMENT_TEXTURE, PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE }; Now we pass this programType enum instead of the mixEnabled bool. In general, I think programInfo should have a programType, and we should remove mixSettings.enabled. I've added a FIXME and a bug for this: https://bugs.webkit.org/show_bug.cgi?id=96448 > > Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.cpp:133 > > + // Only enable these internal symbols if the author is using the CSS mix function. > > Could you explain this in more detail, and maybe in reverse? For example, "when the author is using the mix function in their CSS shader, they are then able to access internal symbols representing the texture of the DOM element..." or something :) I've changed it to: // When the author uses the CSS mix function in a custom filter, we add internal symbols to the shader code. // One of them, css_u_texture, references the texture of the element.

Created attachment 163649 [details] Patch for Landing Added Dean to Reviewed By field in ChangeLogs.

Comment on attachment 163649 [details] Patch for Landing Clearing flags on attachment: 163649 Committed r128334: <http://trac.webkit.org/changeset/128334>

All reviewed patches have been landed. Closing bug.