nsIPrefBranch.get*Pref should support providing a default value

RESOLVED FIXED in Firefox 54

Status

()

Core
Preferences: Backend
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: florian, Assigned: florian)

Tracking

({dev-doc-needed})

53 Branch
mozilla54
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
Created attachment 8835682 [details] [diff] [review]
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 :-).
Attachment #8835682 - Flags: feedback?(benjamin)

Comment 1

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

Updated

a year ago
Attachment #8835682 - Flags: feedback?(benjamin)
(Assignee)

Comment 2

a year ago
(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.
(Assignee)

Comment 3

a year ago
So Benjamin, can I expect an r+ if I finish the patch?
Flags: needinfo?(benjamin)

Comment 4

a year ago
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.
(Assignee)

Comment 6

a year ago
(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.
(Assignee)

Comment 8

a year ago
Created attachment 8838600 [details] [diff] [review]
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.
Attachment #8838600 - Flags: review?(benjamin)

Updated

a year ago
Attachment #8838600 - Flags: review?(benjamin) → review+
(Assignee)

Comment 9

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

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f6479c35a73c
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Keywords: dev-doc-needed
(Assignee)

Updated

a year ago
Blocks: 1344711
(Assignee)

Updated

7 months ago
Depends on: 1428401
You need to log in before you can comment on or make changes to this bug.