[webkit-reviews] review denied: [Bug 55361] SVG 1.1: ineffectual transform attribute for ClipPath : [Attachment 109638] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 55361: SVG 1.1: ineffectual transform attribute for ClipPath
https://bugs.webkit.org/show_bug.cgi?id=55361

Attachment 109638: Patch
https://bugs.webkit.org/attachment.cgi?id=109638&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109638&action=review


In general this looks good, I just think querying animatedLocalTransform()
itself is unnecessary.

> Source/WebCore/ChangeLog:8
> +	   Apply transform to clip path. If a image mask is used the mask
context gets transformed.

"Respect 'transform' attribute/property for <clip-path>" sounds cleaner
"If the masking code path is used the mask context gets transformed, otherwise
the path itself."

> Source/WebCore/ChangeLog:18
> +	  
(WebCore::RenderSVGResourceClipper::calculateClipContentRepaintRect): Repaint
rect must get concatenated with current animation transform.

with the current animated transformation.

> Source/WebCore/ChangeLog:19
> +	   (WebCore::RenderSVGResourceClipper::hitTestClipContent): Point for
hit testing must be transformed to current animation transform.

transformed by the current animated transformation.

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:170
> +    AffineTransform animatedLocalTransform =
static_cast<SVGClipPathElement*>(node())->animatedLocalTransform();

This info should be present in the renderer() of the Clipper, see next comment
below.

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:301
> +    m_clipBoundaries =
static_cast<SVGClipPathElement*>(node())->animatedLocalTransform().mapRect(m_cl
ipBoundaries);

That looks a bit suspicious.
The clipBoundaries are computed by traversing through node()->firstChild().
Though the localToParentTransform() of the (node()->) renderer() also contains
the animatedLocalTransform() there should be no need to ask SVGClipPathElement
for this information.
This would be a step backwards. can you investigate?

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:318
> +    point =
clipPathElement->animatedLocalTransform().inverse().mapPoint(point);

Same comment as above.

> LayoutTests/ChangeLog:8
> +	   Test that the clip path gets concatenated with current animation
transform and

...with the current animated transformation...