[webkit-reviews] review denied: [Bug 52984] SVG animation of Paths with segments of different coordinate modes on begin and end : [Attachment 79944] 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 52984: SVG animation of Paths with segments of different coordinate modes
on begin and end
https://bugs.webkit.org/show_bug.cgi?id=52984

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

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

> Source/WebCore/svg/SVGPathBlender.cpp:62
> +float SVGPathBlender::blendAnimatedHorizontalFloat(float fromX, float toX)
>  {
> -    return FloatPoint((to.x() - from.x()) * m_progress + from.x(), (to.y() -
from.y()) * m_progress + from.y());
> +    if (m_fromMode == m_toMode)
> +	   return blendAnimatedFloat(fromX, toX);
> +
> +    // Transform toY to the coordinate mode of fromY
> +    float animX = blendAnimatedFloat(fromX, m_fromMode ==
AbsoluteCoordinates ? m_toCurrentPoint.x() + toX : toX - m_toCurrentPoint.x());

> +    
> +    if (m_progress < 0.5)
> +	   return animX;
> +    
> +    // Transform the animated point to the coordinate mode, needed for the
current progress.
> +    float currentX = blendAnimatedFloat(m_fromCurrentPoint.x(),
m_toCurrentPoint.x());
> +    return m_toMode == AbsoluteCoordinates ? animX + currentX : animX -
currentX;
> +}

Ouch, the whole code is duplicated again for blendanimatedVerticalFloat
at least use sth like
enum FloatBlendMode { BlendHorizontal, BlendVertical };
static inline float blendAnimatedFloat(float from, float to, FloatBlendMode
mode) { ... }
and call it from blendAnimatedVerticalFloat / blendAnimatedHorizontalFloat to
share code.

> Source/WebCore/svg/SVGPathBlender.cpp:257
> +			 m_progress < 0.5 ? m_fromMode : m_toMode);

Could the question m_progress < 0.5 be stored in a local variable?

> Source/WebCore/svg/SVGPathBlender.cpp:283
> +	   SVGPathSegment fromSegment;
> +	   SVGPathSegment toSegment;
> +	   if (!encodeCommand(fromCommand, fromSegment, m_fromMode)
> +	       || !encodeCommand(toCommand, toSegment, m_toMode)
> +	       || fromSegment != toSegment)

Ok i see the idea behind SVGPathSegment, but I think it could be done more
easily:

enum SVGPathSegType {
    PathSegUnknown = 0,
    PathSegClosePath = 1,
    PathSegMoveToAbs = 2,
    PathSegMoveToRel = 3,
    PathSegLineToAbs = 4,
    PathSegLineToRel = 5,
    PathSegCurveToCubicAbs = 6,
    PathSegCurveToCubicRel = 7,
    PathSegCurveToQuadraticAbs = 8,
    PathSegCurveToQuadraticRel = 9,
    PathSegArcAbs = 10,
    PathSegArcRel = 11,
    PathSegLineToHorizontalAbs = 12,
    PathSegLineToHorizontalRel = 13,
    PathSegLineToVerticalAbs = 14,
    PathSegLineToVerticalRel = 15,
    PathSegCurveToCubicSmoothAbs = 16,
    PathSegCurveToCubicSmoothRel = 17,
    PathSegCurveToQuadraticSmoothAbs = 18,
    PathSegCurveToQuadraticSmoothRel = 19
};

given the nature of this SVGPathSegType enum.

static inline bool isAbsoluteCommand(const SVGPathSegType& type)
{
    if (type < PathSegMoveToAbs)
	return true;
    // Even number = absolute command
    if (!(type % 2))
	return true;
    return false;
}

Even number == absolute coordinate, odd number == relative. And by definition
abs=true for PathSegUnknown and ClosePath.

static inline bool isSegmentEqual(const SVGPathSegType& fromType, const
SVGPathSegType& toType)
{
    if (fromType == toType && (fromType == PathSegUnknown || fromType ==
PathSegClosePath))
	return true;
    int from = fromType;
    int to = toType;
    if (isAbsoluteCommand(from))
	return from == to - !isAbsoluteCommand(to);
     return from - isAbsoluteCommand(to) == to;
}

This helper function would avoid the need for a SVGPathSegType -> (mode |
segment) decomposition. Insstead you can figure out the same from the
SVGPathSegType enum, potenttially faster, but maybe more complex.
What do you think?

> Source/WebCore/svg/SVGPathBlender.cpp:337
> +bool SVGPathBlender::encodeCommand(const SVGPathSegType& segType,
SVGPathSegment& segment, PathCoordinateMode& mode)

encodeCommand? At least sth. like decomposePathSegType(), or even more explicit
naming. (If we need this at all)

> LayoutTests/svg/animations/resources/SVGAnimationTestCase.js:10
> +function shouldBeCloseEnough(_a, _b, tolerance)

Why this copy? Also has wrong format. (8 space indentation?)