Bug 72478

Created attachment 115354 [details] Patch V1

Comment on attachment 115354 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=115354&action=review > Source/WebCore/css/CSSParser.cpp:6649 > + return 0; This was needed to avoid parsing the invalid syntax "custom(none, 10, 10)" as "custom(none, 10)".

Comment on attachment 115354 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=115354&action=review Looks good. I'd like to see the verbose computed style, but if you see other similar properties not doing it then I'm happy. > LayoutTests/css3/filters/custom-filter-property-computed-style-expected.txt:41 > +PASS removeBaseURL(subRule.cssText) is 'custom(url(vertex.shader), 10)' See below. I think we should return 10 10 for the computed value here. > LayoutTests/css3/filters/custom-filter-property-computed-style-expected.txt:59 > +PASS removeBaseURL(subRule.cssText) is 'custom(none url(fragment.shader), detached)' And similarly here should return "1 1 detached". In general I like having the computed style being a verbose form of the input. > LayoutTests/css3/filters/script-tests/custom-filter-property-parsing-invalid.js:44 > testInvalidFilterRule("To many parameter values", "custom(url(shader), p1 1 2 3 4 5)"); Another typo here in the unchanged code. > Source/WebCore/ChangeLog:12 > + Also fixed a case where "custom(none, 10, 10 filter-box)" was actually > + treated as "custom(none, 10)". was incorrectly treated > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:760 > + if (customOperation->meshRows() > 0 || customOperation->meshColumns() > 0) { > + meshParameters->append(primitiveValueCache->createValue(customOperation->meshRows(), CSSPrimitiveValue::CSS_NUMBER)); > + if (customOperation->meshRows() != customOperation->meshColumns()) > + meshParameters->append(primitiveValueCache->createValue(customOperation->meshColumns(), CSSPrimitiveValue::CSS_NUMBER)); > + } I think the computed style for a single mesh input should still give both x and y values. This is what we do for things like translate(10px) which is computed as translate(10px, 0). > Source/WebCore/css/CSSStyleSelector.cpp:5507 > + // The second value might be the mesh box or the list of paramters: typo parameters > Source/WebCore/platform/graphics/filters/CustomFilterOperation.h:57 > + static PassRefPtr<CustomFilterOperation> create(PassRefPtr<StyleShader> vertexShader, PassRefPtr<StyleShader> fragmentShader, > + unsigned meshRows, unsigned meshColumns, MeshBoxType meshBoxType, MeshType meshType) I think here we tend to indent to align with the first parameter. (I'm not sure) > Source/WebCore/platform/graphics/filters/CustomFilterOperation.h:88 > return m_vertexShader.get() == other->m_vertexShader.get() > - && m_fragmentShader.get() == other->m_fragmentShader.get(); > + && m_fragmentShader.get() == other->m_fragmentShader.get() > + && m_meshRows == other->m_meshRows > + && m_meshColumns == other->m_meshColumns > + && m_meshBoxType == other->m_meshBoxType > + && m_meshType == other->m_meshType; > } > Same here (maybe :) > Source/WebCore/platform/graphics/filters/CustomFilterOperation.h:91 > + CustomFilterOperation(PassRefPtr<StyleShader> vertexShader, PassRefPtr<StyleShader> fragmentShader, > + unsigned meshRows, unsigned meshColumns, MeshBoxType meshBoxType, MeshType meshType) > : FilterOperation(CUSTOM) .. and here (again maybe :)

Created attachment 115781 [details] Patch for landing

Created attachment 115783 [details] Patch for landing

(In reply to comment #3) > (From update of attachment 115354 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115354&action=review > > Looks good. I'd like to see the verbose computed style, but if you see other similar properties not doing it then I'm happy. Thanks for the review. I converted the patch to output all the parameters, even if they are only the default values and could be omitted. It's easier to parse the computed style in JS.

Comment on attachment 115783 [details] Patch for landing Clearing flags on attachment: 115783 Committed r100749: <http://trac.webkit.org/changeset/100749>

All reviewed patches have been landed. Closing bug.