Bug 12122 - SVGPathElement should not inherit from SVGPathParser
Summary: SVGPathElement should not inherit from SVGPathParser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-01-05 01:09 PST by Eric Seidel (no email)
Modified: 2007-06-08 09:57 PDT (History)
2 users (show)

See Also:


Attachments
First attempt (4.61 KB, patch)
2007-06-03 00:38 PDT, Rob Buis
eric: review-
Details | Formatted Diff | Diff
Addressing Eric's issues (29.82 KB, patch)
2007-06-07 09:32 PDT, Rob Buis
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2007-01-05 01:09:40 PST
SVGPathElement should not inherit from SVGPathParser

The current design makes it very difficult for things like SVGAnimateMotionElement to parse paths.  They have to create a heavy-weight SVGPathElement, when all they want is something capable of parsing a path into a SVGPathSegList

SVGPathElement is-a SVGPathParser, when it really should have-a SVGPathParser.  all of the "this->createMoveTo" calls would just change to "pathCreator->createMoveTo" calls, and the element could be passed into the SVGPathParser.  There could even be an SVGSegList specific subclass which took a seglist (or created one and then returned it).  SVGPathSegListPathParser perhaps?
Comment 1 Rob Buis 2007-06-03 00:38:01 PDT
Created attachment 14845 [details]
First attempt

This is not a full fix for this bug, but it helps the SVGAnimateMotionElement problem. Let me know if that is enough for this bug and whether SVGPathElement should be changed too.
Cheers,

Rob.
Comment 2 Eric Seidel (no email) 2007-06-05 15:40:05 PDT
Comment on attachment 14845 [details]
First attempt

It's a rather incomplete fix.  Basically you're just adding another class to codify the existing hack of needing to allocate a class to do the parsing.  If you're going to add this PathBuilder class, i would think you would make SVGPathElement use it too.  I don't think that this fix is really much better than the existing hack.  If you got rid of the other instances of classes inheriting from SVGPathParser, that would be a more complete fix.  We can chat about this on IRC if you like.
Comment 3 Rob Buis 2007-06-07 09:32:39 PDT
Created attachment 14896 [details]
Addressing Eric's issues

This seems somewhat nicer IMHO.
Cheers,

Rob.
Comment 4 Eric Seidel (no email) 2007-06-08 02:15:55 PDT
Comment on attachment 14896 [details]
Addressing Eric's issues

Looks good to me!  Great work!
Comment 5 Rob Buis 2007-06-08 09:57:16 PDT
Landed on feature branch in r22068.