[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