Bug 41410 – SVG CleanUp of SVGPathData parsing

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

CleanUp of SVGPathData parsing.

[Dirk Schulze]

Created attachment 60120 [details]
proposal for a CleanUp

Proposal for a CleanUp. A new patch will prepare the other build systems beside Gtk.

[Nikolas Zimmermann]

Comment on attachment 60120 [details]
proposal for a CleanUp

> Index: WebCore/GNUmakefile.am
> ===================================================================
Nice cleanup, several comments following:

WebCore/svg/SVGParserUtilities.cpp:130
 +  bool parseArcFlag(const UChar*& ptr, const UChar* end, bool& flag)
parseArcFlag can be moved to SVGPathParser, no?

WebCore/svg/SVGPathBuilder.cpp:33
 +  SVGPathBuilder::SVGPathBuilder(Path& path) : m_path(path)
The initialization of m_path has to go on the next line.

WebCore/svg/SVGPathBuilder.cpp:39
 +      m_path = Path();
This is wrong, the code can't work. Two issues: a) you're assigning a temporary object, to the m_path reference here - it shouldn't even compile. b) You're not operating on the passed path anymore. Just remove it and be happy.

WebCore/svg/SVGPathBuilder.cpp:40
 +      if (!SVGPathParser::parsePathDataString(this, d, true))
The last argument for parsePathDataString should be an enum, as bools are confusing.

enum PathDataParsingMode {
    NormalizedParsing,
    UnalteredParsing
};

Maybe come up with better names though.

WebCore/svg/SVGPathBuilder.cpp:41
 +          return false;
You can just "return SVGPathParser::parsePathDataString(this, d, true)" here.

WebCore/svg/SVGPathBuilder.cpp:48
 +      if (closed)
Do we actually need the "bool closed" parameter? Can't we just let the SVGPathParser, call consumer->closePath(); should be less confusing, then handling sub path closing here.

WebCore/svg/SVGPathBuilder.cpp:61
 +      FloatPoint p1 = point1;
These temporary variables are unfortunate, you can avoid them completly:

if (mode == RelativeCoordinates) {
    m_path.addBezierCurveTo(point1 + m_current, p2 + m_current, point + m_current);
else {
    m_path.addBezierCurveTo(point1, point2, point) ;
    m_current = point;
}

Note: The current code can't even work, as you were passing point1, point2 to addBezierCurveTo instead of p1, p2 ;-)


WebCore/svg/SVGPathBuilder.h:42
 +      virtual void moveTo(const FloatPoint&, bool closed, CoordinateMode mode = AbsoluteCoordinates);
You can avoid all default parameters for CoordinateMode in all functions.

WebCore/svg/SVGPathConsumer.h:33
 +          AbsoluteCoordinates,
Indention is wrong.

WebCore/svg/SVGPathConsumer.h:39
 +      virtual ~SVGPathConsumer() { }
You should define a default constructor and destructor and make them both protected.

WebCore/svg/SVGPathConsumer.h:41
 +      virtual void moveTo(const FloatPoint&, bool closed, CoordinateMode mode = AbsoluteCoordinates) = 0;
No default mode needed in all functions.


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

WebCore/svg/SVGPathParser.cpp:35
 +  bool SVGPathParser::parsePathDataString(SVGPathConsumer* consumer, const String& s, bool normalized)
I did NOT check this code in detail. Will do soon, need to do some shopping first.

WebCore/svg/SVGPathParser.cpp:40
 +      // FIXME: Do we realy need to parse numbers as double, if we shrink it to float afterwards?
Try without! :-)

WebCore/svg/SVGPathParser.h:24
 +  #ifndef SVGPathParser_h
Newline between license comment and #ifndef.

WebCore/svg/SVGPathParser.h:28
 +  
No need for this newline here.

WebCore/svg/SVGPathParser.h:36
 +      virtual ~SVGPathParser() { }
No need for a virtual destructor, nor a constructor. I'd suggest to add a private, not-implemented ctor/dtor:
private:
SVGPathParser();
~SVGPathParser();

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

WebCore/svg/SVGPathSegListBuilder.h:24
 +  #ifndef SVGPathSegListBuilder_h
Newline between license comment and #ifndef.

[Dirk Schulze]

Created attachment 61154 [details]
Patch

[Dirk Schulze]

(In reply to comment #3)
> Created an attachment (id=61154) [details]
> Patch

Fixed most of the proposals of Niko and talked about other issues on IRC, uplaoding a new patch for review. The style issues mentioned by the style bot don't seem valid to me.

[Dirk Schulze]

Created attachment 61339 [details]
Patch

[Dirk Schulze]

Uploaded new patch, since the pretty-diff tool seems to crash on the diffs for svg/hixie/perf/007.xml. Removed the LAyoutTest results.

[WebKit Review Bot]

Attachment 61339 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/FloatPoint.h:125:  Missing spaces around +=  [whitespace/operators] [3]
WebCore/svg/SVGPathBuilder.cpp:65:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 2 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Comment on attachment 61339 [details]
Patch

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! :-)

