825147 - Convert subclasses of SVGGraphicsElement to WebIDL

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: SVGDefsElement

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: SVGSwitchElement

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: SVGForeignObjectElement

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: SVGGElement

Comment 5

[David Zbarsky (:dzbarsky)]

Attachment: SVGImageElement

Comment 6

[David Zbarsky (:dzbarsky)]

Attachment: SVGRectElement

Comment 7

[David Zbarsky (:dzbarsky)]

Attachment: SVGCircleElement

Comment 8

[David Zbarsky (:dzbarsky)]

Attachment: SVGEllipseElement

Comment 9

[David Zbarsky (:dzbarsky)]

Attachment: SVGPolylineElement and SVGPolygonElement

Comment 10

[David Zbarsky (:dzbarsky)]

Attachment: SVGLineelement

Comment 11

[David Zbarsky (:dzbarsky)]

Attachment: SVGAElement

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696234 [details] [diff] [review]
SVGDefsElement

r=me modulo the include guard thing and MOZ_OVERRIDE on WrapNode.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696237 [details] [diff] [review]
SVGSwitchElement

Usual comments here MOZ_OVERRIDE and the include guard.

r=me with that

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696247 [details] [diff] [review]
SVGForeignObjectElement

With the MOZ_OVERRIDE thing and the include guard thing, r=me

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696249 [details] [diff] [review]
SVGGElement

r=me with MOZ_OVERRIDE and the include guard thing fixed.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696590 [details] [diff] [review]
SVGRectElement

r=me with MOZ_OVERRIDE and the include guard thing fixed.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696591 [details] [diff] [review]
SVGCircleElement

r=me with MOZ_OVERRIDE and the include guard thing fixed.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Er, I guess for circle you already have the MOZ_OVERRIDE, so nevermind me!

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696592 [details] [diff] [review]
SVGEllipseElement

r=me

Comment 21

[Boris Zbarsky [:bzbarsky]]

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.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Boris Zbarsky [:bzbarsky]]

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.

Comment 24

[David Zbarsky (:dzbarsky)]

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

Comment 25

[David Zbarsky (:dzbarsky)]

Attachment: SVGImageElement interdiff

Comment 26

[David Zbarsky (:dzbarsky)]

Pushed the patches before SVGImageElement
https://hg.mozilla.org/integration/mozilla-inbound/rev/f51d9fb6b091
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e56922c8c47
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b3dd403f4b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/908ea24560b5

Comment 27

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbdc0334c124
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ec2ebd28b1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/04feda337f8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd6b8ef397aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/54c8526ef349
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a404732e638

Pushed everything except SVGAElement

Comment 28

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/f51d9fb6b091
https://hg.mozilla.org/mozilla-central/rev/1e56922c8c47
https://hg.mozilla.org/mozilla-central/rev/7b3dd403f4b8
https://hg.mozilla.org/mozilla-central/rev/908ea24560b5
https://hg.mozilla.org/mozilla-central/rev/cbdc0334c124
https://hg.mozilla.org/mozilla-central/rev/5ec2ebd28b1a
https://hg.mozilla.org/mozilla-central/rev/04feda337f8b
https://hg.mozilla.org/mozilla-central/rev/dd6b8ef397aa
https://hg.mozilla.org/mozilla-central/rev/54c8526ef349
https://hg.mozilla.org/mozilla-central/rev/2a404732e638

Comment 29

[David Zbarsky (:dzbarsky)]

Attachment: SVGAElement

Comment 30

[David Zbarsky (:dzbarsky)]

SVGAElement needs hasInstanceInterface

Comment 31

[Boris Zbarsky [:bzbarsky]]

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

Comment 32

[Boris Zbarsky [:bzbarsky]]

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.

Comment 33

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f39d4e9a70a
https://hg.mozilla.org/integration/mozilla-inbound/rev/d36a97058e39

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7f39d4e9a70a
https://hg.mozilla.org/mozilla-central/rev/d36a97058e39