[webkit-reviews] review denied: [Bug 21586] SVG CSS properties don't work with CSS transitions : [Attachment 104486] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Dean Jackson <dino at apple.com>'s
request for review:
Bug 21586: SVG CSS properties don't work with CSS transitions
https://bugs.webkit.org/show_bug.cgi?id=21586

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=104486&action=review


Some snippets and comments.

> LayoutTests/transitions/svg-transitions.html:35
> +    #rect2 {
> +	 fill: rgb(0, 0, 255);
> +	 stroke: rgb(255, 0, 0);
> +    }
> +    .animating #rect2 {
> +	 fill: rgb(0, 255, 0);
> +	 stroke: rgb(0, 0, 0);
> +    }

Can you add a test with fill: url(#invalid) green;  to fill: url(#invalid)
blue; please? Even if does not pass at the beginning, or at least not animate.

> Source/WebCore/page/animation/AnimationBase.cpp:221
> +static inline SVGLength blendFunc(const AnimationBase*, const SVGLength&
from, const SVGLength& to, double progress)

Can you call it blendSVGLength?

> Source/WebCore/svg/SVGLength.h:125
> +	   if (!from.isZero() && !isZero() && from.unitType() != unitType())
> +	       return *this;

Why don't you allow animation between different unit types? We are doing it
with SVGAnimatedLengthAnimator as well. You may need a new private value()
function.

> Source/WebCore/svg/SVGLength.h:128
> +	   if (from.isZero() && isZero())
> +	       return *this;

You need an early return for LengthTypeUnknown as well, no?

> Source/WebCore/svg/SVGLength.h:140
> +	   if (resultType == LengthTypePercentage) {
> +	       float fromPercent = from.valueAsPercentage();
> +	       float toPercent = valueAsPercentage();
> +	       length.newValueSpecifiedUnits(LengthTypePercentage, fromPercent
+ (toPercent - fromPercent) * progress, ec);

Ah ok, animation between different unit types is difficult if one of the values
is percentage since you need the viewport. You can still do it with animation
between other unit types. But for percentage animation we need access to the
SVGElement. I don't know another way to get the information about the viewPort
:/