[WebKit Review Bot]

Attachment 61339 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3470278

[Dirk Schulze]

Created attachment 61875 [details]
Patch

[WebKit Review Bot]

Attachment 61875 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/FloatPoint.h:84:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/FloatPoint.h:126:  Missing spaces around +=  [whitespace/operators] [3]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Comment on attachment 61875 [details]
Patch

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?

[Dirk Schulze]

Created attachment 61878 [details]
Patch

Some changes of comment from Niko.

[Dirk Schulze]

Created attachment 61881 [details]
Patch

[WebKit Review Bot]

Attachment 61881 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/FloatPoint.h:134:  Missing spaces around +=  [whitespace/operators] [3]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Comment on attachment 61881 [details]
Patch

Ok, I tried hard, that's all I could find: :-)

JavaScriptCore/wtf/MathExtras.h:58
 +  const double piOverTwoDouble = 1.57079632679489661923;
Needs a ChangeLog.

WebCore/ChangeLog:5
 +          SVG CleanUp of SVGPathData parsing
Please provide a _detailed_ ChangeLog what happens, so others know it too :-)

WebCore/WebCore.vcproj/WebCore.vcproj:2197
 +  
Still newlines here, please remove.

WebCore/svg/SVGPathConsumer.h:60
 +  
A newline too much.

WebCore/svg/SVGPathParser.cpp:65
 +          if (m_mode == RelativeCoordinates)
This still looks weird. How about:

if (m_mode == RelativeCoordinates)
    m_currentPoint += toPoint;
else
    m_currentPoint = toPoint;

m_subPathPoint = m_currentPoint;

WebCore/svg/SVGPathParser.cpp:86
 +              m_currentPoint = m_currentPoint + toPoint;
m_currentPoint += toPoint;

WebCore/svg/SVGPathParser.cpp:175
 +      if (!(m_lastCommand == 'c'
How about reversing this check:
if (m_lastCommand != 'c' && m_...)
Boolean algebra simplifications :-)

WebCore/svg/SVGPathParser.cpp:243
 +      if (!(m_lastCommand == 'q'
Ditto.

WebCore/svg/SVGPathParser.cpp:274
 +      bool largeArc, sweep;
Two lines needed unfortuantely.

WebCore/svg/SVGPathParser.cpp:290
 +      // Spec: radii are nonnegative numbers
Make this a whole sentence please with a trailing period.

WebCore/svg/SVGPathParser.cpp:402
 +              if (command == 'M')
I wish this was commented :(

WebCore/svg/SVGPathParser.cpp:404
 +              else if (command == 'm')
Ditto. But maybe you don't even know why it's needed? Given that it's old legacy code...

WebCore/svg/SVGPathParser.cpp:454
 +      if (scaleFactorSquared < 0)
Can you use float scaleFactorSquared = std::min(1 / d - 0.25f, 0)?

WebCore/svg/SVGPathParser.cpp:484
 +          if (!isfinite(t))
This seems wrong, isfinite returns a non-zero value if it's finite! So you return here, if the value is finite.

[Nikolas Zimmermann]

(In reply to comment #16)
> 
> WebCore/svg/SVGPathParser.cpp:484
>  +          if (!isfinite(t))
> This seems wrong, isfinite returns a non-zero value if it's finite! So you return here, if the value is finite.

Hah, the apple doc is wrong: "man isfinite" on Leopard states this, but it's not true.

[Dirk Schulze]

Created attachment 61934 [details]
Clean-up patch

The clean-up patch.

[Dirk Schulze]

Created attachment 61935 [details]
LayoutTest updates

LayoutTest updates.

[Nikolas Zimmermann]

Comment on attachment 61934 [details]
Clean-up patch

What a _wonderful_ patch! Took a while, but I'd say it's excellent now :-)

[Nikolas Zimmermann]

Comment on attachment 61935 [details]
LayoutTest updates

Sorrry, the LayoutTestChanges patch seems to crash review/formatted diff tool, I can't look at the pixel test results at the moment - I trust you they are all progressions!
I only checked the textual results, and they look okay to me, though it's very hard to spot the differences w/o the review tool.

If you prefer you can wait until landing, and let me have another look - or land it as is.
Will be away all day though.

[Dirk Schulze]

Committed r63721: <http://trac.webkit.org/changeset/63721>

[WebKit Review Bot]

http://trac.webkit.org/changeset/63721 might have broken Qt Linux Release