825732 - Convert SVG text element classes to WebIDL

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Convert SVGTextContentElement to WebIDL

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: SVGTextPathElement

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: SVGTextPositioningElement

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: SVGTextElement

Comment 5

[David Zbarsky (:dzbarsky)]

Attachment: SVGTSpanElement

Comment 6

[David Zbarsky (:dzbarsky)]

Attachment: SVGAltGlyphElement

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696862 [details] [diff] [review]
Convert SVGTextContentElement to WebIDL

r=me

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696863 [details] [diff] [review]
SVGTextPathElement

>+  *aResult = ToDOMAnimatedEnum().get();

Should pass aSVGElement there, no?  To get it to compile and all.... ;)

r=me with that

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696864 [details] [diff] [review]
SVGTextPositioningElement

The changes to SVGTextContentElement.h/cpp and SVGTextPathElement.h/cpp and nsSVGEnum.cpp and Bindings.conf seem like they should be in the previous patch, no?

r=me with that fixed.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696865 [details] [diff] [review]
SVGTextElement

>  * This class does not inherit nsSVGTextPositioningElement - it reimplements it

That comment seems to no longer be true.

r=me on the WebIDL bits, but I'd like someone who knows the SVG code to OK the other goings on here.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696866 [details] [diff] [review]
SVGTSpanElement

The SVGTextElement.h bits seem to be refugees from the previous patch.  Please move them there?

r=me with that.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696870 [details] [diff] [review]
SVGAltGlyphElement

>+++ b/dom/webidl/SVGAltGlyphElement.webidl
>+  attribute DOMString glyphRef;
>+  attribute DOMString format;

These should be [SetterThrows] and you need setters that do the right thing.

r=me with that.

Comment 13

[Robert Longson [:longsonr]]

Comment on attachment 696865 [details] [diff] [review]
SVGTextElement

> 
>-typedef dom::SVGGraphicsElement nsSVGTextElementBase;
>+typedef SVGTextPositioningElement nsSVGTextElementBase;

Why the swap over? Either one will do so if this is simpler that's fine, but if you swap base classes you have to implement the other interface...

> 
> /**
>  * This class does not inherit nsSVGTextPositioningElement - it reimplements it
>  * instead.
>  *
>  * Ideally this class would inherit nsSVGTextPositioningElement in addition to
>  * nsSVGGraphicElement, but we don't want two instances of nsSVGStylableElement
>  * and all the classes it inherits. Instead we choose to inherit one of the
>  * classes (nsSVGGraphicElement) and reimplement the missing pieces from the
>  * other (nsSVGTextPositioningElement (and thus nsSVGTextContentElement)). Care
>  * must be taken when making changes to the reimplemented pieces to keep
>  * nsSVGTextPositioningElement in sync (and vice versa).
>  */

Given the above swap, you're trying to things the other way round (maybe).

>-class nsSVGTextElement : public nsSVGTextElementBase,
>-                         public nsIDOMSVGTextElement // nsIDOMSVGTextPositioningElement
>+class SVGTextElement : public SVGTextElementBase,
>+                       public nsIDOMSVGTextElement // nsIDOMSVGTextPositioningElement

Where is SVGTransformable coming from?

>   NS_DECL_ISUPPORTS_INHERITED
>   NS_DECL_NSIDOMSVGTEXTELEMENT
>-  NS_DECL_NSIDOMSVGTEXTPOSITIONINGELEMENT
>-  NS_DECL_NSIDOMSVGTEXTCONTENTELEMENT
> 
>   // xxx If xpcom allowed virtual inheritance we wouldn't need to
>   // forward here :-(
>   NS_FORWARD_NSIDOMNODE_TO_NSINODE
>   NS_FORWARD_NSIDOMELEMENT_TO_GENERIC
>-  NS_FORWARD_NSIDOMSVGELEMENT(nsSVGTextElementBase::)
>+  NS_FORWARD_NSIDOMSVGELEMENT(SVGTextElementBase::)

Seems to be missing SVGTransformable stuff if you're going to do it this way.

I'm going to give up at this point as if SVGTransformable is missing in the derivation you're going to have missed out implementing it below.

