[webkit-reviews] review granted: [Bug 72478] [CSSShaders] Implement the computed style for mesh parameters of the custom() filter : [Attachment 115354] Patch V1

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 72478: [CSSShaders] Implement the computed style for mesh parameters of the
custom() filter
https://bugs.webkit.org/show_bug.cgi?id=72478

Attachment 115354: Patch V1
https://bugs.webkit.org/attachment.cgi?id=115354&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
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->meshRo
ws(), CSSPrimitiveValue::CSS_NUMBER));
> +		   if (customOperation->meshRows() !=
customOperation->meshColumns())
> +		      
meshParameters->append(primitiveValueCache->createValue(customOperation->meshCo
lumns(), 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 :)