Bug 43691 – Add missing SVGPathSegList source for SVGPathParser

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

Add missing SVGPathSegList source for SVGPathParser, to create different Path data from SVGPathSegList.

[Dirk Schulze]

Created attachment 63847 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 63847 [details]
Patch

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.

[Dirk Schulze]

(In reply to comment #2)
> (From update of attachment 63847 [details])

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

Sure, but I want to avoid unneccessary includes here. The same for lineTo and moveTo. All of them need to be included, just to get x and y. Not needed in my eyes.

[Dirk Schulze]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 63847 [details] [details])
> 
> > WebCore/svg/SVGPathSegListSource.cpp:138
> >  +      SVGPathSegSingleCoord* quadraticSmooth = static_cast<SVGPathSegSingleCoord*>(m_segment);
> > You can cast to the concrete SVGPathSegCurveToQudaraticSmoothSegment, no?
> 
> Sure, but I want to avoid unneccessary includes here. The same for lineTo and moveTo. All of them need to be included, just to get x and y. Not needed in my eyes.

With ther type ASSERT you mentioned above, I had to include all these files and have to write something like:

    ASSERT(static_cast<SVGPathSegType>(m_segment->pathSegType()) == PathSegMoveToAbs
           || static_cast<SVGPathSegType>(m_segment->pathSegType()) == PathSegMoveToRel);

for every function :-/

[Nikolas Zimmermann]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 63847 [details] [details])
> 
> > WebCore/svg/SVGPathSegListSource.cpp:138
> >  +      SVGPathSegSingleCoord* quadraticSmooth = static_cast<SVGPathSegSingleCoord*>(m_segment);
> > You can cast to the concrete SVGPathSegCurveToQudaraticSmoothSegment, no?
> 
> Sure, but I want to avoid unneccessary includes here. The same for lineTo and moveTo. All of them need to be included, just to get x and y. Not needed in my eyes.

Ok, you can leave it as is.

(In reply to comment #4)
> 
> With ther type ASSERT you mentioned above, I had to include all these files and have to write something like:
> 
>     ASSERT(static_cast<SVGPathSegType>(m_segment->pathSegType()) == PathSegMoveToAbs
>            || static_cast<SVGPathSegType>(m_segment->pathSegType()) == PathSegMoveToRel);

Not at all, why would you want to cast?
ASSERT(m_segment->pathSegType() == PathSegMoveToAbs || m_segment->pathSegType() == PathSegMoveToRel); would work fine.

[Dirk Schulze]

Created attachment 63982 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 63982 [details]
Patch

Ok, I gave a bad suggestion. Storing m_segment as RefPtr is fine, but using static_pointer_cast is pointless, as we'd ref/and deref the object during RefPtr<> creation and destruction, in parse*Segment, which is unncessary and just creates usless refs/derefs.

Leave RefPtr<SVGPathSeg> m_segment this is fine, but use:
SVGPathSegSingleCoord* moveTo = static_cast<SVGPathSegSingleCoord*>(m_segment.get());

here. Sorry for the wrong suggestion.

[Nikolas Zimmermann]

Comment on attachment 63982 [details]
Patch

Forgot to say, other than that it looks great. Please fix the casts, then I'll r+ it... r- for now as EWS is done.

[Dirk Schulze]

Created attachment 63986 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 63986 [details]
Patch

Looks good, r=me.

[Dirk Schulze]

Comment on attachment 63986 [details]
Patch

Clearing flags on attachment: 63986

Committed r65059: <http://trac.webkit.org/changeset/65059>

[Dirk Schulze]

All reviewed patches have been landed.  Closing bug.