[webkit-reviews] review granted: [Bug 45612] SVG Filter cleanup : [Attachment 68145] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Sep 20 22:15:12 PDT 2010
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.
More information about the webkit-reviews
mailing list