866796 - Convert SVGAnimatedRect to WebIDL

Status: RESOLVED FIXED

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[David Zbarsky (:dzbarsky)]

Comment on attachment 743161 [details] [diff] [review]
patch

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

::: content/svg/content/src/SVGAnimatedRect.h
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_svganimatedrect_h
> +#define mozilla_dom_svganimatedrect_h
> +

mozilla_dom_SVGAnimatedRect_h

@@ +38,5 @@
> +  virtual JSObject* WrapObject(JSContext* aCx,
> +                               JS::Handle<JSObject*> aScope) MOZ_OVERRIDE
> +  {
> +    return mozilla::dom::SVGAnimatedRectBinding::Wrap(aCx, aScope, this);
> +  }

drop the mozilla::dom?

::: content/svg/content/src/SVGMarkerElement.cpp
@@ -113,4 @@
>    mViewBox.ToDOMAnimatedRect(getter_AddRefs(rect), this);
>    return rect.forget();
>  }
>  

It would be nice to outparamdel ToDOMAnimatedRect

::: dom/webidl/WebIDL.mk
@@ +206,1 @@
>    SVGAnimatedTransformList.webidl \

this file isn't in the patch ;)

Also, does this actually compile?  I would have thought that you need to make SVGRect wrappercached.

Comment 3

[Andrea Marchesini [:baku]]

> Also, does this actually compile?  I would have thought that you need to
> make SVGRect wrappercached.

Sure. and all the SVG mochitests are green on my laptop. Still it's not tested on try.
New patch coming with all the comments.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch

About the outparams, I prefer to do that in a follow up.

Comment 5

[David Zbarsky (:dzbarsky)]

