Summary: | Add missing SVGPathSegList source for SVGPathParser | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Schulze <krit> | ||||||||
Component: | SVG | Assignee: | 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
Dirk Schulze
2010-08-08 10:07:31 PDT
Created attachment 63847 [details]
Patch
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.
(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. (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 :-/ (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. Created attachment 63982 [details]
Patch
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 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.
Created attachment 63986 [details]
Patch
Comment on attachment 63986 [details]
Patch
Looks good, r=me.
Comment on attachment 63986 [details] Patch Clearing flags on attachment: 63986 Committed r65059: <http://trac.webkit.org/changeset/65059> All reviewed patches have been landed. Closing bug. |