SVGAnimatedType should support SVGPreserveAspectRatio animation

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

SVGAnimatedType should support SVGPreserveAspectRatio animation.

[Dirk Schulze]

Created attachment 98730 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 98730 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98730&action=review

Looks great, please fix up some thing and make sure it actually builds. If you prefer, you can also upload a new one.

> Source/WebCore/svg/SVGAnimatedPreserveAspectRatio.cpp:57
> +    // Not specified what to do on 'by'-animations with SVGPreserveAspectRatio. Fallback to 'to'-animation right now. 
> +    from = constructFromString(fromString);
> +    to = constructFromString(byString);

I used ASSERT_NOT_REACHED here for in SVGAnimatedPathAnimator, and made sure AnimatedPath is not added to the switch() which calls calcFromAndByValues.
Isn't that better than to fail silently?

> Source/WebCore/svg/SVGAnimatedPreserveAspectRatio.cpp:77
> +    // No paced animations for SVGPreserveAspectRatio.

Add the other FIXME?

> Source/WebCore/svg/SVGAnimatedType.h:77
> +    // Used for parsing a String to a SVGPreserveAspectRatio object.
> +    void setPreserveAspectRatioBaseValue(const SVGPreserveAspectRatio&);

Hm, you are not using this method anywhere?

> Source/WebCore/svg/SVGAnimatedType.h:-84
> -        // FIXME: More SVG primitive types need to be added step by step.

Are we done? bool* and ushort* is still missing, no?

[Dirk Schulze]

(In reply to comment #2)
> (From update of attachment 98730 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98730&action=review
> 
> Looks great, please fix up some thing and make sure it actually builds. If you prefer, you can also upload a new one.
> 
> > Source/WebCore/svg/SVGAnimatedPreserveAspectRatio.cpp:57
> > +    // Not specified what to do on 'by'-animations with SVGPreserveAspectRatio. Fallback to 'to'-animation right now. 
> > +    from = constructFromString(fromString);
> > +    to = constructFromString(byString);
> 
> I used ASSERT_NOT_REACHED here for in SVGAnimatedPathAnimator, and made sure AnimatedPath is not added to the switch() which calls calcFromAndByValues.
> Isn't that better than to fail silently?
You did not make sure that you never reached 'by'-animation for path. And of course it's not enough to just add an ASSERT_NOT_REACHED. Path is a special case and I gave r+ to your patch, because calculateFromAndByValues() is never called from SVGAnimationElement. This is not the case for PreserveAspectRatio. We even don't know that we animate PreserveAspectRatio in SVGAnimationElement. Opera is also using 'to' animations in that case.

> 
> > Source/WebCore/svg/SVGAnimatedPreserveAspectRatio.cpp:77
> > +    // No paced animations for SVGPreserveAspectRatio.
> 
> Add the other FIXME?
How do you want to interpolate these 'string' values? You can't. Thats why it is not an FIXME. Same for String, Enum, Boolean.

And to both calls. I don't think we should prevent the calls, this would need special logic in SVGAnimateElement, and I'd like to remove all switch and if conditions in the context of AnimatedType. This makes it easier to concentrate the basic logic in SVGAnimationElement for all kind of animations later.

> 
> > Source/WebCore/svg/SVGAnimatedType.h:77
> > +    // Used for parsing a String to a SVGPreserveAspectRatio object.
> > +    void setPreserveAspectRatioBaseValue(const SVGPreserveAspectRatio&);
> 
> Hm, you are not using this method anywhere?
I am. See SVGPreserveAspectRatio::parsePreserveAspectRatio(), it takes a consumer as argument and calls setPreserveAspectRatioBaseValue on the consumer. I take the SVGAnimatedType as consumer.

> 
> > Source/WebCore/svg/SVGAnimatedType.h:-84
> > -        // FIXME: More SVG primitive types need to be added step by step.
> 
> Are we done? bool* and ushort* is still missing, no?

Well, I can re-add this line, but at this point we can remove it IMHO. 'Done' is relative, we might want to add a TransformationList and I am not sure what we need for AnimateMotion. The main types how ever are converted already. But you are right, I plan to add enum and bool support as well.

[Nikolas Zimmermann]

(In reply to comment #3)
> > 
> > I used ASSERT_NOT_REACHED here for in SVGAnimatedPathAnimator, and made sure AnimatedPath is not added to the switch() which calls calcFromAndByValues.
> > Isn't that better than to fail silently?
> You did not make sure that you never reached 'by'-animation for path. And of course it's not enough to just add an ASSERT_NOT_REACHED. Path is a special case and I gave r+ to your patch, because calculateFromAndByValues() is never called from SVGAnimationElement.
This is exactly what I meant with "and made sure AnimatedPath is not added to the switch() which calls calcFromAndByValues."

> This is not the case for PreserveAspectRatio.
As I said, when you want to add assert_not_reached for by anims, then you have to make sure you're not calling calcFromAndByValues from SVAnimationElement.

>We even don't know that we animate PreserveAspectRatio in SVGAnimationElement. Opera is also using 'to' animations in that case.
Well, now that makes sense, for compatibility. Of course a by animation in the context of aspect ratio is useless, so this behaviour might make sense to some degree.


> 
> > 
> > > Source/WebCore/svg/SVGAnimatedPreserveAspectRatio.cpp:77
> > > +    // No paced animations for SVGPreserveAspectRatio.
> > 
> > Add the other FIXME?
> How do you want to interpolate these 'string' values? You can't. Thats why it is not an FIXME. Same for String, Enum, Boolean.
I'm sorry, you're totally right. (I had viewBox in mind, not pAR)
> 
> And to both calls. I don't think we should prevent the calls, this would need special logic in SVGAnimateElement, and I'd like to remove all switch and if conditions in the context of AnimatedType. This makes it easier to concentrate the basic logic in SVGAnimationElement for all kind of animations later.
That's a good argument as well, keep it as is.

> 
> > 
> > > Source/WebCore/svg/SVGAnimatedType.h:77
> > > +    // Used for parsing a String to a SVGPreserveAspectRatio object.
> > > +    void setPreserveAspectRatioBaseValue(const SVGPreserveAspectRatio&);
> > 
> > Hm, you are not using this method anywhere?
> I am. See SVGPreserveAspectRatio::parsePreserveAspectRatio(), it takes a consumer as argument and calls setPreserveAspectRatioBaseValue on the consumer. I take the SVGAnimatedType as consumer.
Oh wow, that's hidden API ;-)
 
> Well, I can re-add this line, but at this point we can remove it IMHO. 'Done' is relative, we might want to add a TransformationList and I am not sure what we need for AnimateMotion. The main types how ever are converted already. But you are right, I plan to add enum and bool support as well.
Okay great.

[Dirk Schulze]

Committed r89833: <http://trac.webkit.org/changeset/89833>