817442 - Remove XPIDL from SVG classes

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Part 1: nsIDOMSVGPointList

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: nsIDOMSVG(Animated)PreserveAspectRatio

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 687569 [details] [diff] [review]
Part 1: nsIDOMSVGPointList

Looks fine to me, but please get an SVG peer to ok this too?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 687576 [details] [diff] [review]
nsIDOMSVG(Animated)PreserveAspectRatio

Should the constants be static?

r=me with that, I think.  Again, good to have an SVG peer look this over.

Comment 5

[David Zbarsky (:dzbarsky)]

Attachment: nsIDOMSVGAnimatedBoolean

Comment 6

[David Zbarsky (:dzbarsky)]

Attachment: nsIDOMSVGPathSegList

Comment 7

[David Zbarsky (:dzbarsky)]

Attachment: nsIDOMSVG(Animated)Angle

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 689433 [details] [diff] [review]
nsIDOMSVGAnimatedBoolean

r=me.  I note you failed at your "no more patches" promise.  ;)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 689435 [details] [diff] [review]
nsIDOMSVGPathSegList

r=me

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 689437 [details] [diff] [review]
nsIDOMSVG(Animated)Angle

>+++ b/content/svg/content/src/SVGAngle.cpp
>+SVGAngle::GetValueAsString(nsAString& aValue)
>+{
>+  mVal->GetBaseValueString(aValue);

Looks wrong to me.  Should't this use GetAnimValueString if mType == AnimValue?

>+++ b/content/svg/content/src/nsSVGMarkerElement.cpp
>+  dom::SVGAngle* angle = static_cast<dom::SVGAngle*>(aAngle);

That's totally not safe, because any JS object can be passed to a setter or method taking nsISupports.  You need to actually check what sort of thing got passed in.

r- to get those sorted out.

Comment 11

[David Zbarsky (:dzbarsky)]

Attachment: nsIDOMSVG(Animated)Angle

Comment 12

[David Zbarsky (:dzbarsky)]

Attachment: nsIDOMSVG(Animated)Angle interdiff from last patch

Comment 13

[David Zbarsky (:dzbarsky)]

I fixed the comment locally.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 690003 [details] [diff] [review]
nsIDOMSVG(Animated)Angle interdiff from last patch

r=me

Comment 15

[Robert Longson [:longsonr]]

Comment on attachment 690003 [details] [diff] [review]
nsIDOMSVG(Animated)Angle interdiff from last patch

> 
>+// We make DOMSVGTransform a pseudo-interface to allow us to QI to it in order
>+// to check that the objects that scripts pass in are our our *native*
>+// transform objects.

DOMSVGTransform, I don't think so. Too much copy and paste here :-)

Comment 16

[Jonathan Watt [:jwatt]]

Comment on attachment 687569 [details] [diff] [review]
Part 1: nsIDOMSVGPointList

Review of attachment 687569 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/nsSVGPolyElement.cpp
@@ +33,5 @@
>  
>  //----------------------------------------------------------------------
>  // nsIDOMSGAnimatedPoints methods:
>  
> +/* readonly attribute nsISupports points; */

I don't think this change is appropriate since this comment is supposed to indicate the interface that's implemented by the object that is returned via the outparam.

@@ +42,5 @@
>    *aPoints = DOMSVGPointList::GetDOMWrapper(key, this, false).get();
>    return NS_OK;
>  }
>  
> +/* readonly attribute nsISupports animatedPoints; */

Same here.

Comment 17

[Jonathan Watt [:jwatt]]

Comment on attachment 687576 [details] [diff] [review]
nsIDOMSVG(Animated)PreserveAspectRatio

Review of attachment 687576 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/SVGAnimatedPreserveAspectRatio.cpp
@@ +77,5 @@
>      "Unknown align");
>  
>    aAlignString.AssignASCII(
>      sAlignStrings[aAlign -
> +                  SVG_PRESERVEASPECTRATIO_NONE]);

Move this up onto the line above now.

@@ +102,5 @@
>      "Unknown meetOrSlice");
>  
>    aMeetOrSliceString.AssignASCII(
>      sMeetOrSliceStrings[aMeetOrSlice -
> +                        SVG_MEETORSLICE_MEET]);

Move up.

