281414 - Rename nsIPrefBranchInternal to nsIPrefBranch2 and freeze it.

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Benjamin Smedberg]

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

[Alec Flett]

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

[Benjamin Smedberg]

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

[Alec Flett]

Thanks. 

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

[James Ross]

(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

[Darin Fisher]

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

[Benjamin Smedberg]

Attachment: Life is short, compatibility hacks are great

This patch is limited to modules/libpref and builds/works.

Comment 7

[Alec Flett]

*sigh*
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

[Darin Fisher]

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

[timeless]

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

[Benjamin Smedberg]

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

[Simon Fraser [no longer active]]

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

[Darin Fisher]

(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 =
    do_GetService("@mozilla.org/preferences-service;1");

So, there isn't any point in making this interface logically separate from
nsIPrefBranch.  It's an extension of it.

Comment 13

[Frank Wein [:mcsmurf]]

Might it be that this patch here caused a crasher (or exposed one, i'm certainly
not the expert for this)? See
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=contains&searchfor=nsSystemPref%3A%3AReadSystemPref&vendor=All&product=All&platform=All&sortby=bbid
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.

Comment 14

[Benjamin Smedberg]

systempref crasher fixed (misplaced semicolon)
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsSystemPref.cpp&branch=&root=/cvsroot&subdir=mozilla/extensions/pref/system-pref/src&command=DIFF_FRAMESET&rev1=1.7&rev2=1.8