233111 - nsSVGElement shouldn't inherit nsIDOMSVGElement

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Is there a reason why nsSVGElement inherits nsIDOMSVGElement? All the subclasses
inherit that interface through various nsIDOMSVGXXXElement interfaces anyway
(such as nsIDOMSVGCircleElement). The current code just forces nsSVGElement to
implement all the functions in that interface when most of them are just
forwarded to nsGenericElement.

Also this means that all leafclasses get two interfacepointers to
nsIDOMSVGElement which is asking for trouble (i.e. pointercompares could return
false even though both point at the same object).

Is there a reason for this setup?

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't mean to critizes, i'm just trying to figure out what's going on :)

Comment 2

[Alex Fritze]

> Is there a reason why nsSVGElement inherits nsIDOMSVGElement? All the subclasses
> inherit that interface through various nsIDOMSVGXXXElement interfaces anyway
> (such as nsIDOMSVGCircleElement). The current code just forces nsSVGElement to
> implement all the functions in that interface when most of them are just
> forwarded to nsGenericElement.

Implementing nsIDOMSVGElement in nsSVGElement means it's one less thing to worry
about in the subclasses and it aids maintainance.
I can see how, given that there isn't much in the nsIDOMSVGElement
implementation, folding that code into all nsSVGElement subclasses might be a
performance enhancement, but from a pure maintainabiliy view, I think it is
better to have this code shared and use NS_FORWARD_NSIDOMSVGELEMENT(..) to
forward to it from subclasses. (Virtual inheritance would solve all of these
forwarding problems, but I guess it's a no-no with XPCOM).
Since there are still quite a few unimplemented SVG elements and the whole class
hierarchy is a bit ad-hocish, I don't think it would be a good idea to get rid
of the shared implementation just yet (but maybe in future).

> Also this means that all leafclasses get two interfacepointers to
> nsIDOMSVGElement which is asking for trouble (i.e. pointercompares could return
> false even though both point at the same object).
> 

I don't think that's asking for trouble. After all any class with more than one
interface has more than one interface pointer to nsISupports.
The COM specs are quite categorical about object identity meaning identity of
the pointers returned for QIing for nsISupports. 
Hence we have:
inline NSCAP_BOOL SameCOMIdentity( nsISupports* lhs, nsISupports* rhs ) {
    return nsCOMPtr<nsISupports>( do_QueryInterface(lhs) ) ==         
           nsCOMPtr<nsISupports>( do_QueryInterface(rhs) );
}

We'd even be allowed to return different nsIDOMSVGElement interfaces on
consecutive invocations of QI(nsIDOMSVGElement) and everything should still
work, as long as QI(nsISupports) always returns the same value...

> I don't mean to critizes, i'm just trying to figure out what's going on :)

Critize away - It's about time someone did :-)

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> Implementing nsIDOMSVGElement in nsSVGElement means it's one less thing to 
> worry about in the subclasses and it aids maintainance.
> I can see how, given that there isn't much in the nsIDOMSVGElement
> implementation, folding that code into all nsSVGElement subclasses might be a
> performance enhancement, but from a pure maintainabiliy view, I think it is
> better to have this code shared and use NS_FORWARD_NSIDOMSVGELEMENT(..) to
> forward to it from subclasses.

Actually, you wouldn't need to move any code to the leafclasses. Right now
all(?) the leafclasses inherit and implement nsIDOMSVGElement anyway. They
inherit it through the various nsIDOMSVGXXXElement interfaces and they implement
it by forwarding the calls using NS_FORWARD macros.
They could and should still do that even if you don't make nsSVGElement inherit
nsIDOMSVGElement. This is how HTML does the same thing.

You would still be able to implement and override the parts of nsIDOMSVGElement
that is common in nsSVGElement, no additional code should be required in the
leafclasses. However that should be a lot less code then what is there now.
Especially after bug 232480 is checked in. Keep your eyes open for a patch there
soon.

What you will have to do though is to add the nsIDOMSVGElement to the
QueryInterface implementation in all leafclasses. But I think that might a good
idea anyway, see below.

> Since there are still quite a few unimplemented SVG elements and the whole 
> classhierarchy is a bit ad-hocish, I don't think it would be a good idea to 
> get rid of the shared implementation just yet (but maybe in future).

Ah, i could definitly understand that. That's why I filed this bug before i went
ahead and started making the changes :)

