[webkit-reviews] review denied: [Bug 41410] SVG CleanUp of SVGPathData parsing : [Attachment 61339] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jul 13 01:23:56 PDT 2010 

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 61339: Patch
https://bugs.webkit.org/attachment.cgi?id=61339&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
WebCore/Android.mk:854
 +	svg/SVGPathBuilder.cpp \
Tabs (or wrong indention).WebCore/WebCore.vcproj/WebCore.vcproj:13059
 +  
Why these newlines?

WebCore/WebCore.vcproj/WebCore.vcproj:18079
 +  
Ditto.

WebCore/WebCore.vcproj/WebCore.vcproj:22514
 +  
Ditto.

WebCore/WebCore.vcproj/WebCore.vcproj:26490
 +  
Ditto.

WebCore/WebCore.vcproj/WebCore.vcproj:38933
 +  
Ditto.

WebCore/WebCore.vcproj/WebCore.vcproj:40490
 +  
Ditto.

WebCore/WebCore.vcproj/WebCore.vcproj:50955
 +  
Ditto.

WebCore/platform/graphics/transforms/AffineTransform.cpp:304
 +  FloatPoint AffineTransform::mapArcPoint(const FloatPoint& point) const
I don't think this method makes much sense here, only the SVG arc code should
ever need it. It's also confusing, as the logic might seem wrong on first
sight. Please move to SVG specific code.

WebCore/svg/SVGAnimateElement.cpp:193
 +	    SVGPathSegListBuilder builder;
SVGPathSegListBuilder should take a SVGPathSegList* argument. Not build(). Just
like you have done for SVGPathBuilder with Path&.

WebCore/svg/SVGAnimateMotionElement.cpp:92
 +	    builder.build(attr->value());
Add a FIXME that the return value shouldn't be ignored.

WebCore/svg/SVGGlyphElement.cpp:106
 +	builder.build(value);
Add a FIXME that the return value shouldn't be ignored.

WebCore/svg/SVGPathBuilder.cpp:61
 +	    m_path.addBezierCurveTo(point1 + m_current, point2 + m_current,
m_current + point);
Can you reverse the arguments to the + operator for consistency? m_current +
point1, m_current + point2;

WebCore/svg/SVGPathBuilder.cpp:65
 +	    m_path.addBezierCurveTo(point1, point2, m_current) ;
Trailing space before semicolon;

WebCore/svg/SVGPathBuilder.h:43
 +	virtual void lineToHorizontal(float, CoordinateMode) {
ASSERT_NOT_REACHED(); }
Can you group the calls that evaluate to ASSERT_NOT_REACHED, in a second
private: section, with a comment, why they are not reached.

WebCore/svg/SVGPathBuilder.h:50
 +	virtual void closePath();
Please remove the comments around those bools.

