Convert SVGAnimatedRect to WebIDL

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla23
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

No description provided.
Posted patch patch (obsolete) — Splinter Review
Attachment #743161 - Flags: review?(Ms2ger)
Blocks: 853386
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.
> 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.
Posted patch patch (obsolete) — Splinter Review
About the outparams, I prefer to do that in a follow up.
Attachment #743161 - Attachment is obsolete: true
Attachment #743161 - Flags: review?(Ms2ger)
Attachment #743179 - Flags: review?(Ms2ger)
(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...
(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.
> 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.
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 on attachment 743179 [details] [diff] [review]
patch

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

r- for the Creator thing.
Attachment #743179 - Flags: review?(Ms2ger) → review-
Posted patch patch (obsolete) — Splinter Review
I don't know if you like the setParentObject()..
Attachment #743179 - Attachment is obsolete: true
Attachment #744055 - Flags: review?(Ms2ger)
nsSVGGlyphFrame has an mContent you can pass, no?
Posted patch patch (obsolete) — Splinter Review
Yes, I can use mContent!
Attachment #744055 - Attachment is obsolete: true
Attachment #744055 - Flags: review?(Ms2ger)
Attachment #744072 - Flags: review?(Ms2ger)
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
Attachment #744072 - Flags: review?(Ms2ger)
Posted patch patch (obsolete) — Splinter Review
Sorry, my fault! I forgot to do qref :/ This was the old patch.
Attachment #744072 - Attachment is obsolete: true
Attachment #744598 - Flags: review?(Ms2ger)
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
Attachment #744598 - Flags: review?(Ms2ger)
Posted patch patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=3f6f5005b479
Attachment #744598 - Attachment is obsolete: true
Attachment #745776 - Flags: review?(Ms2ger)
Posted patch patch (obsolete) — Splinter Review
Attachment #745776 - Attachment is obsolete: true
Attachment #745776 - Flags: review?(Ms2ger)
Attachment #745782 - Flags: review?(Ms2ger)
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.
Attachment #745782 - Flags: review?(Ms2ger) → review+
Posted patch patchSplinter Review
Attachment #745782 - Attachment is obsolete: true
Attachment #747443 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/78a43b5ffabd
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 872774
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.