Convert SVGAnimatedRect to WebIDL

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
6 years ago
6 years 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)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 743161 [details] [diff] [review]
patch
Attachment #743161 - Flags: review?(Ms2ger)
(Assignee)

Updated

6 years ago
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.
(Assignee)

Comment 3

6 years ago
> 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.
(Assignee)

Comment 4

6 years ago
Created attachment 743179 [details] [diff] [review]
patch

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.
(Assignee)

Comment 7

6 years ago
> 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-
(Assignee)

Comment 10

6 years ago
Created attachment 744055 [details] [diff] [review]
patch

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?
(Assignee)

Comment 12

6 years ago
Created attachment 744072 [details] [diff] [review]
patch

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)
(Assignee)

Comment 14

6 years ago
Created attachment 744598 [details] [diff] [review]
patch

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)
(Assignee)

Comment 17

6 years ago
Created attachment 745782 [details] [diff] [review]
patch
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+
(Assignee)

Comment 19

6 years ago
Created attachment 747443 [details] [diff] [review]
patch
Attachment #745782 - Attachment is obsolete: true
Attachment #747443 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/78a43b5ffabd
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 872774
You need to log in before you can comment on or make changes to this bug.