Closed Bug 816778 Opened 7 years ago Closed 7 years ago

Convert SVG classes to WebIDL bindings

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(18 files, 4 obsolete files)

24.08 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
28.11 KB, patch
bzbarsky
: review-
Details | Diff | Splinter Review
12.09 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.28 KB, text/plain
bzbarsky
: review+
Details
34.99 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.03 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
19.82 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
11.66 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
8.61 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
10.33 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.93 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
156.95 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
11.04 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
24.61 KB, patch
bzbarsky
: review-
Details | Diff | Splinter Review
21.88 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.75 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
33.41 KB, patch
heycam
: feedback+
Details | Diff | Splinter Review
7.87 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Attached patch Part 1: SVGPoint (obsolete) — Splinter Review
No description provided.
Attachment #686856 - Flags: review?(bzbarsky)
Attached patch Part 1: SVGPointSplinter Review
Try run at https://tbpl.mozilla.org/?tree=Try&rev=95bfbd417816, and I fixed the failing test.
Attachment #686856 - Attachment is obsolete: true
Attachment #686856 - Flags: review?(bzbarsky)
Attachment #686900 - Flags: review?(bzbarsky)
Attached patch Part 2: SVGMatrix (obsolete) — Splinter Review
Attachment #686902 - Flags: review?(bzbarsky)
Attachment #686902 - Attachment is patch: true
Ah, forgot to qref.  Pretend SVGMatrix's GetParentObject return mTransform
Attachment #686902 - Attachment is obsolete: true
Attachment #686902 - Flags: review?(bzbarsky)
Comment on attachment 686900 [details] [diff] [review]
Part 1: SVGPoint

>+++ b/content/svg/content/src/DOMSVGPoint.cpp

