428858 - Unify pref getter/setter methods

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Stuart Morgan]

Now that we are no longer compiling PrefPaneBase into each pref pane, there's no need to have two identical copies of the pref getter/setters. They should just be passthroughs to PreferenceManager.

Comment 1

[Stuart Morgan]

Attachment: fix

We had slightly diverged on implementation; this keeps the better implementation of setPref:toBoolean:.

Comment 2

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 315422 [details] [diff] [review]
fix

sr=pink

Comment 3

[Stuart Morgan]

Landed on trunk.

Comment 4

[Chris Lawson (gone)]

Can we land this on branch? Having it in only one place is going to make deploying third-party pref panes for both branch and trunk builds of Camino basically impossible.

Comment 5

[Stuart Morgan]

How so? The API is unchanged.

And if there is an issue, wouldn't landing this on the branch mean breaking things in 1.6.3 instead of 2.0?

Comment 6

[Chris Lawson (gone)]

(In reply to comment #5)
> How so? The API is unchanged.

Prior to this patch landing, pref panes that messed with Gecko prefs needed to check for the existence of mPrefService, and bail if it wasn't there.

Now it's not *ever* there because we got rid of it, so checking for its existence and bailing will cause panes to no-op on trunk. Not checking for its existence and then doing things assuming it exists causes crashes on branch.

Wouldn't it be better to break existing panes (AFAIK, there's only one that deals with Gecko prefs and might have this problem) now and leave the 1.6 branch in a usable state than to allow the one existing pane to keep working and tell any third-party devs from here on out that they're going to have to write different panes for deployment on Camino 1.6 vs. 2.0? I don't really have an answer for that, but it seems like the former is better than the latter to me.

Comment 7

[Stuart Morgan]

No pref pane should ever have actually been looking at mPrefService directly (in reality, we don't expect that it can ever really be null).

I suspect that the real problem here is that I didn't think about the fact that this patch would run afoul of the fragile base class problem. I think we'll have to either add back in the ivar as a placeholder on trunk, or decide that 2.0 will require all prefpanes to be recompiled. I'm not willing to knowingly create a crasher in a security update.

Try adding a dummy ivar on trunk and recompiling; if that fixes your bug, as I suspect it will, then file a new bug for adding it on trunk.