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

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

Next round of comments, sorry:

> Source/WebCore/ChangeLog:8
> +	   The goal is to move the main animation logic for each SVG unit from
SVGAnimateElement into the corresponding SVGAnimated* files.

s/SVG unit/SVG primitive datatype/ (after all 'SVGLength' is not a 'unit').

> Source/WebCore/ChangeLog:20
> +	   We don't support animations between different units right now:
from="20px" to="20%" for example. The second benefit is, that we do not
> +	   rewrite the parsing code for parsing the values in the attributes
'from', 'to' and 'values'. 

I don't understand the second benefit "that we do not rewrite the parsing code
for parsing the values" ?

> Source/WebCore/ChangeLog:21
> +	   The main benefit: Animate SVG values directly will be easier to
implement, once moving the code was finished.

s/Animate/Animated/. I think this is confusing	-- you are referring to animVal
support here, and how it's now easier to implement it as we're actually
animating SVGLengths instead of number+units, so we could use that later on as
"animVal' for the SVG DOM, right? Can you please rephrase this.

> Source/WebCore/ChangeLog:23
> +	   The current patch is starting with SVGLength. SVGAnimator
coordinates the connection from SVGAnimatedType to the SVG unit animators

The current patch converts SVGLength to the new concept. (The patch is
startin... is like "Dieser Zug endet hier" ;-)

> Source/WebCore/ChangeLog:24
> +	   The current patch is starting with SVGLength. SVGAnimator
coordinates the connection from SVGAnimatedType to the SVG unit animators
> +	   like SVGAnimatedLengthAnimator in SVGAnimatedLength.h.

SVGAnimator coordinates the connection?

> Source/WebCore/svg/SVGAnimateElement.cpp:-74
> -    else if (parse.endsWith("px") || parse.endsWith("pt") ||
parse.endsWith("em"))
> -	   unitLength = 2;

What is this about??

> Source/WebCore/svg/SVGAnimateElement.cpp:458
> +	   if (!m_animator)
> +	       m_animator = SVGAnimator::create(targetElement,
m_animatedAttributeType, attributeName());
> +	   m_animator->calculateFromAndByValues(m_fromType, m_toType,
fromString, byString);

Can't you move these in an ensureAnimator() method?
so you could use
ensureAnimator()->calculateFromAndByValues(...)

> Source/WebCore/svg/SVGAnimateElement.cpp:509
> +	   m_animatedType = SVGAnimatedType::createLength(new
SVGLength(SVGLength::lengthModeForAnimatedLengthAttribute(attributeName()),
baseString));

This detail should be hidden, the SVGLength construction, it should be
encapsulated.
Suggestion:
m_animatedType =
SVGAnimatedLength::createLength(SVGLength::lengthModeForAnimatedLengthAttribute
(attributeName()), baseString).

Then SVGAnimatedLength::createLength does what you do now. That just hides the
"new" and "delete". We want to avoid to expose that we're initentionally _Not_
using smart pointers for this SVGLength object, as it lives within an union.

EEEK I've just seen you have "PassOwnPtr<SVGAnimatedType>
SVGAnimatedLengthAnimator::constructFromString(const String& string)".
Why don't you just use that?

m_animatedType = m_animator->constructFromString(baseString)?

> Source/WebCore/svg/SVGAnimateElement.cpp:573
> +	   if (!m_animator)
> +	       m_animator = SVGAnimator::create(targetElement,
m_animatedAttributeType, attributeName());

return ensureAnimator()->calculateDistance(this, fromString, toString);

> Source/WebCore/svg/SVGAnimatedType.h:36
> +    static PassOwnPtr<SVGAnimatedType> createLength(SVGLength* length)
> +    {
> +	   return adoptPtr(new SVGAnimatedType(AnimatedLength, length));
> +    }

This should be removed.

> Source/WebCore/svg/SVGAnimatedType.h:40
> +	   delete m_data.length;

Add the same assertion regarding m_type here, as longs as we don't support
other types.

> Source/WebCore/svg/SVGAnimatedType.h:66
> +	   // FIXME: More SVGUnits need to be added step by step.

s/SVGUnits/SVG primitive types/

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

I'd still love to see a comment here, best with a spec link.

> Source/WebCore/svg/SVGAnimator.h:28
> +class SVGAnimator {

I think SVGAnimator sounds way too generic how about
"SVGAnimatedTypeAnimatorFactory" or short "SVGAnimatorFactory"

> Source/WebCore/svg/SVGLength.cpp:140
> +    setValueAsString(valueAsString, ec);

ASSERT(!ec) ? what to do if an exception is raised? is that possible?

> Source/WebCore/svg/SVGLength.h:91
> +    void setValueAsString(const String&, SVGLengthMode);

Hm, for consistency - pass on the Ec?