filterRegion and outsets of referenced SVG filter are calculated incorrectly

[Said Abou-Hallawa]

Created attachment 449435 [details]
test case

Open the attached test case.

Expected: There should be a drop-shadow effect around the first four boxes. There should be kind of a black border around the fifth box.
Result: None of the five boxes has any effect.

The filterRegion of the referenced is currently set to the targetBoundingBox. Given the geometry of the SVGFilterElement and given the targetBoundingBox, the filterRegion should be calculated using SVGLengthContext::resolveRectangle().
The outsets of the SVGFilter is calculated by getting the outsets of the lastEffect. The outsets of the SVGFilter should be the outsets of the all the effects which are involved in producing the lastEffect.

[Said Abou-Hallawa]

Created attachment 449437 [details]
Patch

[Darin Adler]

Comment on attachment 449437 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        filterRegion and outsets of referenced SVG filter are calculated correctly

Should this title say "incorrectly"? I don’t understand

> Source/WebCore/platform/graphics/filters/FEOffset.cpp:78
> +IntOutsets FEOffset::outsets(const Filter& filter) const
> +{
> +    auto offset = expandedIntSize(filter.resolvedSize({ m_dx, m_dy }));
> +    
> +    IntOutsets outsets;
> +    if (offset.height() < 0)
> +        outsets.setTop(-offset.height());
> +    else
> +        outsets.setBottom(offset.height());
> +
> +    if (offset.width() < 0)
> +        outsets.setLeft(-offset.width());
> +    else
> +        outsets.setRight(offset.width());
> +    return outsets;
> +}

Paragraphing here is a little strange. Not sure why we have the second blank line where we do. Maybe I’d add more lines or remove that one.

> Source/WebCore/svg/graphics/filters/SVGFilter.cpp:168
> +    for (auto& term : m_expression) {
> +        auto& effect = term.effect;
> +        outsets += effect->outsets(*this);
> +    }

Local variable isn’t really helping here. I would write it tighter like this:

    IntOutsets outsets;
    for (auto& term : m_expression)
        outsets += term.effect->outsets(*this);
    return outsets;

I also don’t know why we need to assert !isEmpty. This algorithm looks like it works fine for an empty vector.

[Said Abou-Hallawa]

Created attachment 449462 [details]
Patch

[EWS]

Committed r288183 (246164@main): <https://commits.webkit.org/246164@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449462 [details].

[Radar WebKit Bug Importer]

<rdar://problem/87758495>