[Webkit-unassigned] [Bug 11929] Gradient SVG animation demonstrates tearing at animation extremes

[bugzilla-daemon at webkit.org]

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


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #44155|review?                     |review-
               Flag|                            |




--- Comment #11 from Nikolas Zimmermann <zimmermann at kde.org>  2009-12-02 18:37:50 PST ---
(From update of attachment 44155)
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.checksum: Added.
> +        * platform/mac/svg/custom/radial-gradient-with-outstanding-focalPoint-expected.png: Added.

> +        * platform/mac/svg/custom/radial-gradient-with-outstanding-focalPoint-expected.txt: Added.
> +        * svg/custom/radial-gradient-with-outstanding-focalPoint.svg: Added.
> +
>  2009-12-02  Csaba Osztrogonác  <ossy at webkit.org>G

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