[webkit-reviews] review denied: [Bug 45612] SVG Filter cleanup : [Attachment 68014] 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 68014: Patch
https://bugs.webkit.org/attachment.cgi?id=68014&action=review

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

> WebCore/ChangeLog:9
> +	   All effect inputs are stored in a FilterEffectVector on FilterEffect
instead of
> +	   passing them via the constructors of every single effect type.

All effect inputs are stored in a Vector in FIlterEffect instead of passing
them via constructors to every effect type.

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

s/to/in/

> WebCore/ChangeLog:18
> +	   renamed to repaintRectInLocalCoordinate since this is its properly
meaning.

s/properly/proper/

> WebCore/ChangeLog:23
> +	   * platform/graphics/cairo/GraphicsContextCairo.cpp: scaledSubRegion
was renamed to repaintRectInLocalCoordinate

s/Coordinate/Coordinates./

> WebCore/ChangeLog:31
> +	   * platform/graphics/filters/FEColorMatrix.cpp: Same as on FEBlend.

Same changes like FEBlend.

> WebCore/ChangeLog:37
> +	   * platform/graphics/filters/FEComponentTransfer.cpp: Same as on
FEBlend.

Just use "Ditto" here.

> WebCore/ChangeLog:43
> +	   * platform/graphics/filters/FEComposite.cpp: Same as on FEBlend.

Ditto.

> WebCore/ChangeLog:49
> +	   * platform/graphics/filters/FEGaussianBlur.cpp: Same as on FEBlend.

Ditto.

> WebCore/ChangeLog:60
> +	   (WebCore::FilterEffect::calculateDrawingIntRect): Takes
repaintRectInLocalCoordinate now.

s/Coordinate/Coordinates/

> WebCore/ChangeLog:63
> +	   * platform/graphics/filters/FilterEffect.h: Changed names to match
name schema. Removed unnecessary member variables and funtions.

s/funtions/functions/.

> WebCore/ChangeLog:91
> +	   (WebCore::SourceGraphic::determineFilterPrimitiveSubregion): ditto.

s/ditto/Ditto.

> WebCore/ChangeLog:94
> +	   * rendering/RenderSVGResourceFilter.cpp: Consider renamed functions
in FilterEffect.

Adapt to renames in FilterEffect.

> WebCore/ChangeLog:99
> +	   * svg/SVGFEColorMatrixElement.cpp: Same as on SVGFEBlendElement.

s/Same as....*/Ditto.

> WebCore/ChangeLog:101
> +	   * svg/SVGFEComponentTransferElement.cpp: Same as on
SVGFEBlendElement.

Ditto.

> WebCore/ChangeLog:103
> +	   * svg/SVGFECompositeElement.cpp: Same as on SVGFEBlendElement.

Ditto.

> WebCore/ChangeLog:105
> +	   * svg/SVGFEConvolveMatrixElement.cpp: Same as on SVGFEBlendElement.

Ditto.

> WebCore/ChangeLog:107
> +	   * svg/SVGFEDiffuseLightingElement.cpp: Same as on SVGFEBlendElement.


Ditto.

> WebCore/ChangeLog:109
> +	   * svg/SVGFEDisplacementMapElement.cpp: Same as on SVGFEBlendElement.


Ditto. (Several places which should also just say ditto below in the
ChangeLog..)

> WebCore/ChangeLog:174
> +	   (WebCore::FETile::determineFilterPrimitiveSubregion): Renamed to
match name schema.

s/schema/scheme/ in mutiple places in the ChangeLog.

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:986
> +    RefPtr<FilterEffect> blur = FEGaussianBlur::create(sd, sd);

You might want to change the 'sd' variable to a more reasonable name here, even
if your patch doesn't introduce/change it.

> WebCore/platform/graphics/filters/FEBlend.cpp:91
> +    FilterEffectVector inputEffects = inputFilterEffects();

Ouch, that's a bad idea, you're copying the vector here.
You want to use "FilterEffectVector& inputEffects = inputFilterEffects();".

I'd propose to add accessor for "get input effect at index", so the could could
read lke:

ASSERT(inputEffects().size() == 2);
FilterEffect* in = inputFilterEffect(0);
FilterEffect* in2 = inputFilterEffect(1);
..

> WebCore/platform/graphics/filters/FEBlend.cpp:171
> +    FilterEffectVector inputEffects = inputFilterEffects();

Same here, do NOT copy.
Better add the "inputFilterEffect(unsigned ..)" function.

> WebCore/platform/graphics/filters/FEColorMatrix.cpp:159
> +    FilterEffect* in = inputFilterEffects()[0].get();

This also looks awkward, hence my proposal to add inputFilterEffects(unsigned
number).

> WebCore/platform/graphics/filters/FEColorMatrix.cpp:241
> +    inputFilterEffects()[0]->externalRepresentation(ts, indent + 1);

