[webkit-reviews] review denied: [Bug 47174] Moving all bounding box related calculation to RenderSVGResourceFilterPrimitive : [Attachment 69780] patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Zoltan Herczeg
<zherczeg at webkit.org>'s request for review:
Bug 47174: Moving all bounding box related calculation to
RenderSVGResourceFilterPrimitive
https://bugs.webkit.org/show_bug.cgi?id=47174

Attachment 69780: patch
https://bugs.webkit.org/attachment.cgi?id=69780&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=69780&action=review

Good start. But you still forgot some of the things we spoke of, like
calculating the subregions in buildPrimitives and not to make the functions in
RenderSVGResourceFilterPrimitives static.

> WebCore/platform/graphics/filters/Filter.h:50
>	   virtual bool effectBoundingBoxMode() const = 0;

Do we still need this function?

> WebCore/rendering/RenderSVGResourceFilter.cpp:200
> +    FloatRect subRegion = lastEffect->repaintRectInLocalCoordinates();
>      // At least one FilterEffect has a too big image size,
>      // recalculate the effect sizes with new scale factors.
> -    if (!fitsInMaximumImageSize(filterData->filter->maxImageSize(), scale))
{
> +    if (!fitsInMaximumImageSize(subRegion.size(), scale)) {

Interesting, sound like a good idea to take repaintRectInLocalCoordinates
(alias absolutePaintRect). But please name the variable paintRect.

> WebCore/rendering/RenderSVGResourceFilter.cpp:202
> +	  
RenderSVGResourceFilterPrimitive::determineFilterPrimitiveSubregion(lastEffect,
filterData->filter.get());

We already talked about it on IRC. I wouldn't make this static. Its better to
manage it in the class.

> WebCore/rendering/RenderSVGResourceFilterPrimitive.cpp:47
> +    if (effect->isSourceInput()) {
> +	   // For SourceAlpha and SourceGraphic
> +	   FloatRect clippedSourceRect = filter->sourceImageRect();
> +	   if (filter->sourceImageRect().x() < filter->filterRegion().x())
> +	       clippedSourceRect.setX(filter->filterRegion().x());
> +	   if (filter->sourceImageRect().y() < filter->filterRegion().y())
> +	       clippedSourceRect.setY(filter->filterRegion().y());
> +	   effect->setFilterPrimitiveSubregion(clippedSourceRect);
> +	   clippedSourceRect.scale(filter->filterResolution().width(),
filter->filterResolution().height());
> +	   effect->setRepaintRectInLocalCoordinates(clippedSourceRect);
> +	   return filter->filterRegion();
> +    }

This isn't needed anymore

> WebCore/rendering/RenderSVGResourceFilterPrimitive.cpp:64
> +    if (!effect->isTile()) {
> +	   // FETurbulence, FEImage and FEFlood don't have input effects, take
the filter region as unite rect.
> +	   if (unsigned numberOfInputEffects = effect->inputEffects().size()) {

> +	       for (unsigned i = 0; i < numberOfInputEffects; ++i)
> +		  
uniteRect.unite(determineFilterPrimitiveSubregion(effect->inputEffect(i),
filter));
> +	   } else
> +	       uniteRect = filter->filterRegion();
> +    }
> +    else {
> +	   determineFilterPrimitiveSubregion(effect->inputEffect(0), filter);
> +	   uniteRect = filter->filterRegion();
> +    }

Maybe we should move this to an inline

> WebCore/rendering/RenderSVGResourceFilterPrimitive.cpp:95
> +    if (filter->effectBoundingBoxMode()) {
> +	   newSubRegion = uniteRect;
> +	   // Avoid the calling of a virtual method several times.
> +	   FloatRect targetBoundingBox = filter->sourceImageRect();
> +
> +	   if (effect->hasX())
> +	       newSubRegion.setX(targetBoundingBox.x() +
subRegionBoundingBox.x() * targetBoundingBox.width());
> +
> +	  if (effect->hasY())
> +	       newSubRegion.setY(targetBoundingBox.y() +
subRegionBoundingBox.y() * targetBoundingBox.height());
> +
> +	   if (effect->hasWidth())
> +	       newSubRegion.setWidth(subRegionBoundingBox.width() *
targetBoundingBox.width());
> +
> +	   if (effect->hasHeight())
> +	       newSubRegion.setHeight(subRegionBoundingBox.height() *
targetBoundingBox.height());
> +    } else {
> +	   if (!effect->hasX())
> +	       newSubRegion.setX(uniteRect.x());
> +
> +	   if (!effect->hasY())
> +	       newSubRegion.setY(uniteRect.y());
> +
> +	   if (!effect->hasWidth())
> +	       newSubRegion.setWidth(uniteRect.width());
> +
> +	   if (!effect->hasHeight())
> +	       newSubRegion.setHeight(uniteRect.height());
> +    }
> +

We really don't want the effects to store hasX, hasY and so on anymore. You can
ask your node() directly now.

> WebCore/rendering/RenderSVGResourceFilterPrimitive.cpp:105
> +    FloatRect scaledSubRegion(newSubRegion);
> +    scaledSubRegion.scale(filter->filterResolution().width(),
filter->filterResolution().height());
> +    effect->setRepaintRectInLocalCoordinates(scaledSubRegion);
> +    return newSubRegion;
>  }

I already begun to give the variables better name. Maybe you can continue this
here.

> WebCore/rendering/RenderSVGResourceFilterPrimitive.h:43
> +    RenderSVGResourceFilterPrimitive(SVGFilterPrimitiveStandardAttributes*
filterPrimitiveElement)
> +	   : RenderSVGHiddenContainer(filterPrimitiveElement)
> +    {
> +    }
> +

I would move the ctor to the cpp. Looks more like the other classes. Not sure
if we have a style rule for it yet. But still..

> WebCore/rendering/RenderSVGResourceFilterPrimitive.h:46
> +    static FloatRect determineFilterPrimitiveSubregion(FilterEffect* effect,
Filter* filter);

hm...