... making it obvious, where defaults are expected and what types the prefs are supposed to be of.

I haven't been able to figure out in several instances whether prefs had a default or not (i.e. getting them might throw) and of what type certain prefs were. The new global getAnyPref method should nicely take care of those cases though.

Sorry for the slow review. For fear of throwing, I think I would prefer all the gets/sets to be wrapped in a try/catch block. If the concern is just about knowing where defaults are expected and the types, getAnyPref doesn't help you in that case. I would rather just add comments around the code that gets a pref to document the default and the type.

Tony: Any estimate as to when you'll come to this review?

I admit I've been avoiding this review because in hindsight, I like G_Preferences better (sorry, I know it was my comment that it should be removed). G_Preferences has the following advantages: * It's more scripty (i.e., you don't have to think about the type) * It's safer (try/catch happens automatically) * It allows for better scoping of pref observers. The main disadvantage of G_Preferences is it's different from the rest of the code base, but I think the benefits of it outweigh this. I don't know; I'm willing to hear more arguments about why we should do this.

(In reply to comment #6) > * It's more scripty (i.e., you don't have to think about the type) Not thinking about type could potentially lead to subtle bugs when you try to store a string into an exiting integer pref. As the underlying preference system is strongly typed, using the corresponding set*Pref methods will throw as soon as you make the mistake (and they make the code insofar more readable, as extension developers could easily deduce the type to use for prefs without default values). > * It's safer (try/catch happens automatically) If you have default values, getting/setting a pref should never throw. Automatically try/catching errors in those circumstances may make debugging more difficult. And for prefs without defaults, you can again easier deduce that fact from the code itself. > * It allows for better scoping of pref observers. Not sure what you mean here... the conversion I did was quite straight-forward, even for the pref observers. > The main disadvantage of G_Preferences is it's different from the rest of the > code base, but I think the benefits of it outweigh this. If they really do, then they should be refactored out (into FUEL or a .jsm, as suggested for G_Alarm in bug 382820). Having each component invent its own pref handling code leads to unnecessary code duplication and thus more code, less tested code and finally more difficult to read code for everybody not intimately involved.

By better scoping, I mean you can add pref observers and then call removeAllObservers() to only remove the observers you added. I'd be in favor of refactoring out into FUEL or a .jsm.

(In reply to comment #8) > I'd be in favor of refactoring out into FUEL or a .jsm. Sure -- morphing this bug and adjusting dependencies.

Simon - I can help if you are looking to use FUEL. There may be some tweaks needed to make sure it has the needed functionality, but it appears to have much of what you need.

(In reply to comment #10) > Simon - I can help if you are looking to use FUEL. That'd be great. Unfortunately FUEL won't be an option until bug 380168 has been fixed (as major parts of the safe browsing code live in /toolkit). AFAICT, anyway.

Bug 769960 refactored the safe browsing code into a JSM (SafeBrowsing.jsm) DUPPING