Summary: | Generalize SVGPathParser to allow more than just strings as input source | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Schulze <krit> | ||||||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | eric, zimmermann | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Dirk Schulze
2010-08-06 04:50:08 PDT
Created attachment 63715 [details]
Patch
Created attachment 63719 [details]
Patch
Comment on attachment 63719 [details]
Patch
Looks like you're almost there, a small design problem remains:
The SVGPathSource passed to parsePathDataFromSource, is stored in an OwnPtr in SVGPathParser. As SVGPathParser is a static object in SVGPathParserFactory, no one will delete the SVGPathSource.
How about changing the design a little, by storing a SVGPathSource* pointer in SVGPathParser, just like it's done for SVGPathConsumer, plus a new setCurrentSource(SVGPathSource*) method.
Then you'd only need to modify SVGPathParserFactory, from:
bool ok = parser->parsePathDataFromSource(SVGPathStringSource::create(d), NormalizedParsing);
to:
OwnPtr<SVGPathSource> source = SVGPathStringSource::create(d);
bool ok = parser->parsePathDataFromSource(source, NormalizedParsing);
...
This way it's guaranteed that source is going to die, after the build* methods are finished.
Some const issues:
WebCore/svg/SVGPathSource.h:32
+ virtual bool hasMoreData() = 0;
This should be const.
WebCore/svg/SVGPathSource.h:35
+ virtual bool moveToNextToken() = 0;
Ditto.
WebCore/svg/SVGPathSource.h:36
+ virtual SVGPathSegType parseSVGSegmentType() = 0;
for consistency, it should be:
virtual bool parseSVGSegmentType(SVGPathSegType& result) = 0;
WebCore/svg/SVGPathSource.h:37
+ virtual SVGPathSegType nextCommand(SVGPathSegType) = 0;
maybe write 'previousCommand' as parameter name?
WebCore/svg/SVGPathStringSource.cpp:111
+ if ((*m_current == '+' || *m_current == '-' || *m_current == '.' || (*m_current >= '0' && *m_current <= '9'))
Useless outer brace.
Other than these small issues, it looks great! Looking forward to the bytestream patches :-)
Not setting r- officially, as you wanted me to wait for EWS results.
(In reply to comment #3) > (From update of attachment 63719 [details]) > Looks like you're almost there, a small design problem remains: > > The SVGPathSource passed to parsePathDataFromSource, is stored in an OwnPtr in SVGPathParser. As SVGPathParser is a static object in SVGPathParserFactory, no one will delete the SVGPathSource. > How about changing the design a little, by storing a SVGPathSource* pointer in SVGPathParser, just like it's done for SVGPathConsumer, plus a new setCurrentSource(SVGPathSource*) method. > > Then you'd only need to modify SVGPathParserFactory, from: > bool ok = parser->parsePathDataFromSource(SVGPathStringSource::create(d), NormalizedParsing); > > to: > OwnPtr<SVGPathSource> source = SVGPathStringSource::create(d); > bool ok = parser->parsePathDataFromSource(source, NormalizedParsing); > ... ops, i wanted to write: OwnPtr<SVGPathSource> source = SVGPathStringSource::create(d); parser->setCurrentSource(source.get()); bool ok = parser->parsePathData(NormalizedParsing); ... sorry :-) Attachment 63719 [details] did not build on mac: Build output: http://queues.webkit.org/results/3669014 Comment on attachment 63719 [details]
Patch
No idea, why the mac build failed. Dirk, you might want to redo the Xcode updates? Do you have access to a mac?
> + if ((*m_current == '+' || *m_current == '-' || *m_current == '.' || (*m_current >= '0' && *m_current <= '9'))
> Useless outer brace.
maybe for (*m_current >= '0' && *m_current <= '9'), but not the outer braces.
Created attachment 63768 [details]
Patch
(In reply to comment #3) > (From update of attachment 63719 [details]) > Looks like you're almost there, a small design problem remains: > > The SVGPathSource passed to parsePathDataFromSource, is stored in an OwnPtr in SVGPathParser. As SVGPathParser is a static object in SVGPathParserFactory, no one will delete the SVGPathSource. > How about changing the design a little, by storing a SVGPathSource* pointer in SVGPathParser, just like it's done for SVGPathConsumer, plus a new setCurrentSource(SVGPathSource*) method. > done. > > This way it's guaranteed that source is going to die, after the build* methods are finished. > > Some const issues: > WebCore/svg/SVGPathSource.h:32 > + virtual bool hasMoreData() = 0; > This should be const. done. > > WebCore/svg/SVGPathSource.h:35 > + virtual bool moveToNextToken() = 0; > Ditto. moveToNextToken is base uppon skipOptionalSpaces, can't const it. > > WebCore/svg/SVGPathSource.h:36 > + virtual SVGPathSegType parseSVGSegmentType() = 0; > for consistency, it should be: > virtual bool parseSVGSegmentType(SVGPathSegType& result) = 0; done > > WebCore/svg/SVGPathSource.h:37 > + virtual SVGPathSegType nextCommand(SVGPathSegType) = 0; > maybe write 'previousCommand' as parameter name? done > > WebCore/svg/SVGPathStringSource.cpp:111 > + if ((*m_current == '+' || *m_current == '-' || *m_current == '.' || (*m_current >= '0' && *m_current <= '9')) > Useless outer brace. done Builds on Mac for me. Created attachment 63806 [details]
Patch
Comment on attachment 63806 [details]
Patch
r=me, some minor nitpicks before you land:
WebCore/ChangeLog:12
+ Moved the SVGPathSegType enumeration from SVGPathSeg class for a common use across
Moved the SVGPathSegType enum from SVGPathSeg class in WebCore namespace, for easier access throughout the SVG code.
WebCore/ChangeLog:15
+ No new tests added.
Doesn't affect any tests, sounds better, creates less worries :-)
WebCore/svg/SVGPathParser.cpp:320
+ ASSERT(m_source);
Also add ASSERT(m_consumer) here.
WebCore/svg/SVGPathParser.h:44
+ void setCurrentSource(SVGPathSource* source) { m_source = source;}
Space should be added before ned brace.
WebCore/svg/SVGPathStringSource.cpp:131
+ if ((*m_current == '+' || *m_current == '-' || *m_current == '.' || *m_current >= '0' && *m_current <= '9')
Ah, I misread before, didn't see there was another statement on the next line. Please readd the braces around *m_current >= '0' ...
Committed r64898: <http://trac.webkit.org/changeset/64898> |