Closed Bug 1338306 Opened 3 years ago Closed 3 years ago

nsIPrefBranch.get*Pref should support providing a default value

Categories

(Core :: Preferences: Backend, defect)

53 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files)

Attached patch Proof of conceptSplinter Review
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 :-).
Attachment #8835682 - Flags: feedback?(benjamin)
It seems like a pretty cool hack on the existing system, but it that better than just using Preferences.jsm?
Attachment #8835682 - Flags: feedback?(benjamin)
(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.
So Benjamin, can I expect an r+ if I finish the patch?
Flags: needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
Please do not extend getCharPref. It does not handle non-ASCII characters. We should deprecate getCharPref and provide a JS-counterpart of Preferences::GetString.
(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.
Attached patch Patch v2Splinter Review
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.
Attachment #8838600 - Flags: review?(benjamin)
Attachment #8838600 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6479c35a73ca37abded7fe570dccc67d3e38fac
Bug 1338306 - nsIPrefBranch.get*Pref should support providing a default value, r=bsmedberg.
https://hg.mozilla.org/mozilla-central/rev/f6479c35a73c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Blocks: 1344711
Depends on: 1428401
You need to log in before you can comment on or make changes to this bug.