Closed
Bug 866796
Opened 9 years ago
Closed 9 years ago
Convert SVGAnimatedRect to WebIDL
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: baku, Assigned: baku)
References
Details
Attachments
(1 file, 7 obsolete files)
39.07 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #743161 -
Flags: review?(Ms2ger)
Comment 2•9 years ago
|
||
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•9 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•9 years ago
|
||
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)
Comment 5•9 years ago
|
||
(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•9 years ago
|
||
(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•9 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.
![]() |
||
Comment 8•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
I don't know if you like the setParentObject()..
Attachment #743179 -
Attachment is obsolete: true
Attachment #744055 -
Flags: review?(Ms2ger)
Comment 11•9 years ago
|
||
nsSVGGlyphFrame has an mContent you can pass, no?
Assignee | ||
Comment 12•9 years ago
|
||
Yes, I can use mContent!
Attachment #744055 -
Attachment is obsolete: true
Attachment #744055 -
Flags: review?(Ms2ger)
Attachment #744072 -
Flags: review?(Ms2ger)
Comment 13•9 years ago
|
||
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•9 years ago
|
||
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 15•9 years ago
|
||
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 16•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=3f6f5005b479
Attachment #744598 -
Attachment is obsolete: true
Attachment #745776 -
Flags: review?(Ms2ger)
Assignee | ||
Comment 17•9 years ago
|
||
Attachment #745776 -
Attachment is obsolete: true
Attachment #745776 -
Flags: review?(Ms2ger)
Attachment #745782 -
Flags: review?(Ms2ger)
Comment 18•9 years ago
|
||
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•9 years ago
|
||
Attachment #745782 -
Attachment is obsolete: true
Attachment #747443 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 20•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/78a43b5ffabd
Keywords: checkin-needed
Comment 21•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/78a43b5ffabd
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Updated•3 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•