[webkit-reviews] review denied: [Bug 61368] SVGAnimation should use direct unit animation for SVGLength : [Attachment 95264] 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 61368: SVGAnimation should use direct unit animation for SVGLength
https://bugs.webkit.org/show_bug.cgi?id=61368

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

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

It's a great start, I still have some comments though that lead to r-:

> Source/WebCore/ChangeLog:11
> +
> +	   The goal is to move the main animation logic for SVG units from
SVGAnimateElement into the corresponding SVGAnimated* files.
> +	   Starting with SVGLength on this patch. SVGAnimateElement will just
work with the generic type SVGAnimatedType. SVGAnimator
> +	   coordinates the connection from SVGAnimatedType to the SVG unit
animators like SVGAnimatedLengthAnimator in SVGAnimatedLength.h.
> +

I think you should make it much more clear, that you're going away from
any->string->any operation mode for animations, but rather directly animate
"any" w/o string roundtrips.

> Source/WebCore/svg/SVGAnimateElement.cpp:125
> +static inline SVGLengthMode lengthModeForAnimatedLengthAttribute(const
QualifiedName& attrName)

Does this method really belong here? How about moving it to SVGLength as static
function?

> Source/WebCore/svg/SVGAnimateElement.cpp:448
> +	   m_animator = SVGAnimator::create(AnimatedLength, targetElement);

This is just called, once right? We're not recreating SVGAnimator on every
animation step anymore, right?
Can we assert here, that m_animator is null? Are we properly destructing the
animator at some point?

> Source/WebCore/svg/SVGAnimateElement.cpp:449
> +	  
static_cast<SVGAnimatedLengthAnimator*>(m_animator.get())->setSVGLengthMode(len
gthModeForAnimatedLengthAttribute(attributeName()));

I'm wondering whether this logic could be folded into the SVGAnimator::create
code, by just passing the attributeName() as another constructor parameter.
If other types don't need to know the attribute name they can just ignore the
third ctor param...

> Source/WebCore/svg/SVGAnimateElement.cpp:492
> +	   m_animator = SVGAnimator::create(AnimatedLength, targetElement);
> +	  
static_cast<SVGAnimatedLengthAnimator*>(m_animator.get())->setSVGLengthMode(len
gthModeForAnimatedLengthAttribute(attributeName()));

Ditto.

> Source/WebCore/svg/SVGAnimateElement.cpp:545
> +	   m_animatedType = SVGAnimatedType::create(AnimatedLength);
> +	   m_animatedType->setLength(new
SVGLength(lengthModeForAnimatedLengthAttribute(attributeName()), baseString));

How about adding SVGAnimatedType::createLength() which takes another LengthMode
and const String& parameter, and hides this code?

> Source/WebCore/svg/SVGAnimateElement.cpp:584
> +    // TODO: Distance calculation should be done in SVGAnimatedTypeAnimator.


Can't you fix this directly, and add a calculateDistance method? Or is it too
much work, to move from from the current approach?

> Source/WebCore/svg/SVGAnimatedLength.cpp:36
> +    OwnPtr<SVGAnimatedType> type = SVGAnimatedType::create(AnimatedLength);
> +    type->setLength(new SVGLength(m_lengthMode, string));

Using SVGAnimatedType::createLength(m_lengthMode, string) seems better here.
See suggestion above.

> Source/WebCore/svg/SVGAnimatedLength.cpp:60
> +void SVGAnimatedLengthAnimator::calculateAnimatedValue(SVGSMILElement*
smilElement, float percentage, unsigned repeat,

s/repeat/repeatCount/ ?

> Source/WebCore/svg/SVGAnimatedLength.cpp:62
> +							  bool
fromPropertyInherits, bool toPropertyInherits)

s/fromPropertyInherits/inheritFromValue/ ?

> Source/WebCore/svg/SVGAnimatedLength.cpp:82
> +	   SVGLength tempFrom = SVGLength(m_lengthMode, fromLengthString);

These temporary objects aren't good, I'd say try following:
static inline SVGLength& sharedSVGLength(LengthMode mode, const String&
valueAsString)
{
    DEFINE_STATIC_LOCAL(SVGLength, sharedLength, ());
    sharedLength.setValueAsString(mode, valueAsString);
    return sharedLength;
}

Of course you'll need to add setValueAsString(LengthMode, const String&) which
does the same as the ctor SVGLength(LengthMode, const String&).
This way you can just use:
fromLength = sharedSVGLength(m_lengthMode,
fromLengthString).value(m_contextElement);
here, and avoid the cost of creating/destructing a SVGLength object.

> Source/WebCore/svg/SVGAnimatedLength.cpp:89
> +	   SVGLength tempTo = SVGLength(m_lengthMode, toLengthString);
> +	   toLength = tempTo.value(m_contextElement); 

Ditto.

> Source/WebCore/svg/SVGAnimatedLength.cpp:106
> +	   animated->length().setValue(number, m_contextElement, ec);

I'd store SVGLength& animatedLength = animated->length(); in a local variable
on top.

> Source/WebCore/svg/SVGAnimatedLength.h:52
> +    virtual void calculateAnimatedValue(SVGSMILElement*, float percentage,
unsigned repeat,
> +					   OwnPtr<SVGAnimatedType>& fromValue,
OwnPtr<SVGAnimatedType>& toValue, OwnPtr<SVGAnimatedType>& animatedValue,
> +					   bool fromPropertyInherits, bool
toPropertyInherits);

Same parameter renamings should be done here.

> Source/WebCore/svg/SVGAnimatedType.h:50
> +    void setLength(SVGLength* length)
> +    {
> +	   ASSERT(m_type == AnimatedLength);
> +	   m_data.length = length;
> +    }

If you add a createLength() function here, you can completely remove
setLength().

> Source/WebCore/svg/SVGAnimatedType.h:57
> +    

You have to manually destruct the object here, the SVGAnimatedType dtor is
missing, which invokes the correct operator delete, based on the m_type.
Currently you're leaking.

> Source/WebCore/svg/SVGAnimationElement.cpp:163
> +	   for (unsigned i = 0; i < m_values.size(); ++i)
> +	       m_values[i] = m_values[i].stripWhiteSpace();

Why is this needed?

>
LayoutTests/svg/animations/script-tests/svglength-animation-LengthModeWidth.js:
37
> +    shouldBeCloseEnough("rect.width.baseVal.value", "200", 0.01);

I'd remove the baseVal checks from all tests, only check the animVal,
otherwhise they are wrong as soon as we implement animVal support for real.