Closed Bug 825147 Opened 7 years ago Closed 7 years ago

Convert subclasses of SVGGraphicsElement to WebIDL

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

Attachments

(13 files)

13.52 KB, patch
bzbarsky
: review+
dzbarsky
: checkin+
Details | Diff | Splinter Review
17.25 KB, patch
bzbarsky
: review+
dzbarsky
: checkin+
Details | Diff | Splinter Review
22.74 KB, patch
bzbarsky
: review+
dzbarsky
: checkin+
Details | Diff | Splinter Review
13.19 KB, patch
bzbarsky
: review+
dzbarsky
: checkin+
Details | Diff | Splinter Review
39.92 KB, patch
bzbarsky
: review+
dzbarsky
: checkin+
Details | Diff | Splinter Review
23.63 KB, patch
bzbarsky
: review+
dzbarsky
: checkin+
Details | Diff | Splinter Review
18.20 KB, patch
bzbarsky
: review+
dzbarsky
: checkin+
Details | Diff | Splinter Review
19.70 KB, patch
bzbarsky
: review+
dzbarsky
: checkin+
Details | Diff | Splinter Review
27.35 KB, patch
bzbarsky
: review+
dzbarsky
: checkin+
Details | Diff | Splinter Review
22.51 KB, patch
bzbarsky
: review+
dzbarsky
: checkin+
Details | Diff | Splinter Review
21.16 KB, patch
bzbarsky
: review-
Details | Diff | Splinter Review
5.11 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
19.29 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
No description provided.
Depends on: 824229
Attached patch SVGDefsElementSplinter Review
Attachment #696234 - Flags: review?(bzbarsky)
Attached patch SVGSwitchElementSplinter Review
Attachment #696237 - Flags: review?(bzbarsky)
Attachment #696247 - Flags: review?(bzbarsky)
Attached patch SVGGElementSplinter Review
Attachment #696249 - Flags: review?(bzbarsky)
Attached patch SVGImageElementSplinter Review
Attachment #696589 - Flags: review?(bzbarsky)
Attached patch SVGRectElementSplinter Review
Attachment #696590 - Flags: review?(bzbarsky)
Attached patch SVGCircleElementSplinter Review
Attachment #696592 - Flags: review?(bzbarsky)
Attachment #696591 - Flags: review?(bzbarsky)
Attachment #696593 - Flags: review?(bzbarsky)
Attached patch SVGLineelementSplinter Review
Attachment #696858 - Flags: review?(bzbarsky)
Attached patch SVGAElementSplinter Review
Attachment #696948 - Flags: review?(bzbarsky)
Comment on attachment 696234 [details] [diff] [review]
SVGDefsElement

r=me modulo the include guard thing and MOZ_OVERRIDE on WrapNode.
Attachment #696234 - Flags: review?(bzbarsky) → review+
Comment on attachment 696237 [details] [diff] [review]
SVGSwitchElement

Usual comments here MOZ_OVERRIDE and the include guard.

r=me with that
Attachment #696237 - Flags: review?(bzbarsky) → review+
Comment on attachment 696247 [details] [diff] [review]
SVGForeignObjectElement

With the MOZ_OVERRIDE thing and the include guard thing, r=me
Attachment #696247 - Flags: review?(bzbarsky) → review+
Comment on attachment 696249 [details] [diff] [review]
SVGGElement

r=me with MOZ_OVERRIDE and the include guard thing fixed.
Attachment #696249 - Flags: review?(bzbarsky) → review+
Comment on attachment 696589 [details] [diff] [review]
SVGImageElement

>+++ b/dom/webidl/SVGImageElement.webidl

You need to put the nsIImageLoadingContent stuff in here.  See the patches in bug 825527 for discussion; you should just be able to copy/paste from HTMLImageElement.webidl and have things Just Work, since SVGimageElement inherits from nsImageLoadingContent.  Though you will want to add nsIImageLoadingContent to the classinfo for SVGImageElement.

Also, this patch as is breaks page info for SVG images because that does "instanceof SVGImageElement".  You need to add a 'hasInstanceInterface': 'nsIDOMSVGImageElement' in Bindings.conf.

And the usual MOZ_OVERRIDE and include guard nits.

r=me with the above fixed, though I'd like to see either the updated patch or better yet the interdiff.
Attachment #696589 - Flags: review?(bzbarsky) → review+
Comment on attachment 696590 [details] [diff] [review]
SVGRectElement

