Bug 98973

Created attachment 170183 [details] Patch

Comment on attachment 170183 [details] Patch In this patch, I still need to figure out how to get the meshType in CustomFilterValidatedProgram, since a_triangleCoord is a built-in attribute in detached meshes but not in attached meshes.

Created attachment 170762 [details] Patch Now the patch uses the mesh type to check if a_triangleCoord is a valid attribute.

Comment on attachment 170762 [details] Patch Attachment 170762 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14559802

View in context: https://bugs.webkit.org/attachment.cgi?id=170762&action=review Looks good. > LayoutTests/css3/filters/resources/invalid-a-triangle-coord-with-attached-mesh.vs:9 > +attribute vec3 a_triangleCoord; > + > +attribute vec4 a_position; > + > +uniform mat4 u_projectionMatrix; Remove the empty lines. > LayoutTests/css3/filters/resources/invalid-custom-attribute.vs:9 > +attribute float my_attribute; > + > +attribute vec4 a_position; > + > +uniform mat4 u_projectionMatrix; Ditto. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:-3805 > - A24A3A9D162499FF00522745 /* CustomFilterConstants.h in Headers */ = {isa = PBXBuildFile; fileRef = A24A3A9C162499FF00522745 /* CustomFilterConstants.h */; }; I think you can revert the xcode changes. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:18251 > - A24A3A9C162499FF00522745 /* CustomFilterConstants.h */, > + A2B2AE7B16375EE500CFA50B /* CustomFilterConstants.h */, ditto > Source/WebCore/WebCore.xcodeproj/project.pbxproj:22694 > - A24A3A9D162499FF00522745 /* CustomFilterConstants.h in Headers */, > + A2B2AE7C16375EE500CFA50B /* CustomFilterConstants.h in Headers */, ditto > Source/WebCore/platform/graphics/filters/CustomFilterProgram.h:63 > // StyleCustomFilterProgram has the only implementation for the following method. That means, it casts to StyleCustomFilterProgram > - // withouth checking the type. If you add another implementation, also add a mechanism to check for the correct type. > + // without checking the type. If you add another implementation, also add a mechanism to check for the correct type. I think this comment is not needed anymore. > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:66 > +bool CustomFilterValidatedProgram::validateSymbols(const Vector<ANGLEShaderSymbol>& symbols) Pass the mesh type as a parameter to keep this static. > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:80 > + if (symbol.name == "a_triangleCoord" && m_programInfo.meshType() != MeshTypeDetached) { Why use the != operator instead of == ? We just have two modes at this moment. nit: Put the fastest comparison first. It will avoid making the strcmp when not needed.

Comment on attachment 170762 [details] Patch Attachment 170762 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14544827

Comment on attachment 170762 [details] Patch Attachment 170762 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14569838

Created attachment 170979 [details] Patch Trying to fix Qt, Chromium EWS build errors.

Comment on attachment 170979 [details] Patch Attachment 170979 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14611143

Created attachment 170995 [details] Patch Qt EWS build fix attempt.

Comment on attachment 170995 [details] Patch Attachment 170995 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14617118

Created attachment 171023 [details] Patch Trying qt-wk2 again.

Comment on attachment 171023 [details] Patch Attachment 171023 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14614172

Created attachment 171029 [details] Patch Trying qt-wk2 again.

Created attachment 171046 [details] Patch Updated ChangeLog descriptions.

Thanks for the review, Alex! (In reply to comment #5) > View in context: https://bugs.webkit.org/attachment.cgi?id=170762&action=review > > Looks good. > > > LayoutTests/css3/filters/resources/invalid-a-triangle-coord-with-attached-mesh.vs:9 > > +attribute vec3 a_triangleCoord; > > + > > +attribute vec4 a_position; > > + > > +uniform mat4 u_projectionMatrix; > > Remove the empty lines. Done. It looks better without the empty lines. I've also removed the empty lines from the other shaders referenced by the test file. > > > LayoutTests/css3/filters/resources/invalid-custom-attribute.vs:9 > > +attribute float my_attribute; > > + > > +attribute vec4 a_position; > > + > > +uniform mat4 u_projectionMatrix; > > Ditto. Done. > > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:-3805 > > - A24A3A9D162499FF00522745 /* CustomFilterConstants.h in Headers */ = {isa = PBXBuildFile; fileRef = A24A3A9C162499FF00522745 /* CustomFilterConstants.h */; }; > > I think you can revert the xcode changes. I need this to fixing an incorrect path to CustomFilterConstants.h in the Xcode project. The project thinks the file is in platform/graphics, not platform/graphics/filters. I need to fix the reference so that StyleResolver.cpp can find CustomFilterConstants.h. > > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:18251 > > - A24A3A9C162499FF00522745 /* CustomFilterConstants.h */, > > + A2B2AE7B16375EE500CFA50B /* CustomFilterConstants.h */, > > ditto > > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:22694 > > - A24A3A9D162499FF00522745 /* CustomFilterConstants.h in Headers */, > > + A2B2AE7C16375EE500CFA50B /* CustomFilterConstants.h in Headers */, > > ditto > > > Source/WebCore/platform/graphics/filters/CustomFilterProgram.h:63 > > // StyleCustomFilterProgram has the only implementation for the following method. That means, it casts to StyleCustomFilterProgram > > - // withouth checking the type. If you add another implementation, also add a mechanism to check for the correct type. > > + // without checking the type. If you add another implementation, also add a mechanism to check for the correct type. > > I think this comment is not needed anymore. Removed. You're right. > > > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:66 > > +bool CustomFilterValidatedProgram::validateSymbols(const Vector<ANGLEShaderSymbol>& symbols) > > Pass the mesh type as a parameter to keep this static. Done. > > > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:80 > > + if (symbol.name == "a_triangleCoord" && m_programInfo.meshType() != MeshTypeDetached) { > > Why use the != operator instead of == ? We just have two modes at this moment. Done. I was using a whitelist approach (only allow a_triangleCoord in detached meshes) vs. a blacklist approach (don't allow a_triangleCoord in attached meshes). However, since we only have two mesh types right now, it's easier to read with the blacklist approach. > > nit: Put the fastest comparison first. It will avoid making the strcmp when not needed. Done. I was assuming most of the time people will be using attached meshes. If that's true, the original arrangement is faster. However, I can't back that up with any data, so I'll put the faster comparison first :)

Comment on attachment 171046 [details] Patch Setting cq-. I need to rebase the patch.

Created attachment 171303 [details] Patch Rebased patch.

Created attachment 171306 [details] Patch Rebased patch v2. I had a merge issue in the previous one.

Comment on attachment 171306 [details] Patch Clearing flags on attachment: 171306 Committed r132903: <http://trac.webkit.org/changeset/132903>

All reviewed patches have been landed. Closing bug.

Thanks for the review, Dean!