282247 - make a base class for the SVGXxxList implementations

Status: RESOLVED WONTFIX

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

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]

SVGPathSegList
SVGTransformList
SVGPointList
SVGLengthList
SVGNumberList
SVGStringList (not implemented)

Comment 2

[Jeff Schiller]

Attachment: 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]

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]

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 5

[Jeff Schiller]

Note to self.  Must make sure the base class takes into account Bug 369249.  See http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=&subdir=mozilla/content/svg/content/src&command=DIFF_FRAMESET&root=&file=nsSVGLengthList.cpp&rev1=1.11&rev2=1.12

Comment 6

[Robert Longson [:longsonr]]

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

Note.  Must make sure the base class takes into account Bug 435209.

Comment 8

[Jonathan Watt [:jwatt]]

The list code was rewritten in bug 515116.