[webkit-reviews] review denied: [Bug 41410] SVG CleanUp of SVGPathData parsing : [Attachment 60120] proposal for a CleanUp

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied  review:
Bug 41410: SVG CleanUp of SVGPathData parsing
https://bugs.webkit.org/show_bug.cgi?id=41410

Attachment 60120: proposal for a CleanUp
https://bugs.webkit.org/attachment.cgi?id=60120&action=review

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