[Webkit-unassigned] [Bug 47174] Moving all bounding box related calculation to RenderSVGResourceFilterPrimitive

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=47174


Dirk Schulze <krit at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #69780|review?                     |review-
               Flag|                            |




--- Comment #4 from Dirk Schulze <krit at webkit.org>  2010-10-07 00:05:30 PST ---
(From update of attachment 69780)
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...

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.