382635 - Factor out G_Preferences into a .jsm (or replace it with FUEL)

Status: RESOLVED DUPLICATE of bug 769960

(Toolkit :: Safe Browsing, defect)

Description

[Simon Bünzli]

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

Comment 1

[Simon Bünzli]

Attachment: fix

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.

Comment 2

[Tony Chang (Google)]

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.

Comment 3

[Simon Bünzli]

Attachment: fix (without getAnyPref and unbitrotted)

Comment 4

[Simon Bünzli]

Attachment: replace G_Preferences with nsIPrefBranch2

Comment 5

[Simon Bünzli]

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

Comment 6

[Tony Chang (Google)]

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.

Comment 7

[Simon Bünzli]

(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.

Comment 8

[Tony Chang (Google)]

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.

Comment 9

[Simon Bünzli]

(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.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 11

[Simon Bünzli]

(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.

Comment 12

[Philip Chee]

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