[webkit-reviews] review denied: [Bug 11929] Gradient SVG animation demonstrates tearing at animation extremes : [Attachment 44155] SVG fix for broken radial gradient

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 11929: Gradient SVG animation demonstrates tearing at animation extremes
https://bugs.webkit.org/show_bug.cgi?id=11929

Attachment 44155: SVG fix for broken radial gradient
https://bugs.webkit.org/attachment.cgi?id=44155&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Good morning Dirk,

I see some issues with the patch:
- ChangeLogs are not using the correct style: bug url should sit above
description, without surrounding brackets (best idea is to use
"prepare-ChangeLog --bug=<bug-id>", this takes care of using the right style
for you)
- The patch can't go in without updated test results (I can help with that, if
you don't have a mac around)

> -    float fdx = focalPoint.x() - centerPoint.x();
> -    float fdy = focalPoint.y() - centerPoint.y();
> +    FloatPoint adjustedFocalPoint = focalPoint;
> +    adjustedFocalPoint.move(-centerPoint.x(), -centerPoint.y());
I agree with the first change, using 'FloatPoint adjustedFocalPoint' instead of
the two adjustedFocusX/adjustedFocusY variables, though the "fdx" ones are
useful, as they save function call overhead in the sqrt() below, just the name
is bad. (Re-)Introducing helper variables, also saves you from having to mve
back the adjustFocalPoint by the centerPoint below the first if() statement.


> +	   // The maximum deviation of 0.2% is needed for Cairo, since Cairo
> +	   // is working with fixed point numbers.
> +	   if (focalPoint.x() < centerPoint.x())
> +	       adjustedFocalPoint.setX(cosf(angle) * radius + 0.002f);
> +	   else
> +	       adjustedFocalPoint.setX(cosf(angle) * radius - 0.002f);
> +	   if (focalPoint.y() < centerPoint.y())
> +	       adjustedFocalPoint.setY(sinf(angle) * radius + 0.002f);
> +	   else
> +	       adjustedFocalPoint.setY(sinf(angle) * radius - 0.002f);

This really worries me. If you really need a Cairo specific workaround, it
can't go directly into SVGRadialGradientElement. Not sure about the root of
your problem, and how to fix it in another way, offhand. Please elaborate on
that.

Have a nice day,
Niko

>      }
>  
> +    adjustedFocalPoint.move(centerPoint.x(), centerPoint.y());
> +
>      RefPtr<Gradient> gradient = Gradient::create(
> -	   FloatPoint(adjustedFocusX, adjustedFocusY),
> +	   adjustedFocalPoint,
>	   0.f, // SVG does not support a "focus radius"
>	   centerPoint,
>	   radius);
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 51600)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,18 @@
> +2009-12-02  Dirk Schulze  <krit at webkit.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   In SVG a focalPoint must be inside the radius of a radial gradient.
> +	   It this isn't the case, we have to move the focalPoint into the
radius.
> +	   This checks the correct behavior of WebKit on false values for fx,
fy.	 
> +
> +	   * platform/mac/svg/W3C-SVG-1.1/pservers-grad-13-b-expected.checksum:

> +	   * platform/mac/svg/W3C-SVG-1.1/pservers-grad-13-b-expected.png:
> +	   *
platform/mac/svg/custom/radial-gradient-with-outstanding-focalPoint-expected.ch
ecksum: Added.
> +	   *
platform/mac/svg/custom/radial-gradient-with-outstanding-focalPoint-expected.pn
g: Added.

> +	   *
platform/mac/svg/custom/radial-gradient-with-outstanding-focalPoint-expected.tx
t: Added.
> +	   * svg/custom/radial-gradient-with-outstanding-focalPoint.svg: Added.

> +
>  2009-12-02  Csaba Osztrogonác  <ossy at webkit.org>G