SVG Filter cleanup

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

The filter code was modified several times and needs a cleanup now. Otherwise the code is to hard to understand. More than it should be. I provide a first cleanup shortly. This hopefully makes it easier to continue the work on the new primitive renderer as well as the smalles repaint rect.

[Dirk Schulze]

Created attachment 67336 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 67336 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67336&action=prettypatch

Not sure if I should r+, please answer first.

> WebCore/ChangeLog:8
> +        Cleanup of FilterEffect, deleted unnecessary member variables and functinos.
typo: functions.

> WebCore/ChangeLog:9
> +        Renamed variables and functions to match name schema of SVG specification.
to match the name schema.

> WebCore/ChangeLog:11
> +        with followups.
in follow-up patches.

> WebCore/ChangeLog:26
> +        * platform/graphics/filters/FEBlend.cpp: dito.
> +        (WebCore::FEBlend::apply):
> +        * platform/graphics/filters/FEColorMatrix.cpp: dito.
> +        (WebCore::FEColorMatrix::apply):
> +        * platform/graphics/filters/FEComponentTransfer.cpp: dito.
> +        (WebCore::FEComponentTransfer::apply):
> +        * platform/graphics/filters/FEComposite.cpp: dito.
> +        (WebCore::FEComposite::apply):
> +        * platform/graphics/filters/FEGaussianBlur.cpp: dito.
> +        (WebCore::FEGaussianBlur::apply):
s/dito/ditto/ everywhere in the ChangeLog.

> WebCore/ChangeLog:52
> +        (WebCore::RenderSVGResourceFilter::buildPrimitives): Don't set primitive properties to FilterEffect, ask for them during subregion calculation.
Don't store primitive properties in FilterEffect, query them during the subregion calculation.

> WebCore/platform/graphics/filters/FilterEffect.h:101
> +    SVGFilterPrimitiveStandardAttributes* m_filterPrimitiveElement;
This is clearly a layering violation.
If we land this patch, will you have a follow-up patch ready soon, that fixes this problem?

[Dirk Schulze]

Created attachment 67776 [details]
Patch

[Dirk Schulze]

(In reply to comment #2)
> > WebCore/platform/graphics/filters/FilterEffect.h:101
> > +    SVGFilterPrimitiveStandardAttributes* m_filterPrimitiveElement;
> This is clearly a layering violation.
> If we land this patch, will you have a follow-up patch ready soon, that fixes this problem?

Thats right, it's a violation :-(
Uploaded a new patch, that concentrates more on the cleanup of unnecessary member variables and functions and follows the name schema of SVG.  
I put all SVG specific code in new public or private sections in FilterEffect.h to make clear that they should get removed.

[Dirk Schulze]

Created attachment 68014 [details]
Patch

[Dirk Schulze]

(In reply to comment #5)
> Created an attachment (id=68014) [details]
> Patch

Even if the patch looks big, the most code shares the same pattern. Removed FilterEffect inputs from FilterEffect constructor. Instead append them to a FilterEffectVector and add this to the new created filter effect.

[Nikolas Zimmermann]

Comment on attachment 68014 [details]
Patch

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.

[Dirk Schulze]

Created attachment 68041 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 68041 [details]
Patch

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!

[Dirk Schulze]

Thanks for the review. Great catches!

(In reply to comment #9)
> 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.
The first one is used to calculate the IntRect for getting the ImageData of a previous effect, the second one is to draw the result of a previous effect on top of the current effect's GraphicsContext.

Indeed the naming is bad and I plan to change it. But this is just the first cleanup patch in a row. Would like to fix it in another patch to reduce the size of this one.

> 
> 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.
See comment above, but like you can see: Int the first one we take our location and substract the location of the previous effect, and in the second one it's the other way arround.

> 
> > 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.)
Same as above, I already planned to fix it, but in another patch. It would be great to fix this naming together with calculateDrawingIntRect and calculateDrawingRect.

> Is it possible to constify it? Didn't check.
No, the imageBuffer is created in this function.

> 
> > WebCore/platform/graphics/filters/FilterEffect.h:54
> > +    FilterEffectVector& inputEffects() { return m_inputEffects; }
> 
> Is this accessor still used somewhere?
inputEffects is used by the elements, to add the input effects.


Fixed all other issues.

[Dirk Schulze]

(In reply to comment #10)
> Thanks for the review. Great catches!
> 
> (In reply to comment #9)
> > 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.
> The first one is used to calculate the IntRect for getting the ImageData of a previous effect, the second one is to draw the result of a previous effect on top of the current effect's GraphicsContext.
> 
> Indeed the naming is bad and I plan to change it. But this is just the first cleanup patch in a row. Would like to fix it in another patch to reduce the size of this one.
> 
> > 
> > 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.
> See comment above, but like you can see: Int the first one we take our location and substract the location of the previous effect, and in the second one it's the other way arround.
> 

Btw, do you have an idea how to name the both functions?

[Dirk Schulze]

Created attachment 68072 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 68072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68072&action=prettypatch

> WebCore/svg/graphics/filters/SVGFEOffset.cpp:34
> +FEOffset::FEOffset(const float& dx, const float& dy)

s/const float&/float/

> WebCore/svg/graphics/filters/SVGFEOffset.cpp:41
> +PassRefPtr<FEOffset> FEOffset::create(const float& dx, const float& dy)

s/const float&/float/

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

s/const float&/float/

> WebCore/svg/graphics/filters/SVGFEOffset.h:46
> +        FEOffset(const float&, const float&);

s/const float&/float/

Looks good to me with these fixes!

[Dirk Schulze]

Committed r67847: <http://trac.webkit.org/changeset/67847>

[Dirk Schulze]

Would like to add at least one more clean-up patch.

[Dirk Schulze]

Created attachment 68145 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 68145 [details]
Patch

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.

[Dirk Schulze]

Committed r67929: <http://trac.webkit.org/changeset/67929>

[WebKit Review Bot]

http://trac.webkit.org/changeset/67929 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/67929
http://trac.webkit.org/changeset/67930

[Dirk Schulze]

Committed r67933: <http://trac.webkit.org/changeset/67933>

[Dirk Schulze]

Comment on attachment 68145 [details]
Patch

Already committed. Clearing r-flag.