Bug 72478 – [CSSShaders] Implement the computed style for mesh parameters of the custom() filter

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexandru Chiculita]

This patch should add the mesh parameters to the CustomFilterOperation class and also implement the CSSStyleSelector code that fills that data. Also, to be able to test the patch, the Computed Style will also be added.

[Alexandru Chiculita]

Created attachment 115354 [details]
Patch V1

[Alexandru Chiculita]

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)".

[Dean Jackson]

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 :)

[Alexandru Chiculita]

Created attachment 115781 [details]
Patch for landing

[Alexandru Chiculita]

Created attachment 115783 [details]
Patch for landing

[Alexandru Chiculita]

(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.

[WebKit Review Bot]

Comment on attachment 115783 [details]
Patch for landing

Clearing flags on attachment: 115783

Committed r100749: <http://trac.webkit.org/changeset/100749>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.