827119 - Convert SVGSVGElement to WebIDL

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: move SVGSVGElement to mozilla::dom

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: Convert SVGSVGElement to WebIDL

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 698431 [details] [diff] [review]
move SVGSVGElement to mozilla::dom

>-  NS_INTERFACE_MAP_ENTRY(nsISVGPoint)
>+  NS_INTERFACE_MAP_ENTRY(mozilla::nsISVGPoint)

Why is this change needed?  This is all inside mozilla::dom... is there a mozilla::dom::nsISVGPoint that you're trying to avoid?

r=me with that fixed or explained.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 698432 [details] [diff] [review]
Convert SVGSVGElement to WebIDL

You should probably comment that the XPCOM version of SetCurrentScale and SetCurrentTranslate are OK for us because they only throw on non-finite floats and we don't allow those anyway.

Same thing for unsuspendRedraw(All).

>+bool
>+SVGSVGElement::CheckIntersection(nsSVGElement& element,
..
>+  return nullptr;

This compiled?  return false, please.  Same for CheckEnclosure and DeselectAll.

> SVGSVGElement::CreateSVGLength(nsIDOMSVGLength **_retval)
> {
>+  *_retval = CreateSVGLength().get();
>   NS_ADDREF(*_retval = new DOMSVGLength());

Doesn't that leak?  Nuke the NS_ADDREF line?

r=me with those fixed.

Comment 5

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Comment on attachment 698431 [details] [diff] [review]
> move SVGSVGElement to mozilla::dom
> 
> >-  NS_INTERFACE_MAP_ENTRY(nsISVGPoint)
> >+  NS_INTERFACE_MAP_ENTRY(mozilla::nsISVGPoint)
> 
> Why is this change needed?  This is all inside mozilla::dom... is there a
> mozilla::dom::nsISVGPoint that you're trying to avoid?
> 
> r=me with that fixed or explained.

This is needed because NS_INTERFACE_MAP_ENTRY calls NS_GET_IID, which expands as
#define NS_GET_IID(T) (::T::COMTypeInfo<int>::kIID)

Comment 6

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> Comment on attachment 698432 [details] [diff] [review]
> Convert SVGSVGElement to WebIDL
> 
> >+bool
> >+SVGSVGElement::CheckIntersection(nsSVGElement& element,
> ..
> >+  return nullptr;
> 
> This compiled?  return false, please.  Same for CheckEnclosure and
> DeselectAll.

That's strange.  I'm removing these in another patch anyway so it doesn't matter.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> (::T::COMTypeInfo<int>::kIID)

Hmm.  So how did it use to work?  I guess "using" puts things in the global namespace?

In any case, add a comment please.

> I'm removing these in another patch anyway so it doesn't matter.

I fully expect it to not build on all compilers, so just fix before you land.

Comment 8

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50f4d34efeac
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc06e275194

And the comment:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01b68b87ce26

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/50f4d34efeac
https://hg.mozilla.org/mozilla-central/rev/8dc06e275194
https://hg.mozilla.org/mozilla-central/rev/01b68b87ce26