However if the case is that _all_ leafclasses will inherit various
nsIDOMSVGXXXElement interfaces and that _all_ those interfaces will inherit
nsIDOMSVGElement then it should be safe to remove the code. I know that the
SVG-DOM has a lot of evilness so not all nsIDOMSVGXXXElement interfaces might
end up inheriting nsIDOMSVGElement in our implementation. And if that's the case
then this bug is all moot :)

> I don't think that's asking for trouble. After all any class with more than 
> one interface has more than one interface pointer to nsISupports.
> The COM specs are quite categorical about object identity meaning identity of
> the pointers returned for QIing for nsISupports. 

I know it's allowed per COM rules, but it still is a bit risky. Most of the time
people just compare pointers rather then calling SameCOMIdentity. This is
especially risky since in this case the COM-correct pointer isn't the one you'd
get from an implicit cast from a leafinterface to nsIDOMSVGElement. So for
example code like:

nsresult doSomething(nsIDOMSVGElement* aElem);

nsCOMPtr<nsIDOMSVGCircleElement> myCircle = ...
...
rv = doSomething(myCircle);

Would result in a non-COM-correct pointer, which according to COM rules you are
never supposed to use (nsCOMPtr will assert if you assign it to such a pointer)

Comment 4

[Alex Fritze]

> Actually, you wouldn't need to move any code to the leafclasses. Right now
> all(?) the leafclasses inherit and implement nsIDOMSVGElement anyway. They
> inherit it through the various nsIDOMSVGXXXElement interfaces and they implement
> it by forwarding the calls using NS_FORWARD macros.
> They could and should still do that even if you don't make nsSVGElement inherit
> nsIDOMSVGElement. This is how HTML does the same thing.
> You would still be able to implement and override the parts of nsIDOMSVGElement
> that is common in nsSVGElement, no additional code should be required in the
> leafclasses.

I see what you are saying. You want to keep the common baseclass & just remove
the unnecessary nsIDOMSVGElement inheritance, right? Sounds fair enough. I
thought you wanted to get rid of the baseclass alltogether..

[...]
> However if the case is that _all_ leafclasses will inherit various
> nsIDOMSVGXXXElement interfaces and that _all_ those interfaces will inherit
> nsIDOMSVGElement then it should be safe to remove the code. I know that the
> SVG-DOM has a lot of evilness so not all nsIDOMSVGXXXElement interfaces might
> end up inheriting nsIDOMSVGElement in our implementation. And if that's the case
> then this bug is all moot :)

I'm pretty sure it is the case. (It is for the set of interfaces/classes
implemented now, and I'm pretty sure it is in general).
 
> nsresult doSomething(nsIDOMSVGElement* aElem);
> 
> nsCOMPtr<nsIDOMSVGCircleElement> myCircle = ...
> ...
> rv = doSomething(myCircle);
> 
> Would result in a non-COM-correct pointer, which according to COM rules you are
> never supposed to use (nsCOMPtr will assert if you assign it to such a pointer)

Really? That code is not allowed by the specs??

Comment 5

[Alex Fritze]

> I'm pretty sure it is the case. (It is for the set of interfaces/classes
> implemented now, and I'm pretty sure it is in general).

What I mean is, it is indeed the case that all interfaces will indeed inherit
nsIDOMSVGElement through other paths.

So I'm all for getting rid of the nsIDOMSVGElement in nsSVGElement.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: fix

It definitly sucks that we have to add 3 interfaces to all QI implementations.
Maybe we could do something like nsGenericHTMLElement::DOMQueryInterface?

Comment 7

[Alex Fritze]

Comment on attachment 142486 [details] [diff] [review]
fix

looks good. r=afri.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

checked in