Bug 43618 - Generalize SVGPathParser to allow more than just strings as input source
Summary: Generalize SVGPathParser to allow more than just strings as input source
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-06 04:50 PDT by Dirk Schulze
Modified: 2010-08-06 23:27 PDT (History)
2 users (show)

See Also:


Attachments
Patch (88.05 KB, patch)
2010-08-06 05:19 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (80.48 KB, patch)
2010-08-06 07:13 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (90.36 KB, patch)
2010-08-06 15:16 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (83.86 KB, patch)
2010-08-06 22:43 PDT, Dirk Schulze
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2010-08-06 04:50:08 PDT
Refactor SVGPathParser to transform different sources into different SVGPathConsumer.
Comment 1 Dirk Schulze 2010-08-06 05:19:32 PDT
Created attachment 63715 [details]
Patch
Comment 2 Dirk Schulze 2010-08-06 07:13:14 PDT
Created attachment 63719 [details]
Patch
Comment 3 Nikolas Zimmermann 2010-08-06 07:35:17 PDT
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.
Comment 4 Nikolas Zimmermann 2010-08-06 07:36:31 PDT
(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 :-)
Comment 5 Eric Seidel (no email) 2010-08-06 07:49:57 PDT
Attachment 63719 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3669014
Comment 6 Nikolas Zimmermann 2010-08-06 08:01:41 PDT
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?
Comment 7 Dirk Schulze 2010-08-06 09:29:20 PDT
>  +      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.
Comment 8 Dirk Schulze 2010-08-06 15:16:25 PDT
Created attachment 63768 [details]
Patch
Comment 9 Dirk Schulze 2010-08-06 15:19:59 PDT
(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.
Comment 10 Dirk Schulze 2010-08-06 22:43:02 PDT
Created attachment 63806 [details]
Patch
Comment 11 Nikolas Zimmermann 2010-08-06 23:12:11 PDT
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' ...
Comment 12 Dirk Schulze 2010-08-06 23:27:54 PDT
Committed r64898: <http://trac.webkit.org/changeset/64898>