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

[bugzilla-daemon at webkit.org]

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


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #60120|                            |review-
               Flag|                            |




--- Comment #2 from Nikolas Zimmermann <zimmermann at kde.org>  2010-06-30 09:51:49 PST ---
(From update of attachment 60120)
> 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.