[webkit-reviews] review denied: [Bug 45612] SVG Filter cleanup : [Attachment 68041] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied 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 68041: Patch
https://bugs.webkit.org/attachment.cgi?id=68041&action=review

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

> WebCore/ChangeLog:8
> +	   All effect inputs are stored in a Vector in FIlterEffect instead of
passing them via constructors to every effect type.

s/FI/Fi/

> WebCore/ChangeLog:9
> +	   This simplifies the primitive subregion logic and concentrates it in
determineFilterPrimitiveSubregion.

s/concentrates/centralizes/ - not sure the former word can be used in this
context in english :-)

> WebCore/ChangeLog:17
> +	   renamed to repaintRectInLocalCoordinate since this is its proper
meaning.

InLocalCoordinates.

> WebCore/ChangeLog:20
> +	   No new tests added since the functionality didn't changed.

didn't change or hasn't changed.

> WebCore/platform/graphics/filters/FEComposite.cpp:35
> +FEComposite::FEComposite(const CompositeOperationType& type, const float&
k1, const float& k2, const float& k3, const float& k4)

While you're at it, s/const float&/float/ please :-)

> WebCore/platform/graphics/filters/FEComposite.cpp:213
> +    inputEffect(0)->externalRepresentation(ts, indent + 1);

inputEffect(1)!

> WebCore/platform/graphics/filters/FEComposite.h:45
> +    static PassRefPtr<FEComposite> create(const CompositeOperationType&,
const float&, const float&, const float&, const float&);

s/const float&/float/

> WebCore/platform/graphics/filters/FEComposite.h:67
> +    FEComposite(const CompositeOperationType&, const float&, const float&,
const float&, const float&);

s/const float&/float/

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:41
> +FEGaussianBlur::FEGaussianBlur(const float& x, const float& y)

s/const float&/float/

> WebCore/platform/graphics/filters/FEGaussianBlur.h:33
> +    static PassRefPtr<FEGaussianBlur> create(const float&, const float&);

s/const float&/float/

> WebCore/platform/graphics/filters/FEGaussianBlur.h:48
> +    FEGaussianBlur(const float&, const float&);

s/const float&/float/

> WebCore/platform/graphics/filters/FilterEffect.cpp:52
> +    if (!size)
> +	   uniteRect = filter->filterRegion();
> +
> +    for (unsigned i = 0; i < size; ++i)
> +	  
uniteRect.unite(m_inputEffects.at(i)->determineFilterPrimitiveSubregion(filter)
);

I'd prefer:
if (!size)
    uniteRect = filter->filterRegion();
else {
     for (unsigned i = 0; i < size; ++i)
	uniteRect.unite(m_inputEffects.at(i)->.....);
}

To make it a bit more obvious.

> WebCore/platform/graphics/filters/FilterEffect.cpp:58
> +IntRect FilterEffect::calculateDrawingIntRect(const FloatRect& effectRect)
const

Hm, I don't see why calculateDrawingIntRect and calculateDrawingRect are not
similar at all.
I'd expect that calculateDrawingIntRect ==
roundedIntRect(calculateDrawingRect).

Can you explain the difference? If there's an important difference, the
functions have to be renamed, it looks confusing.

calculateDrawingIntRect returns IntRect(roundedIntPoint(repaintRect.x() -
effectRect.x(), repaintRect.y() - effectRect.y()), m_effectBuffer->size());
calculateDrawingRect returns FloatRect(FloatPoint(srcRect.x() -
repaintRect.x(), srcRect.y() - repaintRect.y()), srcRect.size());

There is a difference that's not obvious to me from the naming of the
functions.

> WebCore/platform/graphics/filters/FilterEffect.h:47
> +    ImageBuffer* resultImage() { return m_effectBuffer.get(); }

This function should be const.

> WebCore/platform/graphics/filters/FilterEffect.h:52
> +    GraphicsContext* getEffectContext();

