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

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

Almost there.... but the ChangeLog is so confusing, I'd like to see a new
version.

> Source/WebCore/ChangeLog:15
> +	   This and the next patches will just fix the number + unit parsing.
We will work with the SVG units directly internaly.

typo: internally.

> Source/WebCore/ChangeLog:18
> +	   There are some benefits with the new infrastructure. At first, the
current parsing in the animation code just respects a few units (5 for
SVGlength).

typo: SVGLength.

> Source/WebCore/ChangeLog:22
> +	   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', see parseNumberValueAndUnit in SVGAnimateElement. 
> +	   The main benefit: Because we use SVG primitive datatype for
animations, it will be easier to support animVal and therefore can avoid
converting values
> +	   to string.

I still understand the ChangeLog, also it's not using proper english. "rewrite
the parsing code for parsing the values"??
I also don't know whether we're supporting anims between different units now
with your patch or not.
Please post a new ChangeLog at least...

> Source/WebCore/svg/SVGAnimateElement.cpp:453
> +	  
ensureAnimator(m_animatedAttributeType)->calculateFromAndByValues(m_fromType,
m_toType, fromString, byString);

Why does everyone have to pass m_animatedAttirbuteType?? Just let
ensureAnimator read m_animatedAttributeType directly.

> Source/WebCore/svg/SVGAnimatorFactory.h:29
> +    WTF_MAKE_FAST_ALLOCATED;

This class is never allocated. I'd remove this and rather provide an
unimplemented private constructor, so no-one can ever create an object of type
SVGAnimatorFactory.