WebCore/svg/SVGPathConsumer.h:33
 +  enum CoordinateMode {
Either both enums should have the PathData prefix or not.

WebCore/svg/SVGPathConsumer.h:38
 +  enum PathDataParsingMode {
Suggestion, use PathParsingMode here, and PathCoordinateMode in the first enum.


WebCore/svg/SVGPathConsumer.h:43
 +  class SVGPathConsumer {
Excellent interface! :-)

WebCore/svg/SVGPathParser.cpp:26
 +  #if ENABLE(SVG)
Newline between config.h and #if.

WebCore/svg/SVGPathParser.cpp:33
 +  static const float oneOverThree = 1 / 3.f;
Did we have an agreement on howto name globals? Not sure on my own, but i'd
suggest a gOneOverThree prefix, or sth. like that.

WebCore/svg/SVGPathParser.cpp:37
 +  SVGPathParser::SVGPathParser()
That's wrong you shouldn't define the ctor and dtor here. Just leave the header
as is, and do not implement the ctor/dtor - that will make the class
inconstructable, as you want it (only has static methods).

WebCore/svg/SVGPathParser.cpp:58
 +	char command = *(ptr++), lastCommand = ' ';
Use two lines here.

WebCore/svg/SVGPathParser.cpp:97
 +	    case 'h':
Please unify 'h' and 'H', just like you did it for 'l' and 'L'.

WebCore/svg/SVGPathParser.cpp:115
 +	    case 'v':
Ditto for 'v' and 'V'.

WebCore/svg/SVGPathParser.cpp:155
 +		    px1 = mode == RelativeCoordinates ? curX + x1 : x1;
This repeated mode check doesn't make much sense. Use one branch.
px1 = curX;
py1 = curY;
..

if (mode == RelativeCoordinates) {
    px1 += curX;
    py1 += curY;
    ..
}

WebCore/svg/SVGPathParser.cpp:192
 +		    px2 = mode == RelativeCoordinates ? curX + x2 : x2;
Ditto, use one if branch for the RelativeCoordinates mode.

WebCore/svg/SVGPathParser.cpp:216
 +		    px1 = mode == RelativeCoordinates ? (curX + 2 * (x1 +
curX)) * oneOverThree : (curX + 2 * x1) * oneOverThree;
Ditto.

WebCore/svg/SVGPathParser.cpp:249
 +		    px1 = mode == RelativeCoordinates ? (curX + 2 * xc) *
oneOverThree : (curX + 2 * xc) * oneOverThree;
Ditto.

WebCore/svg/SVGPathParser.cpp:280
 +		rx = abs(rx);
s/abs/fabs/ here?

WebCore/svg/SVGPathParser.cpp:45
 +  bool SVGPathParser::parsePathDataString(SVGPathConsumer* consumer, const
String& s, bool normalized)
A general suggestion: can you move _all_ the switch branches, into seperated
non-static member functions. That would make this central, important function
much more readable.
This involves making all the variables toX / toY, etc..  members of
SVGPathParser, and not creating a static parsePathDataString function. Then of
course SVGPathParser needs a contructor again, taking the consumer, and the
normalized bool.

Usage:
SVGPathParser parser(myConsumer, true/false);
bool result = parser.parseFromString(myDAttributeString);
...

I know this is a stupid job, but it will help a lot to make this more
readable/understandable.

Example for 'm' / 'M':

switch (command) {
case 'm':
    if (!parseMoveToSegment())
	return false;
    m_closed = false;
    break;
case 'M':
    if (!parseMoveToSegment())
	return false;
    m_closed = false;
    break;
 ...

bool SVGPathParser::parseMoveToSegment()
{
    if (!parseNumber(m_ptr, m_end, m_toX) || !parseNumber(m_ptr, m_end, m_toY))

	return false;

    if (m_normalized) {
	m_subPathX = m_curX = m_mode == RelativeCoordinates ? m_curX + m_toX :
m_toX;
	m_subPathY = m_curY = m_mode == RelativeCoordinates ? m_curY + m_toY :
m_toY;
	m_consumer->moveTo(FloatPoint(m_curX, m_curY), m_closed,
AbsoluteCoordinates);
    } else
	m_consumer->moveTo(FloatPoint(m_toX, m_toY), m_closed, m_mode);

    return true;
}

Sorry, for this late suggestion...

WebCore/svg/SVGPathParser.cpp:329
 +	    delta = FloatPoint((curX - x) / 2.f, (curY - y) / 2.f);
Isn't multiplying be 0.5f faster?

WebCore/svg/SVGPathParser.cpp:334
 +	float Pr1 = r1 * r1;
Pr1? Can you find a better name? r1Squared?

WebCore/svg/SVGPathParser.cpp:335
 +	float Pr2 = r2 * r2;
Ditto.

WebCore/svg/SVGPathParser.cpp:336
 +	float Px = temp.x() * temp.x();
Ditto. mappedXSquared?

WebCore/svg/SVGPathParser.cpp:337
 +	float Py = temp.y() * temp.y();
Ditto.

WebCore/svg/SVGPathParser.cpp:339
 +	// Spec : check if radii are large enough
Make that "Spec: check..." and add a dot.


WebCore/svg/SVGPathParser.cpp:347
 +	arcTransform.scale(1 / r1, 1 / r2);
Are r1/r2 guaranteed to be non-zero?

WebCore/svg/SVGPathParser.cpp:352
 +	if (mode == AbsoluteCoordinates) {
That whole branch can be simplified:
if (mode == AbsoluteCoordinates) {
    curX = x;
    curY = y;
} else {
    curX += x;
    curY += y;
}

point1 = arcTransform.mapArcPoint(FloatPoint(curX, curY));

WebCore/svg/SVGPathParser.cpp:368
 +	float sfactorSq = 1 / d - 0.25f;
sfactorSq?

WebCore/svg/SVGPathParser.cpp:372
 +	float sfactor = sqrtf(sfactorSq);
Ah, sFactorSq == sFactorSquared?
Can you name it "scaleFactor" / "scaleFactorSquared"?

WebCore/svg/SVGPathParser.cpp:389
 +	int nSegs = ceilf(fabsf(thArc / (piFloat * 0.5f + 0.001f)));
This 0.001f stuff, can't be correct. Check wheter you want floor / ceil or
round here.

WebCore/svg/SVGPathParser.cpp:389
 +	int nSegs = ceilf(fabsf(thArc / (piFloat * 0.5f + 0.001f)));
Also nSegs, is a bad name, just like th0/th1, thArc, etc. I know these are old
legacy names, but you should take the opportunity to rename them all.

WebCore/svg/SVGPathParser.h:29
 +  
Newline can be removed here.

WebCore/svg/SVGPathParser.h:35
 +  class SVGPathParser {
As noted in the comments for the cpp file, I think it would be better to move
all the variables toX, toY, etc. as member variables into SVGPathParser, then
make SVGPathParser a class, with non-static parseFromString etc. methods, and
have private methods handling all segments
(parseMoveToSegment/parseLineToSegment etc.) to be more OOP like, and to make
the central methods more readable.

WebCore/svg/SVGPathSegListBuilder.cpp:51
 +	for (size_t i = 0; i < size; ++i) {
The vector should be iterated using iterators.

WebCore/svg/SVGPathSegListBuilder.cpp:53
 +	    segList->appendItem(m_vector[i].release(), ec);
An ASSERT(!ec) is missing below the appendItem.

WebCore/svg/SVGPathSegListBuilder.h:35
 +  class SVGPathSegListBuilder : private SVGPathConsumer {
As noted somewhere above, give this class a constructor, and pass the
SVGPathSegList*.

Sorry for this lengthy review :( Don't hate me! :-)

More information about the webkit-reviews mailing list