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.
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.
(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 ;-)
Created attachment 174002 [details] [diff] [review] Life is short, compatibility hacks are great This patch is limited to modules/libpref and builds/works.
*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 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.
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.
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 = 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.
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.