Bug 43618 – Generalize SVGPathParser to allow more than just strings as input source

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

Refactor SVGPathParser to transform different sources into different SVGPathConsumer.

[Dirk Schulze]

Created attachment 63715 [details]
Patch

[Dirk Schulze]

Created attachment 63719 [details]
Patch

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

(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 :-)

[Eric Seidel (no email)]

Attachment 63719 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3669014

[Nikolas Zimmermann]

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?

[Dirk Schulze]

>  +      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.

[Dirk Schulze]

Created attachment 63768 [details]
Patch

[Dirk Schulze]

(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.

[Dirk Schulze]

Created attachment 63806 [details]
Patch

[Nikolas Zimmermann]

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' ...

[Dirk Schulze]

Committed r64898: <http://trac.webkit.org/changeset/64898>