Rename nsIPrefBranchInternal to nsIPrefBranch2 and freeze it.

RESOLVED FIXED

Status

()

Core
Preferences: Backend
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: bsmedberg, Assigned: bsmedberg)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

12 years ago
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

12 years ago
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)

(Assignee)

Comment 2

12 years ago
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

12 years ago
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

12 years ago
(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

12 years ago
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 ;-)
(Assignee)

Comment 6

12 years ago
Created attachment 174002 [details] [diff] [review]
Life is short, compatibility hacks are great

This patch is limited to modules/libpref and builds/works.
Attachment #174002 - Flags: superreview?(darin)
Attachment #174002 - Flags: review?(darin)

Comment 7

12 years ago
*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

12 years ago
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+

Comment 9

12 years ago
just a reminder: venkman and chatzilla, and perhaps domi do not want a 'quick
global replace' because they try to support mozilla1.0.
(Assignee)

Comment 10

12 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 11

12 years ago
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

12 years ago
(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.
(Assignee)

Comment 14

12 years ago
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
You need to log in before you can comment on or make changes to this bug.