232201 - nsIPrefBranchInternal should inherit from nsIPrefBranch

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Benjamin Smedberg]

caillon points out that in bug 231076 we increased codesize by 100 bytes which
really shouldn't be necessary. This can really add up since we're converting all
the old nsIPref client code. If nsIPrefBranchInternal inherits from
nsIPrefBranch, we can save a QI and a comptr, and I'm pretty sure it costs nothing.

Comment 1

[Benjamin Smedberg]

Attachment: nsIPrefBranchInternal inherits from nsIPrefBranch

Comment 2

[Alec Flett]

ugh... I'm really not into interface inheritance. I want it to be very expclit
that nsIPrefBranchInternal is a seperate interface. 

is there another reason other than code size to make this change?

Comment 3

[Benjamin Smedberg]

(In reply to comment #2)
> ugh... I'm really not into interface inheritance. I want it to be very expclit
> that nsIPrefBranchInternal is a seperate interface. 

What does this mean? Are you philosophically opposed to interface inheritance? I
try to use interface inheritance whenever appropriate, defined as:

1) interface2 extends the functionality of interface1
2) you'ld never have interface2 without interface1
3) you're not planning on implementing interface2 as a tearoff

In these cases I feel that interface inheritance is not only appropriate, but
should be recommended. This is IMO a model case for interface inheritance.

Comment 4

[Darin Fisher]

i agree with bsmedberg.. plus it reduces the size of a pref branch object by one
WORD ;-)

Comment 5

[Darin Fisher]

Comment on attachment 139918 [details] [diff] [review]
nsIPrefBranchInternal inherits from nsIPrefBranch

i think there are other places (in necko for sure) where you can avoid separate
QI's to nsIPrefBranchInternal just as you have done in
mozPersonalDictionary.cpp

sr=darin

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Actually, would it make sense to rename this thing into nsIPrefBranch*2* while
having it inherit from nsIPrefBranch?  I don't really see what is "internal"
about it, to be honest. If we don't, then the inheritance chain will be kind of
weird when we DO have an nsIPrefBranch2....

Comment 7

[Alec Flett]

Comment on attachment 139918 [details] [diff] [review]
nsIPrefBranchInternal inherits from nsIPrefBranch

The internal is a reserved interface that will never be made public or frozen.
If and when we do a to-be-frozen interface, then yes we should call it "2"

If we need to move a method over to the "2" interface, then we can... but the
internal one will always be internal. 

and I'm convinced, or at least persuaded, that this should inhert.
r=alecf

Comment 8

[Benjamin Smedberg]

checked in... I'm gonna leave this open to patch a small basketful of consumers.

Comment 9

[Benjamin Smedberg]

Attachment: part 2, update many callers

This reduced codesize beyond my wildest hopes, so I've gone through the tree
and updated a lot of clients. I didn't touch mailnews (too painful) or any
clients that use the non-root prefbranch.

Comment 10

[Alec Flett]

Comment on attachment 140068 [details] [diff] [review]
part 2, update many callers

*sigh* I guess this is good for code size and all, I'm just sad to see lots of
code going from calling a frozen interface to calling an internal interface.

r/sr=alec

on a side note, I'm wondering if its time for nsIPrefBranch2, now that we've
decided to freeze nsIObserver after all. Originally we put AddObserver() on
nsIPrefBranchInternal because we didn't want to "freeze" topics, like
pref-changed and so forth. I think we're now in a world where that is fairly
safe. 

When we do that, we probably want to add getStringPref() and getUnicharPref()
to simplify nsA[C]String access to prefs.

Comment 11

[Mikael Parknert]

I can fix this in mailnews when I'm removing nsIPref (bug 226005)

Comment 12

[Darin Fisher]

Comment on attachment 140068 [details] [diff] [review]
part 2, update many callers

sr=darin

very nice!!

Comment 13

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

In my tree, I get Venkman failing to init on a QI error from nsIPrefBranch to
nsIPrefBranchInternal in pref-manager.js, probably because of this fix...

Comment 14

[Benjamin Smedberg]

part 2 got checked in last week, with happy codesize savings. Comment #13 was
due to a bad build.