s/getEffectContext/effectContext/ - we don't use the get prefix anywhere in
WebKit (at least we shouldn't.)
Is it possible to constify it? Didn't check.

> WebCore/platform/graphics/filters/FilterEffect.h:54
> +    FilterEffectVector& inputEffects() { return m_inputEffects; }

Is this accessor still used somewhere?

> WebCore/platform/graphics/filters/FilterEffect.h:62
> +    bool isAlphaImage() { return m_alphaImage; }

Add const.

> WebCore/platform/graphics/filters/FilterEffect.h:65
> +    FloatRect repaintRectInLocalCoordinates() { return
m_repaintRectInLocalCoordinates; }

Add const.

> WebCore/platform/graphics/filters/FilterEffect.h:71
> +    virtual bool isSourceInput() { return false; }

Add const.

> WebCore/platform/graphics/filters/FilterEffect.h:78
> +    bool hasX() { return m_hasX; }

Add const.

> WebCore/platform/graphics/filters/FilterEffect.h:81
> +    bool hasY() { return m_hasY; }

Add const.

> WebCore/platform/graphics/filters/FilterEffect.h:84
> +    bool hasWidth() { return m_hasWidth; }

Add const.

> WebCore/platform/graphics/filters/FilterEffect.h:87
> +    bool hasHeight() { return m_hasHeight; }

Add const.

> WebCore/platform/graphics/filters/FilterEffect.h:93
> +    FloatRect filterPrimitiveSubregion() { return
m_filterPrimitiveSubregion; }

Add const.

> WebCore/svg/SVGFEBlendElement.cpp:91
> +    FilterEffectVector& inputEffects = effect->inputEffects();

Call inputEffects.reverseCapacity(2) before appending the two effects, to avoid
reallocations.

> WebCore/svg/SVGFEColorMatrixElement.cpp:127
> +    FilterEffectVector& inputEffects = effect->inputEffects();

No need for the local variable here, just call
effect->inputEffects().append(input1);

> WebCore/svg/SVGFEComponentTransferElement.cpp:87
> +    FilterEffectVector& inputEffects = effect->inputEffects();

No need for the local variable here, just call
effect->inputEffects().append(input1);

> WebCore/svg/SVGFECompositeElement.cpp:115
> +    FilterEffectVector& inputEffects = effect->inputEffects();

Call inputEffects.reverseCapacity(2) before appending the two effects, to avoid
reallocations.

> WebCore/svg/SVGFEConvolveMatrixElement.cpp:158
> +    FilterEffectVector& inputEffects = effect->inputEffects();

No need for the local variable here, just call
effect->inputEffects().append(input1);

> WebCore/svg/SVGFEDiffuseLightingElement.cpp:118
> +    FilterEffectVector& inputEffects = effect->inputEffects();

No need for the local variable here, just call
effect->inputEffects().append(input1);

> WebCore/svg/SVGFEDisplacementMapElement.cpp:107
> +    FilterEffectVector& inputEffects = effect->inputEffects();

Call inputEffect.reverseCapacity(2) before appending the two effects, to avoid
reallocations.

> WebCore/svg/SVGFEGaussianBlurElement.cpp:91
> +    FilterEffectVector& inputEffects = effect->inputEffects();

No need for the local variable here, just call
effect->inputEffects().append(input1);

> WebCore/svg/SVGFEMorphologyElement.cpp:105
> +    FilterEffectVector& inputEffects = effect->inputEffects();

No need for the local variable here, just call
effect->inputEffects().append(input1);

> WebCore/svg/SVGFEOffsetElement.cpp:90
> +    FilterEffectVector& inputEffects = effect->inputEffects();

No need for the local variable here, just call
effect->inputEffects().append(input1);

> WebCore/svg/SVGFESpecularLightingElement.cpp:124
> +    FilterEffectVector& inputEffects = effect->inputEffects();

No need for the local variable here, just call
effect->inputEffects().append(input1);

> WebCore/svg/SVGFETileElement.cpp:66
> +    FilterEffectVector& inputEffects = effect->inputEffects();

No need for the local variable here, just call
effect->inputEffects().append(input1);

> WebCore/svg/graphics/filters/SVGFEDiffuseLighting.cpp:34
> +    : FELighting(DiffuseLighting, lightingColor, surfaceScale,
diffuseConstant, 0.0f, 0.0f, kernelUnitLengthX, kernelUnitLengthY, lightSource)


s/0.0f/0/g

> WebCore/svg/graphics/filters/SVGFEDisplacementMap.cpp:36
> +FEDisplacementMap::FEDisplacementMap(ChannelSelectorType xChannelSelector, 
ChannelSelectorType yChannelSelector, const float& scale)

superfluous whitespace. s/const float&/float/

> WebCore/svg/graphics/filters/SVGFEDisplacementMap.cpp:44
> +PassRefPtr<FEDisplacementMap> FEDisplacementMap::create(ChannelSelectorType
xChannelSelector,

s/const float&/float/

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

s/const float&/float/

> WebCore/svg/graphics/filters/SVGFEDisplacementMap.h:58
> +	   FEDisplacementMap(ChannelSelectorType, ChannelSelectorType, const
float&);

s/const float&/float/

> WebCore/svg/graphics/filters/SVGFEMerge.cpp:40
> +    return adoptRef(new FEMerge());

s/FEMerge()/FEMerge/

> WebCore/svg/graphics/filters/SVGFEMerge.cpp:46
> +    ASSERT(!size);

Oops, ASSERT(size > 0) is what you want.

> WebCore/svg/graphics/filters/SVGFEMerge.cpp:58
> +    for (unsigned i = 0; i < size; ++i) {

Where is size defined?

> WebCore/svg/graphics/filters/SVGFEMerge.cpp:74
> +    ASSERT(!size);

ASSERT(size > 0)!

> WebCore/svg/graphics/filters/SVGFESpecularLighting.cpp:35
> +    : FELighting(SpecularLighting, lightingColor, surfaceScale, 0.0f,
specularConstant, specularExponent, kernelUnitLengthX, kernelUnitLengthY,
lightSource)

s/0.0f/0/

> WebCore/svg/graphics/filters/SVGFETile.cpp:41
> +    return adoptRef(new FETile());

s/FETile()/FETile/

> WebCore/svg/graphics/filters/SVGFETile.cpp:47
> +    inputEffects()[0].get()->determineFilterPrimitiveSubregion(filter);

inputEffect(0)->determine...

> WebCore/svg/graphics/filters/SVGFETile.cpp:82
> +    matrix.translate(in->repaintRectInLocalCoordinates().x() -
repaintRectInLocalCoordinates().x(), in->repaintRectInLocalCoordinates().y() -
repaintRectInLocalCoordinates().y());

Use two lines here please, hard to read.


Almost there, good job Dirk!