Closed Bug 817442 Opened 7 years ago Closed 7 years ago

Remove XPIDL from SVG classes

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

Attachments

(11 files, 5 obsolete files)

16.50 KB, patch
bzbarsky
: review+
jwatt
: review+
Details | Diff | Splinter Review
40.54 KB, patch
bzbarsky
: review+
jwatt
: review+
Details | Diff | Splinter Review
12.95 KB, patch
bzbarsky
: review+
jwatt
: review+
Details | Diff | Splinter Review
19.31 KB, patch
bzbarsky
: review+
jwatt
: review+
Details | Diff | Splinter Review
34.36 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
34.64 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
32.51 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
32.24 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
31.24 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
45.63 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
27.64 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
No description provided.
Attachment #687569 - Flags: review?(bzbarsky)
Depends on: 816778
Attachment #687576 - Flags: review?(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?
Attachment #687569 - Flags: superreview?(jwatt)
Attachment #687569 - Flags: review?(bzbarsky) → review+
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.
Attachment #687576 - Flags: superreview?(jwatt)
Attachment #687576 - Flags: review?(bzbarsky)
Attachment #687576 - Flags: review+
Attachment #689433 - Flags: superreview?(jwatt)
Attachment #689433 - Flags: review?(bzbarsky)
Attachment #689435 - Flags: superreview?(jwatt)
Attachment #689435 - Flags: review?(bzbarsky)
Attached patch nsIDOMSVG(Animated)Angle (obsolete) — Splinter Review
Attachment #689437 - Flags: superreview?(jwatt)
Attachment #689437 - Flags: review?(bzbarsky)
Comment on attachment 689433 [details] [diff] [review]
nsIDOMSVGAnimatedBoolean

r=me.  I note you failed at your "no more patches" promise.  ;)
Attachment #689433 - Flags: review?(bzbarsky) → review+
Comment on attachment 689435 [details] [diff] [review]
nsIDOMSVGPathSegList

r=me
Attachment #689435 - Flags: review?(bzbarsky) → review+
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.
Attachment #689437 - Flags: review?(bzbarsky) → review-
Attached patch nsIDOMSVG(Animated)Angle (obsolete) — Splinter Review
Attachment #689437 - Attachment is obsolete: true
Attachment #689437 - Flags: superreview?(jwatt)
Attachment #690001 - Flags: superreview?(jwatt)
Attachment #690001 - Flags: review?(bzbarsky)
Attachment #690001 - Attachment is obsolete: true
Attachment #690001 - Flags: superreview?(jwatt)
Attachment #690001 - Flags: review?(bzbarsky)
Attachment #690003 - Flags: review?(bzbarsky)
I fixed the comment locally.
Comment on attachment 690003 [details] [diff] [review]
nsIDOMSVG(Animated)Angle interdiff from last patch

r=me
Attachment #690003 - Flags: review?(bzbarsky) → review+
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 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.
Attachment #687569 - Flags: superreview?(jwatt) → superreview+
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.
Attachment #687576 - Flags: superreview?(jwatt) → superreview+
Attachment #689433 - Flags: superreview?(jwatt) → superreview+
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.
Attachment #689435 - Flags: superreview?(jwatt) → superreview+
And of course please address Robert's comment 15.
Comment on attachment 687569 [details] [diff] [review]
Part 1: nsIDOMSVGPointList

Changing my sr+ to ar+ since I'm not a superreviewer.
Attachment #687569 - Flags: superreview+ → review+
Attachment #687576 - Flags: superreview+ → review+
Attachment #689433 - Flags: superreview+ → review+
Attachment #689435 - Flags: superreview+ → review+
Attachment #692193 - Flags: review?(bzbarsky)
Attachment #692194 - Flags: review?(bzbarsky)
Attached patch nsIDOMSVGMatrixSplinter Review
Attachment #692201 - Flags: review?(bzbarsky)
Attachment #692193 - Flags: review?(jwatt)
Attachment #692194 - Flags: review?(jwatt)
Attachment #692201 - Flags: review?(jwatt)
Attached patch nsIDOMSVG(Animated)Angle, r=bz (obsolete) — Splinter Review
Attachment #690003 - Attachment is obsolete: true
Attachment #692506 - Flags: review?(jwatt)
Attachment #692506 - Attachment is obsolete: true
Attachment #692506 - Flags: review?(jwatt)
Attachment #692517 - Flags: review?(bzbarsky)
Attachment #692517 - Flags: review?(bzbarsky)
Attachment #692518 - Flags: review?(jwatt)
Attachment #692517 - Flags: review?(bzbarsky) → review?(jwatt)
Attachment #692518 - Attachment is patch: true
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.
Attachment #692193 - Flags: review?(bzbarsky) → review+
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
Attachment #692194 - Flags: review?(bzbarsky) → review+
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
Attachment #692201 - Flags: review?(bzbarsky) → review+
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
Attachment #692517 - Flags: review?(bzbarsky) → review+
Attached patch nsIDOMSVGPoint (obsolete) — Splinter Review
Sorry, I forgot to post this before.
Attachment #692553 - Flags: review?(bzbarsky)
Attached patch nsIDOMSVGPointSplinter Review
Attachment #692553 - Attachment is obsolete: true
Attachment #692553 - Flags: review?(bzbarsky)
Attachment #692555 - Flags: review?(bzbarsky)
Attachment #692555 - Flags: review?(jwatt)
Comment on attachment 692555 [details] [diff] [review]
nsIDOMSVGPoint

r=me
Attachment #692555 - Flags: review?(bzbarsky) → review+
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?
Attachment #692193 - Flags: review?(jwatt)
Attachment #692194 - Flags: review?(jwatt)
Attachment #692518 - Flags: review?(jwatt) → review+
Attachment #692517 - Flags: review?(jwatt)
Attachment #692201 - Flags: review?(jwatt)
Attachment #692555 - Flags: review?(jwatt)
Flags: needinfo?(longsonr)
Attachment #694702 - Flags: review?(bzbarsky)
Comment on attachment 694702 [details] [diff] [review]
nsIDOMSVG(Animated)LengthList

r=me
Attachment #694702 - Flags: review?(bzbarsky) → review+
No, we've completely moved away from using heavyweight DOM objects and I don't think we'll be going back.
Flags: needinfo?(longsonr)
I'll do the rest in another bug.
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.