Closed Bug 561654 Opened 10 years ago Closed 10 years ago

nsSVGTextPositioningElement should be on the primary inheritance chain

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jwatt, Assigned: jwatt)

Details

Attachments

(1 file)

nsSVGTextPositioningElement doesn't inherit from nsSVGElement, which is a problem when it comes to refactoring if its methods need to be able to find its nsSVGElement. We really want attributes to be defined on the primary inheritance chain only.

As suggested by roc, I'm changing things to make nsSVGTextContentElement inherit nsSVGStylableElement, and then have nsSVGTSpanElement, nsSVGTextPathElement and nsSVGAltGlyphElement inherit nsSVGTextPositioningElement instead of nsSVGStylableElement. nsSVGTextElement is a bit more tricky since it inherits nsSVGGraphicElement, so it reimplements nsIDOMSVGTextPositioningElement and nsIDOMSVGTextContentElement rather than inheriting nsSVGTextPositioningElement.
Attached patch patchSplinter Review
Assignee: nobody → jwatt
Status: NEW → ASSIGNED
Attachment #441384 - Flags: review?(longsonr)
Comment on attachment 441384 [details] [diff] [review]
patch

>+
>+  nsSVGTextContentElement(nsINodeInfo *aNodeInfo)
>+    : nsSVGStylableElement(aNodeInfo)

Best to create a nsSVGTextContentElementBase you can use here.

>+ * must be taken when making changes to the reimplemented pieces to keep
>+ * nsSVGTextPositioningElement in sync (and vica versa).

s/vica versa/vice versa/

>diff --git a/content/svg/content/src/nsSVGTextPositioningElement.h b/content/svg/content/src/nsSVGTextPositioningElement.h

...

>+
>+  nsSVGTextPositioningElement(nsINodeInfo *aNodeInfo)
>+    : nsSVGTextContentElement(aNodeInfo)

And a nsSVGTextPositioningElementBase for here.


Also you should remove the 

  friend class nsSVGTextPositioningElement;

line from nsSVGElement.h

r=longsonr with review comments addressed.
Attachment #441384 - Flags: review?(longsonr) → review+
One other thing....

virtual nsSVGTextContainerFrame* GetTextContainerFrame()

This method no longer needs to be virtual after your changes. You can have a concrete implementation in nsSVGTextElement and another in nsSVGTextContentElement
Thanks. :-) Good catches. I've made those changes locally.
Are you going to check this in soon?
Pushed http://hg.mozilla.org/mozilla-central/rev/17a065ad96f4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.