@@ +216,5 @@
>    GetAlignString(tmpString, mBaseVal.mAlign);
>    aValueAsString.Append(tmpString);
>  
>    if (mBaseVal.mAlign !=
> +      SVG_PRESERVEASPECTRATIO_NONE) {

Move up.

::: content/svg/content/src/SVGContentUtils.cpp
@@ +324,2 @@
>          a < d) ||
> +        (meetOrSlice == SVG_MEETORSLICE_SLICE &&

Could tidy this up too.

@@ +351,1 @@
>        a < d)) {

And here.

::: content/svg/content/src/nsSVGImageElement.cpp
@@ +96,5 @@
>  {
>    return mLengthAttributes[HEIGHT].ToDOMAnimatedLength(aHeight, this);
>  }
>  
> +/* readonly attribute nsISupports preserveAspectRatio; */

I don't think you should make this comment change to nsISupports. Mention the WebIDL interface name instead.

::: content/svg/content/src/nsSVGMarkerElement.cpp
@@ +121,5 @@
>  {
>    return mViewBox.ToDOMAnimatedRect(aViewBox, this);
>  }
>  
> +/* readonly attribute nsISupports preserveAspectRatio; */

Same.

::: content/svg/content/src/nsSVGPatternElement.cpp
@@ +79,5 @@
>  {
>    return mViewBox.ToDOMAnimatedRect(aViewBox, this);
>  }
>  
> +/* readonly attribute nsISupports preserveAspectRatio; */

Same.

::: content/svg/content/src/nsSVGSVGElement.cpp
@@ +624,5 @@
>  {
>    return mViewBox.ToDOMAnimatedRect(aViewBox, this);
>  }
>  
> +/* readonly attribute nsISupports preserveAspectRatio; */

Same.

@@ +1145,5 @@
>        ShouldSynthesizeViewBox()) {
>      // If we're synthesizing a viewBox, use preserveAspectRatio="none";
>      return SVGPreserveAspectRatio(
> +         SVG_PRESERVEASPECTRATIO_NONE,
> +         SVG_MEETORSLICE_SLICE);

Can be on one line now.

::: content/svg/content/src/nsSVGSymbolElement.cpp
@@ +93,5 @@
>  {
>    return mViewBox.ToDOMAnimatedRect(aViewBox, this);
>  }
>  
> +/* readonly attribute nsISupports preserveAspectRatio; */

I don't think you should make this comment change.

::: content/svg/content/src/nsSVGViewElement.cpp
@@ +90,5 @@
>  {
>    return mViewBox.ToDOMAnimatedRect(aViewBox, this);
>  }
>  
> +/* readonly attribute nsISupports preserveAspectRatio; */

Same.

Comment 18

[Jonathan Watt [:jwatt]]

Comment on attachment 689435 [details] [diff] [review]
nsIDOMSVGPathSegList

Review of attachment 689435 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/nsSVGPathElement.cpp
@@ +317,5 @@
>  
>  //----------------------------------------------------------------------
>  // nsIDOMSVGAnimatedPathData methods:
>  
> +/* readonly attribute nsISupports pathSegList; */

Again, not liking the comment change to nsISupports.

@@ +325,5 @@
>    *aPathSegList = DOMSVGPathSegList::GetDOMWrapper(key, this, false).get();
>    return *aPathSegList ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
>  }
>  
> +/* readonly attribute nsISupports normalizedPathSegList; */

Here too.

@@ +331,5 @@
>  {
>    return NS_ERROR_NOT_IMPLEMENTED;
>  }
>  
> +/* readonly attribute nsISupports animatedPathSegList; */

Here too.

@@ +340,5 @@
>      DOMSVGPathSegList::GetDOMWrapper(key, this, true).get();
>    return *aAnimatedPathSegList ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
>  }
>  
> +/* readonly attribute nsISupports animatedNormalizedPathSegList; */

Here too.

Comment 19

[Jonathan Watt [:jwatt]]

And of course please address Robert's comment 15.

Comment 20

[Jonathan Watt [:jwatt]]

Comment on attachment 687569 [details] [diff] [review]
Part 1: nsIDOMSVGPointList

Changing my sr+ to ar+ since I'm not a superreviewer.

Comment 21

[David Zbarsky (:dzbarsky)]

Attachment: nsIDOMSVG(Animated)TransformList

Comment 22

[David Zbarsky (:dzbarsky)]

Attachment: nsIDOMSVGTransform

Comment 23

[David Zbarsky (:dzbarsky)]

Attachment: nsIDOMSVGMatrix

Comment 24

[David Zbarsky (:dzbarsky)]

Attachment: nsIDOMSVG(Animated)Angle, r=bz

Comment 25

[David Zbarsky (:dzbarsky)]

Attachment: nsIDOMSVG(Animated)NumberList

Comment 26

[David Zbarsky (:dzbarsky)]

Attachment: nsIDOMSVG(Animated)Angle, r=bz

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 692193 [details] [diff] [review]
nsIDOMSVG(Animated)TransformList

