Bug 52984

Created attachment 79944 [details] Patch

Comment on attachment 79944 [details] Patch 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?)

(In reply to comment #2) > (From update of attachment 79944 [details]) > 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. blendAnimatedFloat() is also used for normal blendings like raduis that are independent of the current coordinate as well. So either we need to define another function beside blendAnimatedFloat() that is doing your suggestion, or we make a common function for blendAnimatedVerticalFloat / blendAnimatedHorizontalFloat named blendAnimatedDirectionFloat(), this would take your enum and we either take m_fromCurrentPoint.x(), m_toCurrentPoint.x() or m_fromCurrentPoint.y(), m_toCurrentPoint.y(). Another problem with your static inline would be the access to the member variables of the class. > > > 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? We could also use a member variable called isInFirstHalfOfAnimation and set it just once if you want. > > > 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; > } Should work, yes . > > 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? Yes, we could make a switch and check the values one by another. If complex means a lot of code lines, yes. Not sure what is better here. On the other hand we want it as fast as possible. What's your opinion? :-P > > > 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) Not needed anymore, yes. > > > LayoutTests/svg/animations/resources/SVGAnimationTestCase.js:10 > > +function shouldBeCloseEnough(_a, _b, tolerance) > > Why this copy? Also has wrong format. (8 space indentation?) Why is it a copy? It is just similar to shouldBe, but with a third parameter: the tolerance level. But with this function we can save a lot of unnecessary code in the tests, just take a look at the other animation tests. And I couldn't find a function with similar abilities in js-pre... The indentations are tabs that need to be replaced by spaces.

(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 79944 [details] [details]) > > 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. > blendAnimatedFloat() is also used for normal blendings like raduis that are independent of the current coordinate as well. So either we need to define another function beside blendAnimatedFloat() that is doing your suggestion, or we make a common function for blendAnimatedVerticalFloat / blendAnimatedHorizontalFloat named blendAnimatedDirectionFloat(), this would take your enum and we either take m_fromCurrentPoint.x(), m_toCurrentPoint.x() or m_fromCurrentPoint.y(), m_toCurrentPoint.y(). Another problem with your static inline would be the access to the member variables of the class. Well you can pass a FloatPoint& reference to the function. > > > 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? > We could also use a member variable called isInFirstHalfOfAnimation and set it just once if you want. > Okay. > > > > > 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; > > } > Should work, yes > . > > > > 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? > Yes, we could make a switch and check the values one by another. If complex means a lot of code lines, yes. Not sure what is better here. On the other hand we want it as fast as possible. What's your opinion? :-P Well try with my suggestion, and see what you like more. > > > > > > 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) > Not needed anymore, yes. > > > > > > LayoutTests/svg/animations/resources/SVGAnimationTestCase.js:10 > > > +function shouldBeCloseEnough(_a, _b, tolerance) > > > > Why this copy? Also has wrong format. (8 space indentation?) > > Why is it a copy? It is just similar to shouldBe, but with a third parameter: the tolerance level. But with this function we can save a lot of unnecessary code in the tests, just take a look at the other animation tests. And I couldn't find a function with similar abilities in js-pre... Ok, I think we have a "isCloseEnough" method somewhere.. can't find atm. > > The indentations are tabs that need to be replaced by spaces. Ok.

(In reply to comment #4) > > blendAnimatedFloat() is also used for normal blendings like raduis that are independent of the current coordinate as well. So either we need to define another function beside blendAnimatedFloat() that is doing your suggestion, or we make a common function for blendAnimatedVerticalFloat / blendAnimatedHorizontalFloat named blendAnimatedDirectionFloat(), this would take your enum and we either take m_fromCurrentPoint.x(), m_toCurrentPoint.x() or m_fromCurrentPoint.y(), m_toCurrentPoint.y(). Another problem with your static inline would be the access to the member variables of the class. > Well you can pass a FloatPoint& reference to the function. Sure. I can do it. But it must not make the code more readable ;-)

Created attachment 80113 [details] Patch

Comment on attachment 80113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80113&action=review Almost r+, I'll have to recheck the "from == to - 1" stuff, leaving r? until I rechecked it, mom. > Source/WebCore/svg/SVGPathBlender.cpp:64 > + float fromDimensionalValue = blendMode == BlendHorizontal ? m_fromCurrentPoint.x() : m_fromCurrentPoint.y(); > + float toDimensionalValue = blendMode == BlendHorizontal ? m_toCurrentPoint.x() : m_toCurrentPoint.y(); > + > + // Transform toY to the coordinate mode of fromY > + float animValue = blendAnimatedFloat(from, m_fromMode == AbsoluteCoordinates ? to + toDimensionalValue : to - toDimensionalValue, m_progress); > + > + if (m_isInFirstHalfOfAnimation) > + return animValue; > + > + // Transform the animated point to the coordinate mode, needed for the current progress. > + float currentValue = blendAnimatedFloat(fromDimensionalValue, toDimensionalValue, m_progress); > + return m_toMode == AbsoluteCoordinates ? animValue + currentValue : animValue - currentValue; s/DimensionalValue/Value/, I don't think the 'dimensional' prefix is of any benefit. > Source/WebCore/svg/SVGPathBlender.cpp:259 > + if (type < PathSegMoveToAbs) > + return AbsoluteCoordinates; > + > + // Odd number = relative command > + if (type % 2) > + return RelativeCoordinates; > + > + return AbsoluteCoordinates; I'd rewrite this as: if (type >= PathSegMoveToAbs && type % 2) return RelativeCoordinates; return AbsoluteCoordinates. > Source/WebCore/svg/SVGPathBlender.cpp:273 > + if (fromMode == toMode) > + return from == to; > + if (fromMode == AbsoluteCoordinates) > + return from == to - 1; > + return to == from - 1; I don't get this part, I need to recheck.

Comment on attachment 80113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80113&action=review r=me. >> Source/WebCore/svg/SVGPathBlender.cpp:273 >> + return to == from - 1; > > I don't get this part, I need to recheck. I orignally proposed: if (isAbsoluteCommand(from)) return from == to - !isAbsoluteCommand(to); return from - isAbsoluteCommand(to) == to; if from is absolute, then from == to (if to is absolute) OR from == to - 1 (if to is relative). If from is relative, then from == to (if to is relative) OR from -1 == to (if to is absolute). I see that your if (fromMode == toMode) covers the case when boths modes are either abs or relative, that is correct. iAh, just realized the code is identical :-) good work.

Committed r76830: <http://trac.webkit.org/changeset/76830>

http://trac.webkit.org/changeset/76830 might have broken GTK Linux 32-bit Release The following tests are not passing: svg/animations/animate-text-nested-transforms.html

(In reply to comment #10) > http://trac.webkit.org/changeset/76830 might have broken GTK Linux 32-bit Release > The following tests are not passing: > svg/animations/animate-text-nested-transforms.html Landed rebaseline in http://trac.webkit.org/changeset/76846

Committed r76846: <http://trac.webkit.org/changeset/76846>

http://trac.webkit.org/changeset/76846 might have broken Leopard Intel Release (Tests)