852620 - Convert SVGRect to WebIDL

Status: RESOLVED FIXED

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

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 3

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

Comment on attachment 726834 [details] [diff] [review]
part 1 - renaming

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

::: content/svg/content/src/Makefile.in
@@ +50,5 @@
>  		nsSVGPathDataParser.cpp \
>  		nsSVGPathGeometryElement.cpp \
>  		nsSVGPolyElement.cpp \
>  		nsSVGString.cpp \
> +		SVGRect.cpp \

Put this below, in the sorted section of SVG*.cpp

Comment 4

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

Comment on attachment 726835 [details] [diff] [review]
part 2 - webidl

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

::: content/svg/content/src/SVGRect.h
@@ +40,5 @@
> +
> +  void SetX(float aX, ErrorResult& aRv)
> +  {
> +    if (!NS_finite(aX)) {
> +      aRv.Throw(NS_ERROR_ILLEGAL_VALUE);

These aren't unrestricted floats, so this can never happen.

Comment 5

[David Zbarsky (:dzbarsky)]

You haven't converted nsSVGViewBox::DOMBaseVal and nsSVGViewBox::DOMAnimVal.  You're probably going to want to make a base class, like nsISVGPoint.

Comment 6

[David Zbarsky (:dzbarsky)]

Comment on attachment 726835 [details] [diff] [review]
part 2 - webidl

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

::: content/svg/content/src/SVGRect.h
@@ +50,5 @@
> +
> +  float Y() const
> +  {
> +    return mY;
> +  }

Just inline these?

::: content/svg/content/src/SVGSVGElement.cpp
@@ +416,4 @@
>    NS_NewSVGRect(getter_AddRefs(rect));
>    return rect.forget();
>  }
>  

It may be worth changing the signature of NS_NewSVGRect to not use the outparam.

Comment 7

[Andrea Marchesini [:baku]]

This check was in the XPCOM implementation. I prefer to keep it.

> > +  void SetX(float aX, ErrorResult& aRv)
> > +  {
> > +    if (!NS_finite(aX)) {
> > +      aRv.Throw(NS_ERROR_ILLEGAL_VALUE);
> 
> These aren't unrestricted floats, so this can never happen.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

I didn't use a base class, but can you review this patch?

Comment 9

[David Zbarsky (:dzbarsky)]

Comment on attachment 726950 [details] [diff] [review]
part 2 - webidl

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

File a followup for removing nsIDOMSVGRect and classinof?

::: content/svg/content/src/SVGRect.h
@@ +41,5 @@
> +    if (!NS_finite(aX)) {
> +      aRv.Throw(NS_ERROR_ILLEGAL_VALUE);
> +      return;
> +    }
> +

Don't need the Ns_finite check.

@@ +88,3 @@
>  protected:
>    float mX, mY, mWidth, mHeight;
>  };

So subclasses have access to mX, mY, mWidth, and mHeight even though they don't use it?  That seems a little unfortunate, which is why I suggested the base class.

::: content/svg/content/src/nsSVGViewBox.h
@@ +105,4 @@
>      NS_IMETHOD GetX(float *aX)
> +      { *aX = X(); return NS_OK; }
> +    float Y() const
> +      { return mVal->GetBaseValue().y; }

Please mark all of these as MOZ_OVERRIDE, just in case the signature changes.

Comment 10

[David Zbarsky (:dzbarsky)]

(In reply to Andrea Marchesini (:baku) from comment #7)
> This check was in the XPCOM implementation. I prefer to keep it.
> 
> > > +  void SetX(float aX, ErrorResult& aRv)
> > > +  {
> > > +    if (!NS_finite(aX)) {
> > > +      aRv.Throw(NS_ERROR_ILLEGAL_VALUE);
> > 
> > These aren't unrestricted floats, so this can never happen.

Move them to the XPCOM method.  the generated bindings code already performs these checks.

Comment 11

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

Comment on attachment 726950 [details] [diff] [review]
part 2 - webidl

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

::: content/svg/content/src/SVGRect.h
@@ +33,5 @@
> +  // WebIDL
> +  virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope);
> +
> +  virtual float X() const
> +    { return mX; }

Fix the style here, should be

{
  return mX;
}

Same for the others.

::: content/svg/content/src/nsSVGViewBox.h
@@ +148,4 @@
>      }
> +
> +    NS_IMETHOD GetX(float *aX)
> +      { *aX = X(); return NS_OK; }

You should remove all the XPCOM implementations here, and declare them as MOZ_OVERRIDE MOZ_FINAL in dom::SVGRect.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

SVGIRect base class

Comment 13

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

Comment on attachment 727161 [details] [diff] [review]
part 2 - webidl

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

::: content/svg/content/src/SVGRect.cpp
@@ +38,1 @@
>  NS_IMETHODIMP SVGRect::SetX(float aX)

These setters need to die too.

::: content/svg/content/src/SVGRect.h
@@ +18,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +class SVGIRect : public nsIDOMSVGRect

Call it mozilla::dom::ISVGRect, then. Also move it into its own file.

@@ +30,5 @@
> +    *aX = X();
> +    return NS_OK;
> +  }
> +
> +  NS_IMETHOD SetX(float aX)

This should test for finiteness, and they should all be MOZ_OVERRIDE MOZ_FINAL.

@@ +106,5 @@
> +  {
> +    return mX;
> +  }
> +
> +  virtual void SetX(float aX, ErrorResult& aRv) MOZ_OVERRIDE

All of these should assert finiteness.

@@ +143,5 @@
> +  }
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope)
> +  {
> +    return SVGRectBinding::Wrap(aCx, aScope, this);

Move this out of line, as usual.

::: dom/bindings/Bindings.conf
@@ +833,5 @@
>      'headerFile': 'mozilla/dom/SVGGradientElement.h',
>  },
>  
> +'SVGRect': {
> +    'wrapperCache': False

This should now have a nativeType that gives the name of the subclass.

Comment 14

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

And it's probably a good idea to create the new superclass in a separate patch.

Comment 15

[Andrea Marchesini [:baku]]

ok. So let's land the previous patch + comments.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Another quick review?

Comment 17

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

Comment on attachment 727609 [details] [diff] [review]
part 2 - webidl

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

::: content/svg/content/src/SVGRect.cpp
@@ +40,1 @@
>  NS_IMETHODIMP SVGRect::SetX(float aX)

These should be gone

Comment 18

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 19

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

SVGIRect was missing in the previous patch.

Comment 20

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

Comment on attachment 728183 [details] [diff] [review]
part 2 - webidl

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

One issue, still. The bindings should use SVGIRect, not SVGRect.

::: content/svg/content/src/SVGIRect.h
@@ +9,5 @@
> +#include "nsIDOMSVGRect.h"
> +#include "mozilla/Attributes.h"
> +#include "mozilla/ErrorResult.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsContentUtils.h"

Nit: sort these

::: content/svg/content/src/SVGRect.h
@@ +6,5 @@
>  #ifndef mozilla_dom_SVGRect_h
>  #define mozilla_dom_SVGRect_h
>  
>  #include "gfxRect.h"
> +#include "SVGIRect.h"

Include as "mozilla/dom/SVGRect.h"

@@ +7,5 @@
>  #define mozilla_dom_SVGRect_h
>  
>  #include "gfxRect.h"
> +#include "SVGIRect.h"
> +#include "nsWrapperCache.h"

Remove this include

@@ +24,5 @@
>    // nsISupports interface:
>    NS_DECL_ISUPPORTS
>  
> +  // WebIDL
> +  virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope);

