make a base class for the SVGXxxList implementations

RESOLVED WONTFIX

Status

()

Core
SVG
RESOLVED WONTFIX
12 years ago
5 years ago

People

(Reporter: jwatt, Assigned: Jeff Schiller)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

12 years ago
The code implementing these classes has diverged. A base class would rectify
this and prevent it from happening again, not to mention reduce bloat.
(Reporter)

Updated

12 years ago
Blocks: 254712
(Assignee)

Comment 1

11 years ago
SVGPathSegList
SVGTransformList
SVGPointList
SVGLengthList
SVGNumberList
SVGStringList (not implemented)
(Assignee)

Comment 2

10 years ago
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.
Attachment #250685 - Flags: review?(jwatt)
(Assignee)

Comment 3

10 years ago
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.
(Reporter)

Updated

10 years ago
Assignee: jwatt → codedread
(Assignee)

Comment 4

10 years ago
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.
(Assignee)

Comment 5

10 years ago
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

10 years ago
(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.


(Reporter)

Updated

10 years ago
Attachment #250685 - Flags: review?(jwatt)
(Assignee)

Comment 7

9 years ago
Note.  Must make sure the base class takes into account Bug 435209.
QA Contact: ian → general
(Reporter)

Comment 8

5 years ago
The list code was rewritten in bug 515116.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.