[Webkit-unassigned] [Bug 73643] SVG filters incorrectly move elements

[bugzilla-daemon at webkit.org]

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





--- Comment #11 from Dirk Schulze <krit at webkit.org>  2011-12-07 23:48:40 PST ---
(From update of attachment 118088)
View in context: https://bugs.webkit.org/attachment.cgi?id=118088&action=review

This is just a code review. I have to check the functionality again in a second step.

> LayoutTests/ChangeLog:11
> +        * svg/filters/filter-placement-issue.svg: Added.

Is this a manual test? You miss the results for this test otherwise (I'd like to have an automated test with pixel result). Also, do you have examples for clipping and masking at well? Won't this code influence existing results? Don't you need to update pixel results?

> Source/WebCore/platform/graphics/filters/FETile.cpp:58
> +    FloatRect tileRect = enclosingIntRect(in->maxEffectRect());

Why don't you make it an IntRect?

> Source/WebCore/platform/graphics/filters/FETile.cpp:67
> +    // TODO: Clamp image size?

This should be a statement and use FIXME. Also, is it correct to remove the SVGImageBufferTools::createImageBuffer call? Is the following code a complete replacement of the functionality?

> Source/WebCore/platform/graphics/filters/FETile.cpp:68
> +    OwnPtr<ImageBuffer> tileImage = ImageBuffer::create(IntSize((int)tileRect.width(), (int)tileRect.height()), ColorSpaceDeviceRGB, filter()->renderingMode());

the (int)'s would be unnecessary if you use IntRect, same for IntSize.

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:-189
> -        // The save/restore pair is needed for clipToImageBuffer - it doesn't work without it on non-Cg platforms.
> -        GraphicsContextStateSaver stateSaver(*maskContext);

You seem to ignore this comment. This is not addressed somewhere later. This code was added for Cairo and Skia ports.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:231
>      absoluteDrawingRegion.scale(scale.width(), scale.height());

Do we still need this rect?

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:251
> +    // Context is now set up to draw in local coordinates. This shouldn't work.

You should check this first. I don't think that such a comment should be in trunk :)

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:252
> +    // sourceGraphicContext->clearRect(FloatRect(FloatPoint(), absoluteDrawingRegion.size()));

Don't use commented out code. Please be sure that this works correctly and why it works.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:72
> +    // This is done in absolute coordinates

Please use sentences and a dot at the end of a phrase.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:77
> +    // This happens in local coordinates

Ditto.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:78
> +    imageContext->scale(FloatSize(((float)clampedSize.width()) / paintRect.width(), ((float)clampedSize.height()) / paintRect.height()));

you don't check if paintRect can be empty (width or height is zero).

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:145
> +IntSize SVGImageBufferTools::clampedAbsoluteSize(const IntSize& absoluteSize)
> +{
> +    IntSize clampedAbsoluteSize = absoluteSize;
> +
> +    if (clampedAbsoluteSize.width() > kMaxImageBufferSize)
> +        clampedAbsoluteSize.setWidth(kMaxImageBufferSize);
> +
> +    if (clampedAbsoluteSize.height() > kMaxImageBufferSize)
> +        clampedAbsoluteSize.setHeight(kMaxImageBufferSize);
> +
> +    return clampedAbsoluteSize;
> +}

I'm sure that we have this code somewhere else. Did you copy this? We shouldn't have duplicated code.

> Source/WebCore/rendering/svg/SVGImageBufferTools.h:48
> +    static IntRect calcImageBufferRect(const FloatRect& targetRect, const AffineTransform& absoluteTransform)
> +    { return enclosingIntRect(absoluteTransform.mapRect(targetRect)); };

This is the wrong style, please use multiple lines for a function, even on headers.

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