Refactor SVGPathParser to transform different sources into different SVGPathConsumer.
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>