Bug 43691

Summary: Add missing SVGPathSegList source for SVGPathParser
Product: WebKit Reporter: Dirk Schulze <krit>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 43696    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Dirk Schulze 2010-08-08 10:07:31 PDT
Add missing SVGPathSegList source for SVGPathParser, to create different Path data from SVGPathSegList.
Comment 1 Dirk Schulze 2010-08-08 10:31:05 PDT
Created attachment 63847 [details]
Patch
Comment 2 Nikolas Zimmermann 2010-08-09 23:08:49 PDT
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.
Comment 3 Dirk Schulze 2010-08-09 23:22:58 PDT
(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.
Comment 4 Dirk Schulze 2010-08-09 23:42:58 PDT
(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 :-/
Comment 5 Nikolas Zimmermann 2010-08-10 00:07:56 PDT
(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.
Comment 6 Dirk Schulze 2010-08-10 01:01:24 PDT
Created attachment 63982 [details]
Patch
Comment 7 Nikolas Zimmermann 2010-08-10 01:11:33 PDT
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.
Comment 8 Nikolas Zimmermann 2010-08-10 01:13:15 PDT
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.
Comment 9 Dirk Schulze 2010-08-10 01:23:22 PDT
Created attachment 63986 [details]
Patch
Comment 10 Nikolas Zimmermann 2010-08-10 01:36:49 PDT
Comment on attachment 63986 [details]
Patch

Looks good, r=me.
Comment 11 Dirk Schulze 2010-08-10 01:47:57 PDT
Comment on attachment 63986 [details]
Patch

Clearing flags on attachment: 63986

Committed r65059: <http://trac.webkit.org/changeset/65059>
Comment 12 Dirk Schulze 2010-08-10 01:48:07 PDT
All reviewed patches have been landed.  Closing bug.