r=me with MOZ_OVERRIDE and the include guard thing fixed.
Attachment #696590 - Flags: review?(bzbarsky) → review+
Comment on attachment 696591 [details] [diff] [review]
SVGCircleElement

r=me with MOZ_OVERRIDE and the include guard thing fixed.
Attachment #696591 - Flags: review?(bzbarsky) → review+
Er, I guess for circle you already have the MOZ_OVERRIDE, so nevermind me!
Comment on attachment 696592 [details] [diff] [review]
SVGEllipseElement

r=me
Attachment #696592 - Flags: review?(bzbarsky) → review+
Comment on attachment 696593 [details] [diff] [review]
SVGPolylineElement and SVGPolygonElement

>+++ b/content/svg/content/src/nsSVGPolyElement.cpp
>+nsSVGPolyElement::Points()
>+  nsRefPtr<DOMSVGPointList> points = DOMSVGPointList::GetDOMWrapper(key, this, false).get();

This leaks.  Please drop the .get() on the end.


>+nsSVGPolyElement::AnimatedPoints()
>+  nsRefPtr<DOMSVGPointList> points = DOMSVGPointList::GetDOMWrapper(key, this, false).get();

Likewise, and third arg to GetDOMWrapper here should be true.  I guess we have no test coverage for this, eh?  ;)

r=me with those fixed.
Attachment #696593 - Flags: review?(bzbarsky) → review+
Comment on attachment 696858 [details] [diff] [review]
SVGLineelement

The ellipse changes are perhaps meant to be in the ellipse patch?

r=me with that fixed.
Attachment #696858 - Flags: review?(bzbarsky) → review+
Comment on attachment 696948 [details] [diff] [review]
SVGAElement

I'm surprised this passed tests.  It's missing a WrapNode implementation that would instantiate the right binding.  Pelase fix that and add some tests?

Also, this needs a hasInstanceInterface because we have code in browser.js that does instanceof SVGAElement.
Attachment #696948 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky (:bz) from comment #23)
> >+nsSVGPolyElement::AnimatedPoints()
> >+  nsRefPtr<DOMSVGPointList> points = DOMSVGPointList::GetDOMWrapper(key, this, false).get();

> Likewise, and third arg to GetDOMWrapper here should be true.  I guess we have no test coverage for this, eh?  ;)


> Comment on attachment 696948 [details] [diff] [review]
> SVGAElement
> 
> I'm surprised this passed tests.  It's missing a WrapNode implementation
> that would instantiate the right binding.  Pelase fix that and add some
> tests?
> 

Both of these actually failed tests, I just forgot to upload fixed versions.
Attachment #698364 - Flags: review?(bzbarsky)
Attachment #696234 - Flags: checkin+
Attachment #696237 - Flags: checkin+
Attachment #696247 - Flags: checkin+
Attachment #696249 - Flags: checkin+
Attachment #696589 - Flags: checkin+
Attachment #696590 - Flags: checkin+
Attachment #696591 - Flags: checkin+
Attachment #696592 - Flags: checkin+
Attachment #696593 - Flags: checkin+
Attachment #696858 - Flags: checkin+
Attached patch SVGAElementSplinter Review
Attachment #698462 - Flags: review?(bzbarsky)
Status: NEW → ASSIGNED
Summary: Convert subclasses of SVGGraphicsElement → Convert subclasses of SVGGraphicsElement to WebIDL
Version: unspecified → Trunk
SVGAElement needs hasInstanceInterface
Comment on attachment 698364 [details] [diff] [review]
SVGImageElement interdiff

Fix the comment in nsIImageLoadingContent.idl to point to here too, not just HTMLImageElement, and r=me.

Though maybe we should put all this gunk on a separate [NoInterfaceObject] non-concrete interface (called ImageLoadingContent, say) and just have HTMLImageElement and SVGImageElement implement it?  Either way for this part, though that would make keeping things in sync simpler.

r=me
Attachment #698364 - Flags: review?(bzbarsky) → review+
Comment on attachment 698462 [details] [diff] [review]
SVGAElement

>+                              public mozilla::dom::Link

Just Link.

You still need a hasInstanceInterface here.

r=me if you add that.
Attachment #698462 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/7f39d4e9a70a
https://hg.mozilla.org/mozilla-central/rev/d36a97058e39
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.