(In reply to Andrea Marchesini (:baku) from comment #3)
> > Also, does this actually compile?  I would have thought that you need to
> > make SVGRect wrappercached.
> 
> Sure. and all the SVG mochitests are green on my laptop. Still it's not
> tested on try.
> New patch coming with all the comments.

Hmm, I could have sworn that the WebIDL parser only allowed returning non-wrappercached things from creators...

Comment 6

[David Zbarsky (:dzbarsky)]

(In reply to David Zbarsky (:dzbarsky) from comment #5)
> (In reply to Andrea Marchesini (:baku) from comment #3)
> > > Also, does this actually compile?  I would have thought that you need to
> > > make SVGRect wrappercached.
> > 
> > Sure. and all the SVG mochitests are green on my laptop. Still it's not
> > tested on try.
> > New patch coming with all the comments.
> 
> Hmm, I could have sworn that the WebIDL parser only allowed returning
> non-wrappercached things from creators...

Ah, you marked baseVal and animVal as [Creator].  They shouldn't be, because you grab the objects from the hashtable.

Comment 7

[Andrea Marchesini [:baku]]

> Ah, you marked baseVal and animVal as [Creator].  They shouldn't be, because
> you grab the objects from the hashtable.

Unfortunately without Creator, the python generator fails. SVGRect is not wrapperCache instance so the Creator seems a need.

Comment 8

[Boris Zbarsky [:bzbarsky, bz on IRC]]

You have it backwards.

[Creator] is only allowed on things that always return a new object.  If your thing doesn't always return a new object, it can't be a [Creator], period.  How can I make https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Creator clearer about this?

If your object is returned from things that are not [Creator] then it needs to be wrappercached.

If SVGRect is being thus returned, it needs to start inheriting from nsWrapperCache.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 743179 [details] [diff] [review]
patch

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

r- for the Creator thing.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch

I don't know if you like the setParentObject()..

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

nsSVGGlyphFrame has an mContent you can pass, no?

Comment 12

[Andrea Marchesini [:baku]]

Attachment: patch

Yes, I can use mContent!

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 744072 [details] [diff] [review]
patch

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

::: content/svg/content/src/SVGIRect.h
@@ +36,5 @@
>    {
>      return SVGRectBinding::Wrap(aCx, aScope, this);
>    }
>  
> +  virtual nsSVGElement* GetParentObject() const = 0;

I'd prefer that you pass the parent to the SVGIRect constructor.

::: content/svg/content/src/SVGTextContentElement.cpp
@@ +188,5 @@
> +  // Any SVGRect has to have a parent, but currently Frame elements
> +  // are not nsWrapperCache classes.
> +  SVGRect* tmp = static_cast<SVGRect*>(rect.get());
> +  if (tmp && !tmp->GetParentObject()) {
> +    tmp->SetParentObject(this);

This is still here?

::: content/svg/content/src/moz.build
@@ +24,5 @@
>      'SVGAnimatedLength.h',
>      'SVGAnimatedTransformList.h',
>      'SVGAnimateElement.h',
>      'SVGAnimateMotionElement.h',
> +    'SVGAnimatedRect.h',

This isn't sorted right

::: layout/svg/nsSVGGlyphFrame.cpp
@@ +1285,5 @@
>                              metrics.mAscent + metrics.mDescent));
>    tmpCtx->IdentityMatrix();
>  
> +  nsRefPtr<dom::SVGRect> rect =
> +    NS_NewSVGRect(nullptr, tmpCtx->GetUserPathExtent());

mContent

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch

Sorry, my fault! I forgot to do qref :/ This was the old patch.

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 744598 [details] [diff] [review]
patch

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

Mostly good, but I'd like to see one more version.

::: content/svg/content/src/SVGAnimatedRect.h
@@ +23,5 @@
> +                                  public nsWrapperCache
> +{
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_CLASS(SVGAnimatedRect)

_SCRIPT_HOLDER

@@ +25,5 @@
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_CLASS(SVGAnimatedRect)
> +
> +  SVGAnimatedRect(nsSVGViewBox *aVal, nsSVGElement *aSVGElement);

* to the left

@@ +37,5 @@
> +
> +  virtual JSObject* WrapObject(JSContext* aCx,
> +                               JS::Handle<JSObject*> aScope) MOZ_OVERRIDE
> +  {
> +    return SVGAnimatedRectBinding::Wrap(aCx, aScope, this);

Move this out of line

::: content/svg/content/src/SVGIRect.h
@@ +25,3 @@
>  {
>  public:
> +  SVGIRect(nsIContent* aParent) : mParent(aParent)

Make this

  SVGIRect(nsIContent* aParent)
    : mParent(aParent)

::: content/svg/content/src/SVGMarkerElement.cpp
@@ +113,1 @@
>    mViewBox.ToDOMAnimatedRect(getter_AddRefs(rect), this);

Followup to fix the signature

::: content/svg/content/src/SVGRect.cpp
@@ +24,2 @@
>  SVGRect::SVGRect(float x, float y, float w, float h)
> +  : SVGIRect(nullptr), mX(x), mY(y), mWidth(w), mHeight(h)

In which cases is this called? I'd rather not have it if it isn't necessary...

::: content/svg/content/src/moz.build
@@ +24,5 @@
>      'SVGAnimatedLength.h',
>      'SVGAnimatedTransformList.h',
>      'SVGAnimateElement.h',
>      'SVGAnimateMotionElement.h',
> +    'SVGAnimatedRect.h',

This is in the wrong place

::: content/svg/content/src/nsSVGViewBox.cpp
@@ +354,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(SVGAnimatedRect)

Give this its own .cpp.

@@ +355,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(SVGAnimatedRect)
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)

Missing wrappercache entry

@@ +358,5 @@
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(SVGAnimatedRect)
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END
> +
> +NS_SVG_VAL_IMPL_CYCLE_COLLECTION(SVGAnimatedRect, mSVGElement)

_wrappercache, IIRC

@@ +363,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(SVGAnimatedRect)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(SVGAnimatedRect)
> +
> +SVGAnimatedRect::SVGAnimatedRect(nsSVGViewBox *aVal, nsSVGElement *aSVGElement)

* to the left

@@ +364,5 @@
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(SVGAnimatedRect)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(SVGAnimatedRect)
> +
> +SVGAnimatedRect::SVGAnimatedRect(nsSVGViewBox *aVal, nsSVGElement *aSVGElement)
> +  : mVal(aVal), mSVGElement(aSVGElement)

One line for each

::: content/svg/content/src/nsSVGViewBox.h
@@ +106,5 @@
>      NS_DECL_CYCLE_COLLECTION_CLASS(DOMBaseVal)
>  
>      DOMBaseVal(nsSVGViewBox *aVal, nsSVGElement *aSVGElement)
> +      : mozilla::dom::SVGIRect(aSVGElement)
> +      , mVal(aVal) , mSVGElement(aSVGElement) {}

One line each, and {} on a line of its own as well

@@ +150,5 @@
>      NS_DECL_CYCLE_COLLECTION_CLASS(DOMAnimVal)
>  
>      DOMAnimVal(nsSVGViewBox *aVal, nsSVGElement *aSVGElement)
> +      : mozilla::dom::SVGIRect(aSVGElement)
> +      , mVal(aVal), mSVGElement(aSVGElement) {}

Ditto

Comment 16

[Andrea Marchesini [:baku]]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=3f6f5005b479

Comment 17

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 745782 [details] [diff] [review]
patch

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

lgtm

::: content/svg/content/src/SVGTextContentElement.cpp
@@ +188,1 @@
>    return rect.forget();

Do you need the changes in this file? If not, drop them.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78a43b5ffabd

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/78a43b5ffabd