Comment 14

[Robert Longson [:longsonr]]

The other think SVGText needs of course is your favourite class/interface DOMSVGTests/nsIDOMSVGTests! nsSVGGraphicElement gave it that previously.

Comment 15

[David Zbarsky (:dzbarsky)]

(In reply to Robert Longson from comment #13)
> Comment on attachment 696865 [details] [diff] [review]
> SVGTextElement
> 
> > 
> >-typedef dom::SVGGraphicsElement nsSVGTextElementBase;
> >+typedef SVGTextPositioningElement nsSVGTextElementBase;
> 
> Why the swap over? Either one will do so if this is simpler that's fine, but
> if you swap base classes you have to implement the other interface...

The other patches in this bug change the inheritance chain, so we end up with SVGTextElement <= SVGTextPositioningelement <= SVGTextContentElement <= SVGGraphicsElement.

Comment 16

[Robert Longson [:longsonr]]

If you do that you'll run into problems with text child elements.

None of tspan, tref or altglyph can implement DOMSVGTests.

tspan and altglyphelement are not SVGTransformable but it is an SVGTextContentElement.

tref is not SVGTransformable but it is an SVGTextPositioningElement

Comment 17

[David Zbarsky (:dzbarsky)]

(In reply to Robert Longson from comment #16)
> If you do that you'll run into problems with text child elements.
> 
> None of tspan, tref or altglyph can implement DOMSVGTests.

These all implement SVGTests in SVG1.1 and SVG2.

> tspan and altglyphelement are not SVGTransformable but it is an
> SVGTextContentElement.
> 
> tref is not SVGTransformable but it is an SVGTextPositioningElement

Hmm, looks like they were changed to be transformable in SVG2.  Cameron?

Comment 18

[Robert Longson [:longsonr]]

(In reply to David Zbarsky (:dzbarsky) from comment #17)
> 
> These all implement SVGTests in SVG1.1 and SVG2.

My mistake, you're right there.

Comment 19

[Cameron McCormack (:heycam)]

Yes, SVG 2 makes <tspan>s etc. transformable, but that's something we haven't implemented yet.  It might be easier to have SVGTextContentElement not inherit from SVGGeometryElement until we get to that, I'm not sure.

Comment 20

[Robert Longson [:longsonr]]

As long as you think your current implementation is something your patches in bug 655877 can be modified to implement at some future date and that tspan being transformable is not going to change again in SVG 2 then I guess it's OK to go with this structure. It's simpler to implement in content.

Comment 21

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02f069bc1cc3
https://hg.mozilla.org/integration/mozilla-inbound/rev/a671c210b131
https://hg.mozilla.org/integration/mozilla-inbound/rev/370b247b1b7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/31ced8e31f8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4ba770505d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/59ae61d1d059

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/02f069bc1cc3
https://hg.mozilla.org/mozilla-central/rev/a671c210b131
https://hg.mozilla.org/mozilla-central/rev/370b247b1b7f
https://hg.mozilla.org/mozilla-central/rev/31ced8e31f8b
https://hg.mozilla.org/mozilla-central/rev/c4ba770505d9
https://hg.mozilla.org/mozilla-central/rev/59ae61d1d059

Comment 23

[[e-mail]]

Comment on attachment 696862 [details] [diff] [review]
Convert SVGTextContentElement to WebIDL

>+already_AddRefed<nsISVGPoint>
>+SVGTextContentElement::GetStartPositionOfChar(uint32_t charnum, ErrorResult& rv)
>+{
>+  nsSVGTextContainerFrame* metrics = GetTextContainerFrame();
>+
>+  if (!metrics) {
>+    rv.Throw(NS_ERROR_FAILURE);
>+    return nullptr;
>+  }
>+
>+  nsCOMPtr<nsISVGPoint> point;
>+  rv = metrics->GetStartPositionOfChar(charnum, getter_AddRefs(point));
As of bug 817442, nsSVGTextContainerFrame::GetStartPositionOfChar is declared to "return" an nsISupports*, and you're losing compile-time type checking because getter_AddRefs allows a silent upcast :-(