1338306 - nsIPrefBranch.get*Pref should support providing a default value

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Florian Quèze [:florian]]

Attachment: Proof of concept

The get{Char,Bool,Int}Pref methods in nsIPrefBranch seem to have been designed with C++ callers in mind, and are painful to use from JS. We have lots of try/catch blocks in our JS code that are there only to catch the exception thrown when a preference doesn't exist, and fallback to a default value.

I think these methods could take a second parameter that would be a default value to return instead of throwing. This shouldn't require any change to existing code, and should simplify new code.

The attached patch is a proof of concept (only handling getCharPref) to get initial feedback.

It may be possible to use a script to simplify callers that are the only instruction in a try block; but I'm not promising this... yet :-).

Comment 1

[Benjamin Smedberg]

It seems like a pretty cool hack on the existing system, but it that better than just using Preferences.jsm?

Comment 2

[Florian Quèze [:florian]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #1)
> It seems like a pretty cool hack on the existing system, but it that better
> than just using Preferences.jsm?

I don't think it really competes with Preferences.jsm, so for me the question isn't really "is this better than using Preferences.jsm?", but "is it better than what we currently have?".

I would say yes, as I see no obvious downside to this hack, and there must be reasons why Preferences.jsm isn't used everywhere in new code (maybe importing a module to read just one pref seems overkill?). I'm seeing more and more usage of defineLazyPreferenceGetter lately, and that's not in Preferences.jsm either.

Comment 3

[Florian Quèze [:florian]]

So Benjamin, can I expect an r+ if I finish the patch?

Comment 4

[Benjamin Smedberg]

I'm sad because I didn't know about defineLazyPreferenceGetter and I hate that this is spread across everywhere. And although I've had a "redesign prefs" plan in the back of my mind for 18 months now, I'm never going to get to it :-(

So yeah, go ahead this should be ok.

Comment 5

[Masatoshi Kimura [:emk]]

Please do not extend getCharPref. It does not handle non-ASCII characters. We should deprecate getCharPref and provide a JS-counterpart of Preferences::GetString.

Comment 6

[Florian Quèze [:florian]]

(In reply to Masatoshi Kimura [:emk] from comment #5)
> Please do not extend getCharPref. It does not handle non-ASCII characters.
> We should deprecate getCharPref and provide a JS-counterpart of
> Preferences::GetString.

I agree that the

  Services.prefs.getComplexValue(pref, Ci.nsISupportsString).data;

and

  let str = Cc["@mozilla.org/supports-string;1"]
              .createInstance(Ci.nsISupportsString);
  str.data = val;
  Services.prefs.setComplexValue(aPrefName, Ci.nsISupportsString, str);

dance is another bucket of pain we should eliminate.

I think adding a getStringPref/setStringPref pair of methods on nsIPrefBranch would be a good solution, and I would be willing to work on it (including cleaning up the existing occurrences of the above pattern in our code base). But this should be a separate bug, and shouldn't block removing the try/catch pain that we currently have for all types of prefs.

Comment 7

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb6e2fdf4c53

Comment 8

[Florian Quèze [:florian]]

Attachment: Patch v2

I didn't handle getComplexValue because I don't think encouraging people to create an nsISupportsString, nsIFile or nsIPrefLocalizedString instance to avoid a try/catch is a good idea.
I guess people could provide null for that parameter though, so let me know if you would prefer getComplexValue to support a default value too.

I wonder if we should consider deprecating getComplexValue altoghether, given that the nsISupportsString case would be nice to replace with a new getStringPref method, the nsIPrefLocalizedString would also be nice to replace with a specific method, and the nsIFile case seems like it would be better to encourage JS callers to use OS.File.

Comment 9

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6479c35a73ca37abded7fe570dccc67d3e38fac
Bug 1338306 - nsIPrefBranch.get*Pref should support providing a default value, r=bsmedberg.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f6479c35a73c