This should be on SVGIRect, and should be non-virtual.

::: dom/bindings/Bindings.conf
@@ +833,5 @@
>      'headerFile': 'mozilla/dom/SVGGradientElement.h',
>  },
>  
> +'SVGRect': {
> +    'wrapperCache': False

This should have 'nativeType': 'mozilla::dom::SVGIRect'

Comment 21

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 22

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

Comment on attachment 728251 [details] [diff] [review]
part 2 - webidl

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

r=me. Phew :)

::: content/svg/content/src/SVGIRect.h
@@ +13,5 @@
> +#include "mozilla/Attributes.h"
> +#include "mozilla/ErrorResult.h"
> +
> +////////////////////////////////////////////////////////////////////////
> +// SVGIRect class

(I don't think this comment is useful.)

::: content/svg/content/src/SVGRect.h
@@ +71,5 @@
>  } // namespace dom
>  } // namespace mozilla
>  
> +nsresult
> +NS_NewSVGRect(mozilla::dom::SVGRect** result,

Could you file a followup to make this not use an outparam?

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

https://tbpl.mozilla.org/?tree=Try&rev=454be8191ba7

Comment 24

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Added using SVGRect::SetX & C.

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

Comment 25

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

Comment on attachment 728655 [details] [diff] [review]
part 2 - webidl

Interdiff looks good. Ship it!

Comment 26

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b81c844b0e5f
https://hg.mozilla.org/integration/mozilla-inbound/rev/086a80ce6c78

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b81c844b0e5f
https://hg.mozilla.org/mozilla-central/rev/086a80ce6c78

Comment 28

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

(In reply to :Ms2ger from comment #22)
> Comment on attachment 728251 [details] [diff] [review]
> ::: content/svg/content/src/SVGRect.h
> @@ +71,5 @@
> >  } // namespace dom
> >  } // namespace mozilla
> >  
> > +nsresult
> > +NS_NewSVGRect(mozilla::dom::SVGRect** result,
> 
> Could you file a followup to make this not use an outparam?

Got around to this already? :)