Investigate if nsISVGValueObserver could inherit nsISupportsWeakReference

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

12 years ago
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?
(Assignee)

Comment 1

12 years ago
Created attachment 255203 [details] [diff] [review]
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.
Attachment #255203 - Flags: review?(tor)
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

12 years ago
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.
(Assignee)

Comment 4

12 years ago
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.
Attachment #255203 - Flags: review?(roc)
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.
Attachment #255203 - Flags: review?(roc) → superreview+
(Assignee)

Comment 6

12 years ago
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

12 years ago
Comment on attachment 255203 [details] [diff] [review]
possible patch

We are getting rid of nsISVGValueObserver gradually, there just hasn't been a concerted effort.
Attachment #255203 - Flags: review?(tor) → review+
(Assignee)

Updated

12 years ago
Assignee: general → Olli.Pettay
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.