Last Comment Bug 281414 - Rename nsIPrefBranchInternal to nsIPrefBranch2 and freeze it.
: Rename nsIPrefBranchInternal to nsIPrefBranch2 and freeze it.
Product: Core
Classification: Components
Component: Preferences: Backend (show other bugs)
: Trunk
: All All
-- normal (vote)
: ---
Assigned To: Benjamin Smedberg [:bsmedberg]
: Benjamin Smedberg [:bsmedberg]
Depends on:
Blocks: 268520
  Show dependency treegraph
Reported: 2005-02-07 12:58 PST by Benjamin Smedberg [:bsmedberg]
Modified: 2005-03-12 06:37 PST (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Life is short, compatibility hacks are great (21.64 KB, patch)
2005-02-10 16:46 PST, Benjamin Smedberg [:bsmedberg]
darin.moz: review+
darin.moz: superreview+
Details | Diff | Splinter Review

Description User image Benjamin Smedberg [:bsmedberg] 2005-02-07 12:58:19 PST
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.
Comment 1 User image Alec Flett 2005-02-07 13:16:09 PST
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)

Comment 2 User image Benjamin Smedberg [:bsmedberg] 2005-02-07 13:35:42 PST
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.
Comment 3 User image Alec Flett 2005-02-07 13:50:12 PST

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.
Comment 4 User image James Ross 2005-02-07 15:32:07 PST
(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.
Comment 5 User image Darin Fisher 2005-02-07 15:55:30 PST
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 ;-)
Comment 6 User image Benjamin Smedberg [:bsmedberg] 2005-02-10 16:46:26 PST
Created attachment 174002 [details] [diff] [review]
Life is short, compatibility hacks are great

This patch is limited to modules/libpref and builds/works.
Comment 7 User image Alec Flett 2005-02-10 16:56:00 PST
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 8 User image Darin Fisher 2005-02-11 09:07:58 PST
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.
Comment 9 User image timeless 2005-02-11 16:23:07 PST
just a reminder: venkman and chatzilla, and perhaps domi do not want a 'quick
global replace' because they try to support mozilla1.0.
Comment 10 User image Benjamin Smedberg [:bsmedberg] 2005-02-25 12:51:02 PST
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.
Comment 11 User image Simon Fraser 2005-02-25 13:26:45 PST
Why did we have to use a dumb name like nsIPrefBranch2? Why not nsIPrefObservers
or something else that says what the interface does?
Comment 12 User image Darin Fisher 2005-02-25 23:48:35 PST
(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.
Comment 13 User image Frank Wein [:mcsmurf] 2005-03-10 09:50:05 PST
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.

Note You need to log in before you can comment on or make changes to this bug.