>+DOMSVGPoint::SetX(float aX, ErrorResult& rv)
>+  if (!NS_finite(aX)) {

Since the webidl type is "float", not "unrestricted float", the binding code already ensured this is a finite float. So you can just remove this check.

Same in SetY().

>+already_AddRefed<DOMSVGPoint>
>+DOMSVGPoint::MatrixTransform(nsIDOMSVGMatrix* matrix)
>   nsCOMPtr<DOMSVGMatrix> domMatrix = do_QueryInterface(matrix);
>-  if (!domMatrix)
>-    return NS_ERROR_DOM_SVG_WRONG_TYPE_ERR;

You can't do that.  Either the null-check should stay, or you need to mark nsIDOMSVGMatrix as builtinclass in the XPIDL.  I vote for the latter.

>+  nsRefPtr<DOMSVGPoint> SVGPoint = new DOMSVGPoint(pt);

How about newPoint for the variable name?

>+++ b/content/svg/content/src/DOMSVGPoint.h
>   DOMSVGPoint(const gfxPoint &aPt)

Please make this one explicit too.

>+  nsISupports* GetParentObject() {
>+    return nullptr;

Shouldn't that be mList?  Maybe it doesn't matter much since DOMSVGPointList is already on WebIDL bindings.

I just realized we can insert points from one window into a list from another window, which is totally sadmaking, but ah, well.

>diff --git a/dom/webidl/SVGPoint.webidl b/dom/webidl/SVGPoint.webidl

>+++ b/dom/webidl/SVGPoint.webidl
>+ * http://www.w3.org/TR/SVG2/

Not SVG11?

>+  SVGPoint matrixTransform(SVGMatrix matrix);

Should this be a [Creator]?  Seems to me like it should.

r=me with the above fixed
Attachment #686900 - Flags: review?(bzbarsky) → review+
Attachment #686917 - Flags: review?(bzbarsky)
Comment on attachment 686917 [details] [diff] [review]
Part 2: SVGMatrix

SetA() and company will be called from the new bindings, and possibly throw, but the exception will be ignored by the binding...  You need to declare all those as [SetterThrows] and implement accordingly.

multiply() and company should all be [Creator].

translate() should do the NS_IsFinite bits only in the XPCOM method.

Same for scaleNonUniform(), rotate(), rotateFromVector(), skewX(), skewY().  You can then remove the [Throws] for a bunch of these.

Please fix the indent of the "matrix" decl in RotateFromVector.

>+DOMSVGMatrix::SkewX(float angle, ErrorResult& rv)
>+  double ta = tan( angle*radPerDegree );
>+  if (!NS_finite(angle)) {

You want to be testing "ta" there.

>+DOMSVGMatrix::SkewY(float angle, ErrorResult& rv)

Same issue here.

>DOMSVGPoint::MatrixTransform

Need to merge this part to my review comments on the previous patch...

r- because I want to see those setters.
Attachment #686917 - Flags: review?(bzbarsky) → review-
Oh, and please do an interdiff for addressing the review comments on part2 if you can?  I'd really rather not re-review all that skew stuff.  ;)
Attachment #686943 - Flags: review?(bzbarsky)
Comment on attachment 686943 [details] [diff] [review]
Part 2, addressing review comments

r=me as far as this goes, but there should be changes to the .webidl here too, right?
Attachment #686943 - Flags: review?(bzbarsky) → review+
Attached file new SVGMatrix.webidl
Sorry, can't convince interdiff to work =(
Attachment #686946 - Flags: review?(bzbarsky)
Comment on attachment 686946 [details]
new SVGMatrix.webidl

r=me.

Just realized one more nit for part 2: Please put the '{' of the SetA/B/etc on the next line, not on the same line as the arguments.

That should be it!
Attachment #686946 - Flags: review?(bzbarsky) → review+
Attachment #686946 - Attachment is patch: false
Attachment #686993 - Flags: review?(bzbarsky)
Attachment #687240 - Flags: review?(bzbarsky)
Comment on attachment 687240 [details] [diff] [review]
Part 4: SVGAnimatedTransformList

>+++ b/content/svg/content/src/DOMSVGAnimatedTransformList.cpp
>-NS_SVG_VAL_IMPL_CYCLE_COLLECTION(DOMSVGAnimatedTransformList, mElement)

How about:

  NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(DOMSVGAnimatedTransformList, mElement)

instead of all the boilerplate?

>+already_AddRefed<DOMSVGTransformList>
>+DOMSVGAnimatedTransformList::BaseVal()

Why not have this return DOMSVGTransformList* instead?  And maybe mark it as resultNotAddRefed in teh conf file?  You'll need to NS_ADDREF in the XPCOM getter, of course.

>+already_AddRefed<DOMSVGTransformList>
>+DOMSVGAnimatedTransformList::AnimVal()

Same.

>+++ b/dom/webidl/SVGAnimatedTransformList.webidl
>+  readonly attribute SVGTransformList baseVal;
>+  readonly attribute SVGTransformList animVal;

Want to mark these as [Constant] too?

r=me with the above
Attachment #687240 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #14)

> How about:
> 
>   NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(DOMSVGAnimatedTransformList,
> mElement)
> 
> instead of all the boilerplate?

Can't do that because we don't want to unlink mElement.  See http://dxr.mozilla.org/mozilla-central/content/svg/content/src/nsSVGElement.h.html#l644

> >+already_AddRefed<DOMSVGTransformList>
> >+DOMSVGAnimatedTransformList::BaseVal()
> 
> Why not have this return DOMSVGTransformList* instead?  And maybe mark it as
> resultNotAddRefed in teh conf file?  You'll need to NS_ADDREF in the XPCOM
> getter, of course.

Ah, didn't realize we could use resultNotAddRefed if we're holding a weak ref.
Attachment #687298 - Flags: review?(bzbarsky)
> Can't do that because we don't want to unlink mElement.

Oh.  That needs a nice comment in the unlink impl so someone doesn't helpfully add it!

> Ah, didn't realize we could use resultNotAddRefed if we're holding a weak ref.

We're holding a weak ref?  Gah.  Then no, you shouldn't return a weak ref.  And add a comment saying that's why you're not doing it!
Comment on attachment 686993 [details] [diff] [review]
Part 3: SVGTransform

>+++ b/content/svg/content/src/DOMSVGTransform.cpp
>+DOMSVGMatrix*
>+DOMSVGTransform::Matrix()

mMatrixTearoff is a weak ref, so we should return an already_AddRefed here... :(

That said, could you file a followup bug on the fact that this code is totally broken if the user script sets expandos on the matrix?

r=me with that
Attachment #686993 - Flags: review?(bzbarsky) → review+
Attachment #687445 - Flags: review?(bzbarsky)
Depends on: 571734
Attachment #687480 - Flags: review?(bzbarsky)
Blocks: 817442
Attachment #687617 - Flags: review?(bzbarsky)
Comment on attachment 687480 [details] [diff] [review]
Part 7: SVGAnimatedNumberList

Is it worth adding a macro like NS_SVG_VAL_IMPL_CYCLE_COLLECTION but with wrapper caching?  NS_SVG_VAL_IMPL_CYCLE_COLLECTION_WRAPPERCACHED or something?

r=me
Attachment #687480 - Flags: review?(bzbarsky) → review+
Yep, I'll write a patch to do that.
Attachment #688069 - Flags: review?(bzbarsky)
Comment on attachment 687617 [details] [diff] [review]
Part 8: SVGAnimatedBoolean

If you're adding a new header, just export it to mozilla/dom, please.  Then you won't even need a Bindings.conf entry here.

>+using namespace dom;

  using namespace mozilla::dom;

>+NS_IMPL_CYCLE_COLLECTION_CLASS(SVGAnimatedBoolean)

Please use that new macro I asked you to add.

>+  NS_ADDREF(*aResult = new mozilla::dom::SVGAnimatedBoolean(this, aSVGElement));

You can drop the "mozilla::" bit; the file has "using namespace mozilla;" already.

r=me
Attachment #687617 - Flags: review?(bzbarsky) → review+
Sorry about the size of this, it's mostly just shuffling code around.  This patch also includes the xpidl removal because I had to fix the webidl implementation as I was removing the xpidl.  I can try to split it out if you want.
Attachment #688610 - Flags: review?(bzbarsky)
Comment on attachment 688069 [details] [diff] [review]
Part 9: Add a wrappercached svg CC macro

r=me
Attachment #688069 - Flags: review?(bzbarsky) → review+
Attachment #688872 - Flags: review?(bzbarsky)
Attached patch Part 11: SVGAngle (obsolete) — Splinter Review
Attachment #689438 - Flags: review?(bzbarsky)
Attachment #689439 - Flags: review?(bzbarsky)
Comment on attachment 688872 [details] [diff] [review]
Part 1.5: Convert another use of DOMSVGPoint

I'd rather we did nsISVGPoint that inherited nsIDOMSVGPoint and nsWrapperCache and did WrapObject bits that both these subclasses inherited from.
Attachment #688872 - Flags: review?(bzbarsky) → review-
Comment on attachment 691718 [details] [diff] [review]
Part 1.5: Create nsISVGPoint and make nsSVGTranslatePoint::DOMVal a subclass

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

::: content/svg/content/src/nsSVGSVGElement.cpp
@@ +70,5 @@
>  nsSVGTranslatePoint::ToDOMVal(nsSVGSVGElement *aElement,
>                                nsIDOMSVGPoint **aResult)
>  {
> + NS_ADDREF(*aResult = new DOMVal(this, aElement));
> + return NS_OK;

Something with the indentation here?

@@ +134,5 @@
>  nsSVGTranslatePoint::DOMVal::MatrixTransform(nsIDOMSVGMatrix *matrix,
>                                               nsIDOMSVGPoint **_retval)
>  {
> +  *_retval = MatrixTransform(matrix).get();
> +  return NS_OK;

You lost the null-check here?

::: content/svg/content/src/nsSVGSVGElement.h
@@ +77,2 @@
>  
> +    virtual nsISupports* GetParentObject();

Always bonus points for MOZ_OVERRIDE :)
(In reply to :Ms2ger from comment #33)
> Comment on attachment 691718 [details] [diff] [review]
> Part 1.5: Create nsISVGPoint and make nsSVGTranslatePoint::DOMVal a subclass

> 
> @@ +134,5 @@
> >  nsSVGTranslatePoint::DOMVal::MatrixTransform(nsIDOMSVGMatrix *matrix,
> >                                               nsIDOMSVGPoint **_retval)
> >  {
> > +  *_retval = MatrixTransform(matrix).get();
> > +  return NS_OK;
> 
> You lost the null-check here?

This code goes away in part 2, so I don't think it's worth fixing.
Attachment #689438 - Attachment is obsolete: true
Attachment #689438 - Flags: review?(bzbarsky)
Attachment #692220 - Flags: review?(bzbarsky)
Comment on attachment 687298 [details] [diff] [review]
Part 5: SVGPreserveAspectRatio

>+++ b/content/svg/content/src/SVGPreserveAspectRatio.cpp
>+NS_IMPL_CYCLE_COLLECTION_CLASS(DOMSVGPreserveAspectRatio)

Either use the new macro, or document why you're not unlinking mElement, please.

>+++ b/content/svg/content/src/SVGPreserveAspectRatio.h
>+class DOMSVGPreserveAspectRatio MOZ_FINAL : public nsIDOMSVGPreserveAspectRatio,
>+  bool mIsBaseValue;

I believe this can be a const bool.

r=me with that.
Attachment #687298 - Flags: review?(bzbarsky) → review+
Comment on attachment 687445 [details] [diff] [review]
Part 6: SVGAnimatedPreserveAspectRatio

Please use the new CC macro you added.

>+  // These aren't weak refs because mBaseVal and mAnimVal are weak

No, they aren't weak refs because it's a new object each time.  There are no mBaseVal/mAnimVal members here.

>+  [Constant]

No, they're not.  Please take that out, for both properties!

r=me with that fixed.
Attachment #687445 - Flags: review?(bzbarsky) → review+
Comment on attachment 691718 [details] [diff] [review]
Part 1.5: Create nsISVGPoint and make nsSVGTranslatePoint::DOMVal a subclass

>+++ b/content/svg/content/src/DOMSVGPointList.cpp
>+DOMSVGPointList::Initialize(nsISVGPoint& aNewItem, ErrorResult& aError)
>+  nsCOMPtr<DOMSVGPoint> domItem = do_QueryInterface(&aNewItem);
>+  MOZ_ASSERT(domItem);

Why is this OK?  Why can't aNewItem be the other kind of nsISVGPoint?  Note that since DOMSVGPoint has no IID of its own, the above operation is completely pointless, by the way.

>+DOMSVGPointList::InsertItemBefore(nsISVGPoint& aNewItem, uint32_t aIndex,

And here.

>+DOMSVGPointList::ReplaceItem(nsISVGPoint& aNewItem, uint32_t aIndex,

And here.

>+++ b/content/svg/content/src/nsISVGPoint.h
>+ * Class nsISVGPoint

The comment looks identical to the one for DOMSVGPoint.  Shouldn't it talk about what _this_ class is for?

>+++ b/content/svg/content/src/nsSVGSVGElement.cpp
>+NS_IMPL_CYCLE_COLLECTION_CLASS(nsSVGTranslatePoint::DOMVal)

Add the comment about not unlinking mElement, or use the new macro...

The rest looks fine, but I really just don't get the DOMSVGPointList code.  What's the story?
Attachment #691718 - Flags: review?(bzbarsky) → review-
Comment on attachment 689439 [details] [diff] [review]
Part 12: SVGAnimatedAngle

>+  [Constant]

It's a new object every time.  How can it be [Constant]?

Take those annotations out, and r=me.
Attachment #689439 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #38)
> Comment on attachment 691718 [details] [diff] [review]
> Part 1.5: Create nsISVGPoint and make nsSVGTranslatePoint::DOMVal a subclass
> 
> >+++ b/content/svg/content/src/DOMSVGPointList.cpp
> >+DOMSVGPointList::Initialize(nsISVGPoint& aNewItem, ErrorResult& aError)
> >+  nsCOMPtr<DOMSVGPoint> domItem = do_QueryInterface(&aNewItem);
> >+  MOZ_ASSERT(domItem);
> 
> Why is this OK?  Why can't aNewItem be the other kind of nsISVGPoint?  Note
> that since DOMSVGPoint has no IID of its own, the above operation is
> completely pointless, by the way.

DOMSVGPoint is used for points that are in a list, and these points keep references to the DOMSVGPointList.
Any points in DOMSVGPointList must be DOMSVGPoints, not just nsISVGPoints.

> 
> Add the comment about not unlinking mElement, or use the new macro...

The last few patches you reviewed came before the macro patch, which I've updated for these classes.
> Any points in DOMSVGPointList must be DOMSVGPoints, not just nsISVGPoints.

Yes, but the arguments to these functions need not be coming from inside this list, unless I'm missing something...
Attachment #692545 - Flags: review?(bzbarsky)
Comment on attachment 692545 [details] [diff] [review]
nsISVGPoint interdiff

r=me.  Thanks!
Attachment #692545 - Flags: review?(bzbarsky) → review+
Comment on attachment 692220 [details] [diff] [review]
Part 11: SVGAngle

> +  mVal->SetBaseValueInSpecifiedUnits(aValue, mType == BaseValue ? mSVGElement
> : nullptr);

nsSVGAngle::SetBaseValueInSpecifiedUnits assumes a non-null element.  Note that the created-value code didn't use to call that function.  Please fix.

>+SVGAngle::NewValueSpecifiedUnits(uint16_t unitType,
>+                                    float valueInSpecifiedUnits)

Fix indent?

>+SVGAngle::NewValueSpecifiedUnits(uint16_t unitType,
>+                                    float valueInSpecifiedUnits,
>+                                    ErrorResult& rv)

And here.

>+  rv = mVal->SetBaseValueString(aValue, mType == BaseValue ? mSVGElement :
> nullptr, true);

That last arg needs to be "false" for CreatedValue values, no?

r=me with the above issues fixed.
Attachment #692220 - Flags: review?(bzbarsky) → review+
Depends on: 821955
Attachment #688610 - Flags: review?(jwatt)
Comment on attachment 688610 [details] [diff] [review]
Part 10: SVGPathSeg and subclasses

>+++ b/content/svg/content/src/DOMSVGPathSeg.cpp
>-nsIDOMSVGPathSeg*
>+nsISupports*
> NS_NewSVGPathSegClosePath()

Should this and the other NS_New* things return DOMSVGPathSeg*, perhaps?

>+++ b/dom/bindings/Bindings.conf
>+'SVGPathSeg': {
>+    'nativeType': 'mozilla::DOMSVGPathSeg',
>+    'headerFile': 'DOMSVGPathSeg.h'

Should this be "'concrete':False"?  At first glance (given the pure virtual WrapObject), yes.

>+'SVGPathSegClosePath': {

I wonder whether it's worth doing some sort of function for adding all these identical guys, similar to addExternalIface or whatnot...

>+++ b/dom/interfaces/svg/nsIDOMSVGPathElement.idl

In the usual way, please comment what the real types involved are?

r=me
Attachment #688610 - Flags: review?(bzbarsky) → review+
Attached patch SVG IDL testsSplinter Review
Should we upstream this and import it?
Attachment #693230 - Flags: feedback?(Ms2ger)
Comment on attachment 693230 [details] [diff] [review]
SVG IDL tests

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

Thanks for the test, but I don't think it should live in dom/imptests unless it's actually imported from somewhere. Cameron, does SVG have some place for testharness.js tests already?
Attachment #693230 - Flags: feedback?(Ms2ger) → feedback?(cam)
Attachment #688610 - Flags: review?(jwatt)
Depends on: 823394
Attachment #694701 - Flags: review?(bzbarsky)
Comment on attachment 694701 [details] [diff] [review]
Part 12: SVGAnimatedLengthList

r=me
Attachment #694701 - Flags: review?(bzbarsky) → review+
Comment on attachment 693230 [details] [diff] [review]
SVG IDL tests

So we definitely want to be able to add testharness.js tests for our SVG 2 test suite, but we don't have any yet.  I will get our repo ready at some point for such tests and migrate this into dom/imptests/ if that's the easiest way to import it.
Comment on attachment 693230 [details] [diff] [review]
SVG IDL tests

I filed bug 824250 to migrate tests to the SVG WG's repo and import them, once we're able to.
Attachment #693230 - Flags: feedback?(cam) → feedback+
I'll do the rest in another bug
Whiteboard: [leave open]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.