[webkit-reviews] review granted: [Bug 233618] [GPU Process] [Filters 16/23] Move the effectBoundaries from FilterEffect to Filter : [Attachment 445511] Patch

[bugzilla-daemon at webkit.org]

Cameron McCormack (:heycam) <heycam at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 233618: [GPU Process] [Filters 16/23] Move the effectBoundaries from
FilterEffect to Filter
https://bugs.webkit.org/show_bug.cgi?id=233618

Attachment 445511: Patch

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




--- Comment #5 from Cameron McCormack (:heycam) <heycam at apple.com> ---
Comment on attachment 445511
  --> https://bugs.webkit.org/attachment.cgi?id=445511
Patch

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

> Source/WebCore/ChangeLog:18
> +	   with the effectBoundaries. According to the sepcs[1], primitive sub
region

"According to the spec"
"subregion"

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:60
> +    // If there is no input effects, take the effect boundaries as unite
rect.
> +    if (!m_inputEffects.isEmpty()) {
> +	   for (auto& effect : m_inputEffects) {
> +	       auto inputPrimitiveSubregion =
effect->determineFilterPrimitiveSubregion(filter);
> +	       primitiveSubregion.unite(inputPrimitiveSubregion);
>	   }
>      } else
> -	   subregion = filter.filterRegion();
> +	   primitiveSubregion = effectBoundaries;
>  
> -    // After calling determineFilterPrimitiveSubregion on the target effect,
reset the subregion again for <feTile>.
> +    // Don't use the input's subregion for FETile.
>      if (filterType() == FilterEffect::Type::FETile)
> -	   subregion = filter.filterRegion();
> +	   primitiveSubregion = effectBoundaries;

Any reason not to adjust the condition above (`!m_inputEffects.isEmpty()`) so
that we can re-use the other `primitiveSubregion = effectBoundaries` assignment
above?

> Source/WebCore/svg/graphics/filters/SVGFilter.h:59
> +    FloatRect effectBoundaries(FilterEffect&) const override;

`const FilterEffect&`?

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:59
> +    auto boundaries = SVGLengthContext::resolveRectangle(&element,
primitiveUnits, targetBoundingBox);
> +    auto effectBoundaries = filterRegion;
> +
> +    if (element.hasAttribute(SVGNames::xAttr))
> +	   effectBoundaries.setX(boundaries.x());
> +
> +    if (element.hasAttribute(SVGNames::yAttr))
> +	   effectBoundaries.setY(boundaries.y());
> +
> +    if (element.hasAttribute(SVGNames::widthAttr))
> +	   effectBoundaries.setWidth(boundaries.width());
> +
> +    if (element.hasAttribute(SVGNames::heightAttr))
> +	   effectBoundaries.setHeight(boundaries.height());

The SVGFilterPrimitiveStandardAttributes m_x/m_y/m_width/m_height member
variables, which is what SVGLengthContext::resolveContext look up above,
already have default values of 0%/0%/100%/100%. I don't think there is a need
to check for the attributes' presence on the element and do this, since the
x/y/width/height values on boundaries should already have the correct values.