Ok, this pattern is used everywhere, I'm not going to repeat above comments for
the rest.

> WebCore/platform/graphics/filters/FilterEffect.cpp:52
> +    if (!m_inputEffects.size())
> +	   uniteRect = filter->filterRegion();
> +
> +    FilterEffectVector::const_iterator it = m_inputEffects.begin();
> +    const FilterEffectVector::const_iterator end = m_inputEffects.end();
> +    for (; it != end; ++it)
> +	  
uniteRect.unite(it->get()->determineFilterPrimitiveSubregion(filter));

This could be simplifed:
unsigned size = m_inputEffects.size();
if (!size)
   uniteRect = filter->filterRegion();

for (unsigned i = 0; i < size; ++i)
   
uniteRect.unite(m_inputEffects.at(i)->determineFIlterPrimitiveSubregion(filter)
);
...

(I've been told the vector-by-index access is faster than iterators, not sure
about that though.

> WebCore/platform/graphics/filters/FilterEffect.cpp:60
> +    IntPoint location =
roundedIntPoint(FloatPoint(repaintRectInLocalCoordinates().x() -
effectRect.x(),

I wouldn't call repaintRectInLocalCoordinates() twice. How about:

FloatPoint location = repaintRectInLocalCoordinates.location();
location.move(-effectRect.x(), -effectRect.y());
return IntRect(roundedIntPoint(location), resultImage()->size());

Question: Are you sure resultImage() is always not null here? Either chec or
asser it.

> WebCore/platform/graphics/filters/FilterEffect.cpp:67
> +    FloatPoint startPoint = FloatPoint(srcRect.x() -
repaintRectInLocalCoordinates().x(), srcRect.y() -
repaintRectInLocalCoordinates().y());

Same as above.
FloatPoint location = repaintRectInLocalCoordinates().location();
return FloatRect(FloatPoint(srcRect.x() - location.x(), srcRect.y() -
location.y()), srcRect.size());

> WebCore/platform/graphics/filters/FilterEffect.cpp:76
> +    if (m_effectBuffer.get())

I'd reverse the check:
if (!m_effectBuffer)
    return 0;
return m_effectBuffer->context();

Note: No need to call .get() as well.

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

I'd call the function setInputEffects (FilterEffects sounds redundant). And let
it take a const reference!
void setInputEffects(const FilterEffectVector& inputEffects) { m_inputEffects =
inputEffects; }
Please remove the trailing whitespace.

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

This is awkward. You either want:
const FilterEffectVector& inputEffects() const { return m_inputEffects;} for
read only access or
FilterEffectVector& inputEffects() { return m_inputEffects; } for read write
access.

My proposal is to add:
FilterEffect* inputFilterEffect(unsigned number) const
{
    ASSERT(number < m_inputEffects.size());
    return m_inputEffects.at(number).get();
}

This also removes the need for the ASSERT(foo.size() == xx) everywhere in the
client code.

> WebCore/platform/graphics/filters/FilterEffect.h:103
> +    mutable OwnPtr<ImageBuffer> m_effectBuffer;
> +    mutable FilterEffectVector m_inputEffects;

Why are those mutable? Which const method needs to modify it? Change the
design.

> WebCore/svg/SVGFEBlendElement.cpp:96
> +    FilterEffectVector inputEffects;
> +    inputEffects.append(input1);
> +    inputEffects.append(input2);
>  
> -    return FEBlend::create(input1, input2,
static_cast<BlendModeType>(mode()));
> +    PassRefPtr<FilterEffect> effect =
FEBlend::create(static_cast<BlendModeType>(mode()));
> +    effect->setInputFilterEffects(inputEffects);
> +    return effect;

This is ineffcient. You want to use
RefPtr<FilterEffect> effect =
FEBlend::Create(static_cast<BlendModeType>(mode());
FilterEffectVector& inputEffects = effect->inputEffects();
inputEffects.append(input1);
inputEffects.append(input2);
return effect.release();

Note that you are not allowed to store the passed PassRefPtr in a PassRefPtr,
you need to use a RefPtr here, and release it afterwards!

> WebCore/svg/SVGFEColorMatrixElement.cpp:126
> +    FilterEffectVector inputEffects;

Same as above, affects all SVGFE* files you've touched! Won't repeat the
comments for the following ones.

> WebCore/svg/graphics/filters/SVGFEMerge.cpp:61
> +    it = inputEffects.begin();

Use:
unsigned size = m_mergeInputs.size();
for (unsigned i = 0; i < size; ++i) {
   FilterEffect* input = inputEffect(i);
   filterContext->drawImageBuffer(input->resultImage, DeviceColorSpace,
calculateDrawingRect(input->repaintRectInLocalCoordinates()));
}

Looks cleaner.

Okay, that's my first iteration, if you upload a fixed version, I'm going to
recheck in detail.