Closed
Bug 1338306
Opened 7 years ago
Closed 7 years ago
nsIPrefBranch.get*Pref should support providing a default value
Categories
(Core :: Preferences: Backend, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: florian, Assigned: florian)
References
Details
(Keywords: dev-doc-needed)
Attachments
(2 files)
2.24 KB,
patch
|
Details | Diff | Splinter Review | |
10.62 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter 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)
Comment 1•7 years ago
|
||
It seems like a pretty cool hack on the existing system, but it that better than just using Preferences.jsm?
Updated•7 years ago
|
Attachment #8835682 -
Flags: feedback?(benjamin)
Assignee | ||
Comment 2•7 years 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•7 years ago
|
||
So Benjamin, can I expect an r+ if I finish the patch?
Flags: needinfo?(benjamin)
Comment 4•7 years 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)
Comment 5•7 years ago
|
||
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•7 years 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 7•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb6e2fdf4c53
Assignee | ||
Comment 8•7 years ago
|
||
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•7 years ago
|
Attachment #8838600 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 9•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f6479c35a73c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•7 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•