>+++ b/dom/interfaces/svg/nsIDOMSVGGradientElement.idl
>+  readonly attribute nsISupports                  gradientTransform;

Maybe comment that this is a SVGAnimatedTransformList, for when we're converting to WebIDL?

>+++ b/dom/interfaces/svg/nsIDOMSVGPatternElement.idl
>+  readonly attribute nsISupports                    patternTransform;

Likewise.

>+++ b/dom/interfaces/svg/nsIDOMSVGTransformable.idl
>+  readonly attribute nsISupports transform;

And here.

>+++ b/dom/interfaces/svg/nsIDOMSVGViewSpec.idl
>+  readonly attribute nsISupports            transform;

And this one an SVGTransformList

r=me with that.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 692194 [details] [diff] [review]
nsIDOMSVGTransform

>+++ b/dom/interfaces/svg/nsIDOMSVGSVGElement.idl
>+  nsISupports           createSVGTransform();
>+  nsISupports           createSVGTransformFromMatrix(in nsIDOMSVGMatrix matrix);

Please comment what the real type of the return value is, and r=me

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 692201 [details] [diff] [review]
nsIDOMSVGMatrix

>+++ b/dom/interfaces/svg/nsIDOMSVGLocatable.idl
>+  nsISupports     getCTM();
>+  nsISupports     getScreenCTM();
>+  nsISupports     getTransformToElement(in nsIDOMSVGElement element);

The same comment-the-type drill.

>+++ b/dom/interfaces/svg/nsIDOMSVGPoint.idl
>+  nsIDOMSVGPoint matrixTransform(in nsISupports matrix);

And here.

>+++ b/dom/interfaces/svg/nsIDOMSVGSVGElement.idl
>+  nsISupports           createSVGMatrix();
>+  nsISupports           createSVGTransformFromMatrix(in nsISupports matrix);

And here.

r=me

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 692517 [details] [diff] [review]
nsIDOMSVG(Animated)NumberList

>+++ b/dom/interfaces/svg/nsIDOMSVGFilters.idl
>+    readonly attribute nsISupports  values;

You know the drill.

>+    readonly attribute nsISupports                  tableValues;

And here.

>+    readonly attribute nsISupports                  kernelMatrix;

And here.

>+++ b/dom/interfaces/svg/nsIDOMSVGTextPositionElem.idl
>+  readonly attribute nsISupports rotate;

And here.

r=me

Comment 31

[David Zbarsky (:dzbarsky)]

Attachment: nsIDOMSVGPoint

Sorry, I forgot to post this before.

Comment 32

[David Zbarsky (:dzbarsky)]

Attachment: nsIDOMSVGPoint

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 692555 [details] [diff] [review]
nsIDOMSVGPoint

r=me

Comment 34

[Jonathan Watt [:jwatt]]

Regarding the general concept of removing the XPIDL code, dzbarsky tells me the most significant issue is that it makes it harder to use the classes from C++. I'm fine with that, since we don't really want to be using the heavyweight DOM objects internally.

longsonr, do you foresee any problems?

Comment 35

[David Zbarsky (:dzbarsky)]

Attachment: nsIDOMSVG(Animated)LengthList

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 694702 [details] [diff] [review]
nsIDOMSVG(Animated)LengthList

r=me

Comment 37

[Robert Longson [:longsonr]]

No, we've completely moved away from using heavyweight DOM objects and I don't think we'll be going back.

Comment 38

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc1c750ee0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4e40d2db9ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b915ddf9d18
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b53a620ef9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/f76cba206706
https://hg.mozilla.org/integration/mozilla-inbound/rev/822291ecc695
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef22f0b8e466
https://hg.mozilla.org/integration/mozilla-inbound/rev/29225925d6e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/6516c71accd0
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d9587921eb9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8734f4f5d20

Comment 39

[David Zbarsky (:dzbarsky)]

I'll do the rest in another bug.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7fc1c750ee0c
https://hg.mozilla.org/mozilla-central/rev/b4e40d2db9ad
https://hg.mozilla.org/mozilla-central/rev/9b915ddf9d18
https://hg.mozilla.org/mozilla-central/rev/8b53a620ef9e
https://hg.mozilla.org/mozilla-central/rev/f76cba206706
https://hg.mozilla.org/mozilla-central/rev/822291ecc695
https://hg.mozilla.org/mozilla-central/rev/ef22f0b8e466
https://hg.mozilla.org/mozilla-central/rev/29225925d6e4
https://hg.mozilla.org/mozilla-central/rev/6516c71accd0
https://hg.mozilla.org/mozilla-central/rev/3d9587921eb9
https://hg.mozilla.org/mozilla-central/rev/a8734f4f5d20