Closed Bug 923026 Opened 6 years ago Closed 6 years ago

Make use of Preferences.jsm in calendar code


(Calendar :: Internal Components, enhancement, P2)



(Not tracked)



(Reporter: Fallen, Assigned: Fallen)


(Whiteboard: [good first bug][mentor=Fallen][lang=js])


(1 file)

I just came across which closely resembles our getPrefSafe / setPref functions. I think we should use this module and get rid of our pref handling functions.

Since addons might be using our getPrefSafe/setPref, we should mark it as deprecated for one cycle and internally use Preferences.jsm.
Severity: normal → enhancement
Attached patch Fix - v1 β€” β€” Splinter Review
Archaeopteryx, can I bug you for the review on this one? You have most experience in doing these kinds of changes.

One issue that might arise is that our setPref/getPrefSafe just returned gracefully if something goes wrong and this version actually throws. I haven't tested this in detail, maybe you could give it a quick check?
Assignee: nobody → philipp
Attachment #8383721 - Flags: review?(archaeopteryx)
Priority: -- → P2
Comment on attachment 8383721 [details] [diff] [review]
Fix - v1

Review of attachment 8383721 [details] [diff] [review]:

Spotted no issues. Two remarks:
- Hard-coding the preference fallback values: This can result in unexpected side effects when a default pref gets changed but not the fallback. Furthermore, I feel uneasy about still continuing with the function if getting a pref which ships by default fails.
- Not yet problematic in the files here, just as simple recommendation for the future (in which more JavaScript modules can be expected): Sorting the imported modules alphabetically avoids accidental double import.
Attachment #8383721 - Flags: review?(archaeopteryx) → review+
As discussed via IRC, we will keep the issues in mind for new code. Changing all existing code to rely on the pref-set default values is deemed to risky.
Pushed to comm-central changeset b14b32a5896e
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.2
You need to log in before you can comment on or make changes to this bug.