Closed
Bug 724450
Opened 12 years ago
Closed 12 years ago
Replace last Application.prefs uses by Services.prefs, in SeaMonkey
Categories
(SeaMonkey :: MailNews: General, defect)
SeaMonkey
MailNews: General
Tracking
(Not tracked)
VERIFIED
FIXED
seamonkey2.10
People
(Reporter: sgautherie, Assigned: rsx11m.pub)
References
(Blocks 1 open bug, )
Details
Attachments
(1 file, 1 obsolete file)
3.43 KB,
patch
|
mnyromyr
:
review+
mnyromyr
:
superreview+
|
Details | Diff | Splinter Review |
"Found 5 matching lines" NB: Services.* has deprecated Application.*, hasn't it?
Flags: in-testsuite-
Given that I'm responsible for two of these occurrences (bug 520108) I'll have a look at this, it should be fairly straight-forward to swap those calls.
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
Switching those instances from Application.prefs to Services.prefs, for all of these preferences defaults are defined. I assume that bug 513524 will take care of the various gPrefBranch usages.
Attachment #601834 -
Flags: review?(mnyromyr)
Reporter | ||
Comment 3•12 years ago
|
||
(In reply to Serge Gautherie (:sgautherie) from comment #0) > NB: Services.* has deprecated Application.*, hasn't it? Hum, was I wrong at assuming that? (In reply to rsx11m from comment #2) > Switching those instances from Application.prefs to Services.prefs, for all > of these preferences defaults are defined. I assume that bug 513524 will Ah, I hadn't noticed that Application.prefs supported an additional default value :-| http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/exthelper/extIApplication.idl#184 http://mxr.mozilla.org/comm-central/source/mozilla/modules/libpref/public/nsIPrefBranch.idl#89 > take care of the various gPrefBranch usages. Yet, shouldn't your patch account for a default value nonetheless? See previous code at http://hg.mozilla.org/comm-central/rev/06ae9dd4bf85 for reference/example.
(In reply to Serge Gautherie (:sgautherie) from comment #3) > I hadn't noticed that Application.prefs supported an additional default value Yes, the lack of such in Services.prefs.get*Value() confused me a bit as well, which is why I double-checked that all of those preferences actually have some default value defined (which they do). In those cases the additional default specification could be considered redundant (especially if the "real" default changes any time in the future and would need to be adjusted here as well). It is not obvious to me how the case is handled where no default pref is given, apparently the old gPrefBranch usage threw an exception. If that's the same in Services.prefs they could be put into a "try {} catch {}" construct, but that reverts in essence what I've done in bug 520108 to get rid of those. :-\ Mnyromyr, any insights?
Reporter | ||
Comment 5•12 years ago
|
||
(In reply to rsx11m from comment #4) > I double-checked that all of those preferences actually > have some default value defined (which they do). In those cases the > additional default specification could be considered redundant (especially > if the "real" default changes any time in the future and would need to be > adjusted here as well). Can a user "unset" the preference value (in his profile)? > It is not obvious to me how the case is handled where no default pref is > given, apparently the old gPrefBranch usage threw an exception. If that's Easy to test, with a fake pref name. > the same in Services.prefs they could be put into a "try {} catch {}" Services.prefs is just a wrapper to get the service: no modified behavior, unlike Application.prefs. http://mxr.mozilla.org/mozilla-central/source/toolkit/content/Services.jsm#46 > construct, but that reverts in essence what I've done in bug 520108 to get > rid of those. :-\ Yeah, hence I'm not sure anymore whether Application or Services should be preferred. > Mnyromyr, any insights?
As a minimum, I'd change these two occurrences to use the proper getValue() form with explicit defaults, *if* the decision is to stick with Application.prefs: > Application.prefs.get("mail.identity.default.archive_granularity").value; > Application.prefs.get("mail.identity.default.archive_keep_folder_structure").value; On the other hand, one would have to hack defaults/pref/mailnews.js in omni.ja to really remove the defaults for any of these preferences, thus the question is how likely that no-default-defined case is going to happen in practical terms. (In reply to Serge Gautherie (:sgautherie) from comment #5) > Can a user "unset" the preference value (in his profile)? Not sure, one could possibly assign an invalid value or maybe even change the type (e.g., bool or integer to string) of the preference in this way.
Reporter | ||
Comment 7•12 years ago
|
||
(In reply to rsx11m from comment #6) I just meant a user using <about:config>...
No, if you right-click > Reset the default (if defined, and it is here) is taken. Double-clicking on the entry would only allow you to provide values of the given type (thus, you couldn't just change it to string, for example), and you cannot redefine it with New > ...; so AFAICT, the Config Editor should be safe to use.
(In reply to Serge Gautherie (:sgautherie) from comment #5) > > It is not obvious to me how the case is handled where no default pref is > > given, apparently the old gPrefBranch usage threw an exception. If that's > Easy to test, with a fake pref name. I've just tested this case by commenting out mail.forward_message_mode from the default mailnews.js definitions, and without a user_pref set, it throws indeed: > Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED)" I don't get any default checked in the drop-down menu of the "Forward" button, and clicking on it doesn't do anything other than throwing the exception. So, it's a clear indicator that "something" is wrong but in a rather excessive way.
Assignee | ||
Comment 10•12 years ago
|
||
Given that Services.prefs apparently deprecated the old gPrefBranch but not Application.prefs, this is an alternative patch along the lines of comment #6, which just addresses the missing default-value definitions in the current implementation. In accordance with how archive_granularity is used later in the code, the constant for its default is obtained from Ci.nsIMsgIdentity. Please plus one of these patches and minus the other (hopefully not both), depending on which way you think is the right one to go.
Attachment #602059 -
Flags: review?(mnyromyr)
Assignee | ||
Comment 11•12 years ago
|
||
Mnyromyr: Any opinion on this, one way or the other?
Comment 12•12 years ago
|
||
Well, actually, I don't see what's wrong with Application.prefs — it's there, it's not going away anytime soon, and it's only kind of deprecated when "webby" extensions take over, which isn't exactly tomorrow or next week or so. In fact, the Application.prefs "API" is richer than the Services.prefs by far (like exception handling, default values and such), the latter being just a shortcut to the bare interface! Absurdly enough, by now we even have helper functions in MsgComposeCommands.js (getPref) and utilityOverlay.js (GetBoolPref, GetIntPref) which even mimic Application.prefs on a poor scale … That said, since the instances here all have default values, the point is moot.
Updated•12 years ago
|
Attachment #601834 -
Flags: review?(mnyromyr) → review+
Updated•12 years ago
|
Attachment #602059 -
Flags: review?(mnyromyr) → review-
Comment 13•12 years ago
|
||
Comment on attachment 601834 [details] [diff] [review] Proposed patch Oh, and moa=me.
Attachment #601834 -
Flags: superreview+
Assignee | ||
Comment 14•12 years ago
|
||
Comment on attachment 602059 [details] [diff] [review] Just set the archive defaults Thanks, thus let's go with the simpler implementation.
Attachment #602059 -
Attachment is obsolete: true
Assignee | ||
Comment 15•12 years ago
|
||
Push of attachment 601834 [details] [diff] [review] on trunk, please.
Keywords: checkin-needed
Whiteboard: [c-n: comm-central]
Comment 16•12 years ago
|
||
http://hg.mozilla.org/comm-central/rev/11c1e0e006b2 For future patches, please follow the guidelines below to make life easier for those checking in on your behalf. Thanks! https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: comm-central]
Target Milestone: --- → seamonkey2.10
Reporter | ||
Comment 17•12 years ago
|
||
V.Fixed, per MXR. Ftr, there are no more (SMILE) Application.* uses in SeaMonkey atm.
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 18•12 years ago
|
||
(In reply to Karsten Düsterloh from comment #12) > Absurdly enough, by now we even have helper functions in > MsgComposeCommands.js (getPref) and utilityOverlay.js (GetBoolPref, > GetIntPref) which even mimic Application.prefs on a poor scale … I filed bug 734547 and bug 734551 (and a few related ones).
You need to log in
before you can comment on or make changes to this bug.
Description
•