[webkit-reviews] review granted: [Bug 45612] SVG Filter cleanup : [Attachment 68145] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has granted Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 45612: SVG Filter cleanup
https://bugs.webkit.org/show_bug.cgi?id=45612

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=68145&action=prettypatch

> WebCore/platform/graphics/filters/FEBlend.h:33
> +    FEBLEND_MODE_UNKNOWN  = 0,

Can you remove the whitespace before the =?

> WebCore/platform/graphics/filters/FEColorMatrix.h:34
> +    FECOLORMATRIX_TYPE_UNKNOWN	   = 0,

Can you remove the whitespace before the =?

> WebCore/platform/graphics/filters/FEComponentTransfer.h:34
> +    FECOMPONENTTRANSFER_TYPE_UNKNOWN  = 0,

Can you remove the whitespace before the =?

> WebCore/platform/graphics/filters/FEComponentTransfer.h:45
> +	   , slope(0.0f)

s/0.0f/0/

> WebCore/platform/graphics/filters/FEComponentTransfer.h:67
> +	       const ComponentTransferFunction&, const
ComponentTransferFunction&, const ComponentTransferFunction&);

Move two transfer functions in one line and line up the "const" in the next
line.
Also you should really provide a name :-) red/green/blue/alpha

> WebCore/platform/graphics/filters/FEComposite.cpp:129
>      FloatRect srcRect = FloatRect(0.f, 0.f, -1.f, -1.f);

FloatRect(0, 0, -1, -1)

> WebCore/rendering/RenderTreeAsText.cpp:79
> +    double epsilon = 0.0001;

This should be static const double s_epsilon. But I don't like these magic
values. You want to use std::numeric_limits<double>::epsilon()?

> WebCore/rendering/RenderTreeAsText.h:78
> +    for (unsigned i = 0; i < v.size(); i++) {

Cache v.size() in a local variable. Rename v to vector. Use ++i.

> WebCore/rendering/SVGRenderTreeAsText.cpp:127
>  static bool hasFractions(double val)

Can you remove hasFractions from here and reuse the one in TextStream?

> WebCore/svg/graphics/filters/SVGFEDisplacementMap.h:33
> +    CHANNEL_UNKNOWN = 0,

Can you remove the whitespace before the =?

> WebCore/svg/graphics/filters/SVGFEDisplacementMap.h:42
> +    static PassRefPtr<FEDisplacementMap> create(ChannelSelectorType,
ChannelSelectorType, float);

Name these variables.

> WebCore/svg/graphics/filters/SVGFEFlood.cpp:84
> +    ts << " flood-color=\"" << floodColor().name() << "\" "

Why is .name() needed now?

> WebCore/svg/graphics/filters/SVGFEFlood.h:34
> +    static PassRefPtr<FEFlood> create(const Color&, const float&);

s/const float&/float/

> WebCore/svg/graphics/filters/SVGFEImage.h:34
> +    static PassRefPtr<FEImage> create(RefPtr<Image>,
SVGPreserveAspectRatio);

Use const SVGPreserveAspectRatio& here.
Why does it get a RefPtr<Image>, not a PassRefPtr?

> WebCore/svg/graphics/filters/SVGFEMorphology.h:32
> +    FEMORPHOLOGY_OPERATOR_UNKNOWN = 0,

Can you remove the whitespace before the =?

> WebCore/svg/graphics/filters/SVGFEOffset.h:33
> +    static PassRefPtr<FEOffset> create(float, float);

Name these variables.

Please fix these issues before landing, r=me.