Last Comment Bug 282247 - make a base class for the SVGXxxList implementations
: make a base class for the SVGXxxList implementations
Status: RESOLVED WONTFIX
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Jeff Schiller
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 254712
  Show dependency treegraph
 
Reported: 2005-02-14 11:27 PST by Jonathan Watt [:jwatt]
Modified: 2012-01-27 10:16 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First draft patch of common base class(es) with a rewrite of nsSVGNumberList (31.31 KB, patch)
2007-01-06 00:36 PST, Jeff Schiller
no flags Details | Diff | Splinter Review

Description Jonathan Watt [:jwatt] 2005-02-14 11:27:53 PST
The code implementing these classes has diverged. A base class would rectify
this and prevent it from happening again, not to mention reduce bloat.
Comment 1 Jeff Schiller 2006-12-14 08:42:07 PST
SVGPathSegList
SVGTransformList
SVGPointList
SVGLengthList
SVGNumberList
SVGStringList (not implemented)
Comment 2 Jeff Schiller 2007-01-06 00:36:48 PST
Created attachment 250685 [details] [diff] [review]
First draft patch of common base class(es) with a rewrite of nsSVGNumberList

This is my first pass at a base SVG list class - it compiles but I'm not totally satisfied with the design and I need some review - basically I'm not happy with all the casting that goes on in the DOM methods inside nsSVGNumberList.cpp now.  

My original idea was to make the base list class a templated subclass, but I think this would grow the size of the executable, wouldn't it?  Let me know if you think this is a better idea.
Comment 3 Jeff Schiller 2007-01-11 07:41:03 PST
Woops, nsSVGListBase::AppendElement() needs to call EnsureElementIsParentless(aElement) just like nsSVGListBase::InsertElementAt().

Also, I notice that the DOM method Initialize (see http://www.w3.org/TR/SVG11/paths.html#InterfaceSVGPathSegList) does not explicitly state that the item inserted must be removed from its previous list, but I'm guessing that's the intent.  I'll ask for clarity from the SVG WG.
Comment 4 Jeff Schiller 2007-01-26 10:59:47 PST
jwatt has mentioned that it might be a good idea to wrap up the declaration and definition of a derived list class into a C-macro for that one-line creation goodness (to avoid the code divergence that happened).

Also, would it be possible to do this in a cleaner way if I introduced a new interface class nsISVGListItem ?  I've never done that before so I avoided it in my first patch, but maybe I'll attempt something once I get some feedback here.
Comment 6 Robert Longson 2007-03-08 07:57:21 PST
(In reply to comment #4)

> Also, would it be possible to do this in a cleaner way if I introduced a new
> interface class nsISVGListItem ?  I've never done that before so I avoided it
> in my first patch, but maybe I'll attempt something once I get some feedback
> here.
> 

I don't think you want an interface class. We're getting rid of those where possible. You just end up with lots of virtual methods and CallQueryInterface calls which are slow.


Comment 7 Jeff Schiller 2008-05-22 12:15:22 PDT
Note.  Must make sure the base class takes into account Bug 435209.
Comment 8 Jonathan Watt [:jwatt] 2012-01-27 10:16:44 PST
The list code was rewritten in bug 515116.

Note You need to log in before you can comment on or make changes to this bug.