[webkit-reviews] review denied: [Bug 41410] SVG CleanUp of SVGPathData parsing : [Attachment 61875] 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 41410: SVG CleanUp of SVGPathData parsing
https://bugs.webkit.org/show_bug.cgi?id=41410

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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?