[webkit-reviews] review denied: [Bug 76253] CSS Shaders: Parse float parameters for the custom() filter syntax : [Attachment 122618] Patch V1

[bugzilla-daemon at webkit.org]

Chris Marrin <cmarrin at apple.com> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 76253: CSS Shaders: Parse float parameters for the custom() filter syntax
https://bugs.webkit.org/show_bug.cgi?id=76253

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

------- Additional Comments from Chris Marrin <cmarrin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=122618&action=review


r- because you should really just have a single Vector parameter class with a
variable number of elements rather than a bunch of vector classes of different
sizes.

> Source/WebCore/css/CSSStyleSelector.cpp:5281
> +	       // TODO: Implement other parameters types parsing.

This should be FIXME

> Source/WebCore/platform/graphics/filters/CustomFilterParameter.h:85
> +class CustomFilterNumberVectorOf2Parameter : public CustomFilterParameter {

It would be much better to just have a single Vector class that can hold any
number of values. Then you could use it for the current vector type's you've
defined as well as all the matrix types. It would be way less code with not
much runtime cost (and extra pointer and length field). Given that we don't
expect thousands of custom filters on the page or thousands of parameters per
custom filter, this cost seems reasonable.

I think it's reasonable to keep CustomFilterNumberParameter (as opposed to just
using a vector of 1 number) if you want. But even getting rid of that wouldn't
be bad.