[Webkit-unassigned] [Bug 41410] SVG CleanUp of SVGPathData parsing

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

https://bugs.webkit.org/show_bug.cgi?id=41410


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #61339|review?                     |review-
               Flag|                            |




--- Comment #8 from Nikolas Zimmermann <zimmermann at kde.org>  2010-07-13 01:23:57 PST ---
(From update of attachment 61339)
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! :-)

-- 
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