561654 - nsSVGTextPositioningElement should be on the primary inheritance chain

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

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.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Robert Longson [: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.

Comment 3

[Robert Longson [:longsonr]]

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

Comment 4

[Jonathan Watt [:jwatt]]

Thanks. :-) Good catches. I've made those changes locally.

Comment 5

[Robert Longson [:longsonr]]

Are you going to check this in soon?

Comment 6

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/17a065ad96f4