Closed Bug 281414 Opened 16 years ago Closed 16 years ago

Rename nsIPrefBranchInternal to nsIPrefBranch2 and freeze it.


(Core :: Preferences: Backend, defect)

Not set





(Reporter: benjamin, Assigned: benjamin)




(1 file)

We should freeze the pref-observer functionality of nsIPrefBranchInternal; it is
a fairly basic functionality needed by various embedding scenarios. Timeless
suggested a good solution to keep binary compat with existing embedders while
not freezing an "internal" interface name:

Rename nsIPrefBranchInternal to nsIPrefBranch2, keeping the ABI/IID the same.
Make a new (empty) interface nsIPrefBranchInternal inheriting from
nsIPrefBranch2, to preserve existing code in the tree.
bah. Lets change our tree to do the right thing. its called search-and-replace, or:

find -name '*.cpp' | perl -pi.bak -e 's/PrefBranchInternal/PrefBranch2/'

I agree with your approach for keeping nsIPrefBranchInternal usable (keeping the
same IID, etc) but we shouldn't have any uses of it in our codebase that
developers might be tempted to use.

Ultimately, I'm not sure why we made nsIPrefBranchInternal derive from
nsIPrefBranch in the first place (it may very well have been my decision but
looking back I'm not sure I'm happy about it :)) Now we have a chance to make
nsIPrefBranchInternal really internal - lets make it derive from nsISupports -
that way we'll be forcing developers to switch over to the new interface rather
than using nsIPrefBranchInternal (because they'll have to update their code to
compile against 1.8)

There is no reason why an internal/private interface can't inherit from a
public/frozen one; in many cases it is a good design decision. Anyway, if we're
all ok with ditching nsIPrefBranchInternal, I can do a global search-n-replace
and check it in, but I'm worried about unnecessarily breaking extensions/etc
that might currently use it.

What I'm saying is: break them, compile-wise.. don't let them keep using
nsIPrefBranchInternal - we want to encourage/enforce the use of the frozen API -
the sooner we get them on a frozen API, the better. Better to break everyone
now, before the next release goes out, then slowly break people as we muck with
nsIPrefBranchInternal sometime in the future.
(In reply to comment #1)
> bah. Lets change our tree to do the right thing. its called
search-and-replace, or:

If only it were that easy. ;)  Just to cite a random (*cough*) example,
ChatZilla needs to work on Mozilla 1.0 - present, so we'll have to check which
interface exists etc.  Not a problem, though, as it is fairly trivial to code.
If time is short (which I know it is), then I'd support bsmedberg's original
plan as an intermediary.  I trust him to go back and remove
nsIPrefBranchInternal later.  Let's focus on getting a better Gecko SDK sooner
than later ;-)
This patch is limited to modules/libpref and builds/works.
Attachment #174002 - Flags: superreview?(darin)
Attachment #174002 - Flags: review?(darin)
at least search 'n replace mozilla code, I beg of you. people look more at our
own code than our [virtually non existant] docs to learn which interfaces to use...
Comment on attachment 174002 [details] [diff] [review]
Life is short, compatibility hacks are great

r+sr=darin for this patch.

I agree with alecf: a quick global search and replace should do the trick for
the rest of the source tree.

Either file a follow-up bug, or better yet keep this bug open for that patch.
Attachment #174002 - Flags: superreview?(darin)
Attachment #174002 - Flags: superreview+
Attachment #174002 - Flags: review?(darin)
Attachment #174002 - Flags: review+
just a reminder: venkman and chatzilla, and perhaps domi do not want a 'quick
global replace' because they try to support mozilla1.0.
Fixed on trunk. I also had to update extensions/pref which used
nsIPrefBranchInternal for the gconf system-pref service.

Also landed global s/nsIPrefBranchInternal/nsIPrefBranch2/ except for
extensions/irc extensions/venkman extensions/inspector.
Closed: 16 years ago
Resolution: --- → FIXED
Why did we have to use a dumb name like nsIPrefBranch2? Why not nsIPrefObservers
or something else that says what the interface does?
(In reply to comment #11)
> Why did we have to use a dumb name like nsIPrefBranch2? Why not nsIPrefObservers
> or something else that says what the interface does?

In defense of dumb names... this one inherits from nsIPrefBranch, so it is an
extension of nsIPrefBranch, so nsIPrefBranch2 fit the bill.  Yeah, sure...
numbered interfaces leave something to be desired... *shrug*

One requirement here was that we freeze the vtable as is, so changing the
inheritance wasn't an option.  The use case is not QI to nsIPrefBranch2 to set
observers, but rather:

  nsCOMPtr<nsIPrefBranch2> prefs =

So, there isn't any point in making this interface logically separate from
nsIPrefBranch.  It's an extension of it.
Might it be that this patch here caused a crasher (or exposed one, i'm certainly
not the expert for this)? See
it shows that the crasher started to appear on the day after this fix was
checked in. Only Linux seems to be affected by this crasher, maybe "preferences
-> enable inheriting system properties" gives you a hint what's wrong.
You need to log in before you can comment on or make changes to this bug.