[webkit-reviews] review denied: [Bug 73643] SVG filters incorrectly move elements : [Attachment 118088] Proposed fix

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Branimir Lambov
<blambov at google.com>'s request for review:
Bug 73643: SVG filters incorrectly move elements
https://bugs.webkit.org/show_bug.cgi?id=73643

Attachment 118088: Proposed fix
https://bugs.webkit.org/attachment.cgi?id=118088&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
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.