[webkit-reviews] review denied: [Bug 43691] Add missing SVGPathSegList source for SVGPathParser : [Attachment 63847] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 43691: Add missing SVGPathSegList source for SVGPathParser
https://bugs.webkit.org/show_bug.cgi?id=43691

Attachment 63847: Patch
https://bugs.webkit.org/attachment.cgi?id=63847&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Still needs some tweaks:

WebCore/ChangeLog:11
 +	    Did some refactoring of SVGPathSource, the input data of
SVGPathParser. The parsing process
Refactored SVGPathSource code, to read concrete path segments instead of
type/flag/coordinates from the data sources.
This is a further abstraction and seperates the reading of content from the
parsing and interpreting.

WebCore/svg/SVGPathByteStreamSource.cpp:79
 +  bool SVGPathByteStreamSource::parseCurveToCubicSegment(FloatPoint& point1 ,
FloatPoint& point2, FloatPoint& targetPoint)
Superfluous space before comma.

WebCore/svg/SVGPathParser.cpp:76
 +	if (!m_source->parseMoveToSegment(targetPoint))
parseLineToSegment! :-)

WebCore/svg/SVGPathSegListSource.cpp:39
 +	m_itemEnd = pathSegList->numberOfItems();
For consistency: s/pathSegList/m_pathSegList/

WebCore/svg/SVGPathSegListSource.cpp:78
 +	SVGPathSegSingleCoord* moveTo =
static_cast<SVGPathSegSingleCoord*>(m_segment);
You should ASSERT that the type is correct before casting, and also in all the
other parse*Segment functions.

WebCore/svg/SVGPathSegListSource.cpp:107
 +  bool SVGPathSegListSource::parseCurveToCubicSegment(FloatPoint& point1 ,
FloatPoint& point2, FloatPoint& targetPoint)
Superfluous space before comma.

WebCore/svg/SVGPathSegListSource.cpp:138
 +	SVGPathSegSingleCoord* quadraticSmooth =
static_cast<SVGPathSegSingleCoord*>(m_segment);
You can cast to the concrete SVGPathSegCurveToQudaraticSmoothSegment, no?

WebCore/svg/SVGPathSegListSource.h:60
 +	SVGPathSeg* m_segment;
Hm, as SVGPathSeg is refcounted, you'd better store a RefPtr<SVGPathSeg> here,
it's safer.
Instead of using static_cast to cast from SVGPathSeg to the individual types,
you can use static_pointer_cast, that gives the same functionality but operates
on RefPtrs.

WebCore/svg/SVGPathStringSource.cpp:164
 +  bool SVGPathStringSource::parseCurveToCubicSegment(FloatPoint& point1 ,
FloatPoint& point2, FloatPoint& targetPoint)
Superfluous space before comma.