Bug 47174

Created attachment 69780 [details] patch This patch is tested and working on mac leopard. I think this is a good first step, since it keeps the logic (in a refactored way). There is no more regression, and I think it is finally ready for review.

Attachment 69780 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/rendering/RenderSVGResourceFilterPrimitive.cpp:61: An else should appear on the same line as the preceding } [whitespace/newline] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Can you wait before you update the patch please? The patch on bug 31370 is nearly done.

Comment on attachment 69780 [details] patch 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...

> We already talked about it on IRC. I wouldn't make this static. Its better to manage it in the class. Me neither. But moving all the logic to one file caused enough regression fails to fix them in one patch. That is why I tried to stress that the static function is temporary.

(In reply to comment #5) > > We already talked about it on IRC. I wouldn't make this static. Its better to manage it in the class. > > Me neither. But moving all the logic to one file caused enough regression fails to fix them in one patch. That is why I tried to stress that the static function is temporary. If you have a followup that fixes this, I'm fine with a temporary static function. Over read this in you ChangeLog, sorry. To my last comment. You can't use absolutePaintRect, since it is calculated during the drawing phase (on calling FilterEffect::apply(Filter*)). You have to use maxEffectRect, the subregion in absolute coordinate space.

Created attachment 71283 [details] patch 2 Still keeping the static function for the first step.

Comment on attachment 71283 [details] patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=71283&action=review Just style issues remaining. Great Patch! Looking forward to see a fixed version of this patch and a followup with turning determineFilterPrimitiveSubregion into a member of the Renderer. r- for now. > WebCore/platform/graphics/filters/ImageBufferFilter.h:-46 > - virtual FloatSize maxImageSize() const { return FloatSize(); } This file doesn't exist any longer. Revert this file in your local copy and update to trunk. Shouldn't cause any problems. > WebCore/rendering/RenderSVGResourceFilterPrimitive.cpp:39 > + FloatRect subRegionBoundingBox = effect->effectBoundaries(); > + FloatRect newSubRegion = subRegionBoundingBox; We use just subregion, not subRegion now. > WebCore/rendering/RenderSVGResourceFilterPrimitive.cpp:49 > + } > + else { } else { > WebCore/rendering/RenderSVGResourceFilterPrimitive.h:46 > + // These two arguments are depend on the RenderObject s/depend/dependent/ or just they depend on... > WebCore/svg/graphics/filters/SVGFilter.cpp:@ > SVGFilter::SVGFilter(const AffineTransform& absoluteTransform, const FloatRect& Remove the SVGFEImage.h include in this file.

Created attachment 71533 [details] patch Thanks for the review. The updated patch is attached.

Comment on attachment 71533 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=71533&action=review LGTM. r=me. Please fix the name of absoluteSubRegion. You can do it before uploading it. > WebCore/rendering/RenderSVGResourceFilterPrimitive.cpp:85 > + FloatRect absoluteSubRegion = filter->mapLocalRectToAbsoluteRect(subregion); s/absoluteSubRegion/absoluteSubregion/

Landed in http://trac.webkit.org/changeset/70296 Closing bug.