[Webkit-unassigned] [Bug 41410] SVG CleanUp of SVGPathData parsing
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Jul 17 00:27:45 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=41410
Nikolas Zimmermann <zimmermann at kde.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #61875|review? |review-
Flag| |
--- Comment #12 from Nikolas Zimmermann <zimmermann at kde.org> 2010-07-17 00:27:45 PST ---
(From update of attachment 61875)
WebCore/svg/SVGPathBuilder.h:40
+ private:
Wonderful grouping between the Unaltered / Normalized interface, can you add comments here:
WebCore/svg/SVGPathBuilder.h:41
+ virtual void moveTo(const FloatPoint&, bool closed, PathCoordinateMode);
// Used in UnalteredParisng/NormalizedParsing modes
virtual void moveTo(...);
WebCore/svg/SVGPathBuilder.h:47
+ virtual void lineToHorizontal(float, PathCoordinateMode) { ASSERT_NOT_REACHED(); }
// Only used in UnalteredParsing mode
virtual void lineToHorizontal(...)
WebCore/svg/SVGPathBuilder.h:52
+ virtual void arcTo(const FloatPoint&, float, float, float, bool , bool , PathCoordinateMode) { ASSERT_NOT_REACHED(); }
trailing space after bool "bool , bool, ".
WebCore/svg/SVGPathConsumer.h:44
+ public:
Can you group these as well in two public sections, with the same comments as mentioned above.
public:
// Used in UnalteredParisng/NormalizedParsing modes
virtual void moveTo(...);
....
public:
// Used in UnalteredParisng/NormalizedParsing modes
virtua void lineToHorizontal(...);
WebCore/svg/SVGPathParser.cpp:49
+ // reset m_curX, m_curY for next path
Make this a whole sentence: // Reset m_curX, m_curY for the next path.
WebCore/svg/SVGPathParser.cpp:148
+ FloatPoint point3(toX, toY);
Can you move these three FloatPoints out of the if (m_normalized) branch? That would save the need to recreate them in the not-normalized case.
m_consumer->curveToCubic(point1, point2, point3, m_mode);
WebCore/svg/SVGPathParser.cpp:188
+ m_controlX = m_curX;
How about storing FloatPoint m_controlPoint instead of m_controlX/m_controlY.
Same for m_curX / m_curY could be FloatPoint m_currentPoint.
Same for m_subPathX / m_subPathY -> FloatPoint m_subPathStartPoint
WebCore/svg/SVGPathParser.cpp:322
+ rx = abs(rx);
s/abs/fabs/!
WebCore/svg/SVGPathParser.cpp:439
+ && (command != 'z' && command != 'Z')) {
Superflous braces.
WebCore/svg/SVGPathParser.cpp:464
+ FloatSize delta = point1 - point2;
Isn't delta just the midPointDistance here? (as in distance between point1 and point2).
WebCore/svg/SVGPathParser.cpp:470
+ FloatPoint dPoint1 = pointTransform.mapPoint(FloatPoint(delta.width(), delta.height()));
s/dPoint1/transformedMidPoint/ ?
WebCore/svg/SVGPathParser.cpp:518
+ int nSegs = ceilf(fabsf(thetaArc / (piFloat * 0.5f)));
Don't we have a piHalf constant in MathExtras.h? turns out we don't please addd a M_PI_2 section to it.
WebCore/svg/SVGPathParser.cpp:520
+ float tempTheta1 = theta1 + i * thetaArc / nSegs;
s/tempTheta/currentTheta/? I dislike the temp name.
WebCore/svg/SVGPathParser.cpp:525
+ float sinTheta1 = sinf(tempTheta1);
You don't really need these local variables.
WebCore/svg/SVGPathSegListBuilder.cpp:68
+ ASSERT(ec);
ASSERT(!ec) everywhere! You should test a debug build :-)
WebCore/svg/SVGPathSegListBuilder.h:40
+ private:
Can you apply the same grouping and comments as in SVGPathConsumer.h / SVGPathBuilder.h?
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list