Closed Bug 825732 Opened 7 years ago Closed 7 years ago

Convert SVG text element classes to WebIDL

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

Attachments

(6 files)

No description provided.
Attachment #696862 - Flags: review?(bzbarsky)
Attachment #696863 - Flags: review?(bzbarsky)
Attachment #696864 - Flags: review?(bzbarsky)
Attached patch SVGTextElementSplinter Review
Attachment #696865 - Flags: review?(bzbarsky)
Attached patch SVGTSpanElementSplinter Review
Attachment #696866 - Flags: review?(bzbarsky)
Attachment #696870 - Flags: review?(bzbarsky)
Comment on attachment 696862 [details] [diff] [review]
Convert SVGTextContentElement to WebIDL

r=me
Attachment #696862 - Flags: review?(bzbarsky) → review+
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
Attachment #696863 - Flags: review?(bzbarsky) → review+
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.
Attachment #696864 - Flags: review?(bzbarsky) → review+
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.
Attachment #696865 - Flags: review?(longsonr)
Attachment #696865 - Flags: review?(bzbarsky)
Attachment #696865 - Flags: review+
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.
Attachment #696866 - Flags: review?(bzbarsky) → review+
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.
Attachment #696870 - Flags: review?(bzbarsky) → review+
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.
Attachment #696865 - Flags: review?(longsonr) → review-
The other think SVGText needs of course is your favourite class/interface DOMSVGTests/nsIDOMSVGTests! nsSVGGraphicElement gave it that previously.
(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.
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
(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?
Flags: needinfo?(cam)
(In reply to David Zbarsky (:dzbarsky) from comment #17)
> 
> These all implement SVGTests in SVG1.1 and SVG2.

My mistake, you're right there.
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.
Flags: needinfo?(cam)
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.
Attachment #696865 - Flags: review- → review+
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 :-(
You need to log in before you can comment on or make changes to this bug.