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

[bugzilla-daemon at webkit.org]

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





--- Comment #19 from Nikolas Zimmermann <zimmermann at kde.org>  2011-12-20 02:39:12 PST ---
(From update of attachment 119995)
View in context: https://bugs.webkit.org/attachment.cgi?id=119995&action=review

Some comments regarding the newest patch:

> LayoutTests/svg/clip-path/clipper-placement-issue.svg:21
> +    <!-- clipPath doesn't like use, so we copy the paths here. -->

Hm, this should work just fine, if not you've found a bug. Can you elaborate on this?

> LayoutTests/svg/filters/filter-placement-issue.svg:6
> +change with zooming. -->

change with zooming? You're not testing zooming here?

> LayoutTests/svg/filters/filter-placement-issue.svg:58
> +  <rect x="135.9" y="55.9" width="8" height="30" />
> +  <rect x="151" y="56" width="8" height="30" />

... these all have the same 8x30 size, I guess you meant "changes with the position of the target element"?

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:190
> +        if (resources && (clipper = resources->clipper())) {

This looks awkward.
I'd propose:
if (RenderSVGResourceClipper* clipper = resources ? resources->clipper() : 0) {
    ...
}

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:233
> +    AffineTransform effectiveTransform;
> +    effectiveTransform.scale(scale.width(), scale.height());
> +    effectiveTransform.multiply(filterData->shearFreeAbsoluteTransform);

A comment doesn't hurt here :-)

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:63
> +

This newline can go away.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:69
> +

Ditto.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:74
> +

Ditto.

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

Don't use c-style casts, use an explicit static_cast<float>(..) if you need it.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:130
> +    FloatRect absoluteTargetRect = calcImageBufferRect(targetRect, absoluteTransform);

Please don't use abbreviations, "calculateImageBufferRect". (Note that 'Rect' is okay, you don't need to expand it to 'Rectangle', after all FloatRect isn't named FloatRectangle...) It's inconsistent, but we have to live with it.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:146
> +    const FloatSize maxImageBufferSize(kMaxImageBufferSize, kMaxImageBufferSize);

I don't think adding const gives you a benefit here, I'd rather make it static.
DEFINE_STATIC_LOCAL(FloatSize, maxImageBufferSize, (kMaxImageBuferSize, ..));

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:147
> +    return FloatRect(absoluteTargetRect.location(), absoluteTargetRect.size().shrunkTo(maxImageBufferSize));

hm, can't you reuse clampedAbsoluteSize?
IMHO clampedAbsoluteTargetRect should use clampedAbsoluteSize with FloatSizes, there could be another variant of clampedAbsoluteSize taking IntSizes, but using the FloatSize code-path, with a "enclosingIntSize".

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:152
> +    const IntSize maxImageBufferSize(kMaxImageBufferSize, kMaxImageBufferSize);

Ditto.

> Source/WebCore/rendering/svg/SVGImageBufferTools.h:47
> +     static IntSize clampedAbsoluteSize(const IntSize& absoluteSize);

One leading space too much, and you don't need the "absoluteSize" parameter name here.

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