370454 - Investigate if nsISVGValueObserver could inherit nsISupportsWeakReference

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

If nsISVGValueObserver would inherit nsISupportsWeakReference, the size of vtable should be smaller for many SVG objects, I think.
Also that way the comment http://lxr.mozilla.org/seamonkey/source/content/svg/content/src/nsISVGValueObserver.h#52 would be really forced to be true.

Comments?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

This is one way to reduce the size of all the implementations of
nsISVGValueObserver (on 32bit systems 4 bytes, 64bit systems 8 bytes). 

I know it is a bit ugly the make nsI*** to inherit nsSomeClass but that
makes fixing this very easy.

Comment 2

[Robert Longson [:longsonr]]

We seem to be gradually removing nsISVGValueObserver and replacing it with  nsIMutationObserver (bug 359516 and bug 370416 for instance).

I wonder if we could or should take that to its logical conclusion and remove nsISVGValueObserver altogether.

Comment 3

[tor]

While in general this looks good (though you if you were really feeling industrious you could remove the includes of nsWeakReference.h), I'd prefer to leave the style question of a nsI* inheriting from a nsSomeClass to one of the core gecko people.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 255203 [details] [diff] [review]
possible patch

roc, what do you say about this?

One (still ugly) solution would be to rename nsISVGValueObserver to nsSVGValueObserver but leave the IID (which is needed for do_QueryReferent)
or
nsISVGValueObserver could just inherit nsISupportsWeakReference and there would be a separate class (inheriting nsISVGValueObserver) to implement nsISupportsWeakReference.
That would mean copy-pasting code from nsSupportsWeakReference.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 255203 [details] [diff] [review]
possible patch

I'm OK with this. Although it'd be better still to get rid of nsISVGValueObserver if we could.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I hope we could take the patch for now, and if someone comes up 
with a solution to remove nsISVGValueObserver, then even better.

Comment 7

[tor]

Comment on attachment 255203 [details] [diff] [review]
possible patch

We are getting rid of nsISVGValueObserver gradually, there just hasn't been a concerted effort.