[webkit-reviews] review granted: [Bug 235338] filterRegion and outsets of referenced SVG filter are calculated correctly : [Attachment 449437] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 235338: filterRegion and outsets of referenced SVG filter are calculated
correctly
https://bugs.webkit.org/show_bug.cgi?id=235338

Attachment 449437: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 449437
  --> https://bugs.webkit.org/attachment.cgi?id=449437
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.