Closed Bug 343173 Opened 14 years ago Closed Last year
Audit calendar code for places to use preference/string/Services helpers
After getting suitably scolded by shaver for not using more helper, I've implemented some extra functions that make it easier to read a pref and to get a localized string. These now live in calendarUtils.js. We need to look through calendar code for places where we're using the stringBundle service or pref-branches that can instead be replaced with these helper functions. Quick searches that should get most of them: String bundle: http://landfill.mozilla.org/mxr-test/mozilla/search?string=sbs&find=calendar&filter= (ignore the ones in components) Prefs: http://landfill.mozilla.org/mxr-test/mozilla/search?string=nsipref&find=calendar&filter=
Everyone loves helpers (ask shaver for the melody to the song.)
Assignee: base → jminta
Status: NEW → ASSIGNED
Attachment #227737 - Flags: first-review?(dmose)
Comment on attachment 227737 [details] [diff] [review] everyone loves helpers dmose says "This is totally the review to push off to shaver." Helper functions in all their glory.
Attachment #227737 - Flags: first-review?(dmose) → first-review?(shaver)
Comment on attachment 227737 [details] [diff] [review] everyone loves helpers r=shaver. Doesn't that feel nicer!
Attachment #227737 - Flags: first-review?(shaver) → first-review+
"everyone loves helpers" checked in. I suspect there are more pref callers in chrome:// code that need to be cleaned up here. If I'm wrong, someone should feel free to close this bug.
Comment on attachment 257772 [details] [diff] [review] use string helpers for import/export Nice cleanup. Just two small nits: >Index: mozilla/calendar/base/content/import-export.js >+ showError(calGetString("calendar", "unableToRead") + filePath + "\n"+ex ); Nit: add spaces around the +, drop the space at the end, like so: "\n" + ex); >+ showError(calGetString("calendar", "unableToWrite") + filePath ); Nit: drop the extra space at the end. r=mvl with those fixed.
Attachment #257772 - Flags: first-review?(mvl) → first-review+
Patch with nits addressed, ready for checkin. Carry over r=mvl from Comment #6.
Attachment #257844 - Attachment description: use string helpers for import/export, v2 → use string helpers for import/export, v2 [checked in]
Leaving this open
Assignee: jminta → ssitter
Status: ASSIGNED → NEW
Component: Internal Components → Preferences
QA Contact: base → preferences
This should include both pref/string helpers that are calendar specific (cal.getPrefSafe/cal.calGetString) as well as the Services.jsm helpers.
Summary: Audit calendar code for places to use preference/string helpers → Audit calendar code for places to use preference/string/Services helpers
Whiteboard: [good first bug]
Whiteboard: [good first bug] → [good first bug][mentor=Fallen][lang=js]
Whiteboard: [good first bug][mentor=Fallen][lang=js] → [good first bug][lang=js]
Hi I would like to work on this bug. I went through the description and previous commands. Could you give me some pointers as to what my patch is expected to contain when compared to the previous patch submitted?
The previous patch will be kept as is, this could just as well be done in a new bug. For the record: 9:17:27 AM - Fallen: chaithanya: hi. The deal with that bug is that you need to find places that don't use the helpers in Services.jsm and the string helpers, and change them to do so 9:19:08 AM - Fallen: this is services.jsm: http://dxr.mozilla.org/comm-central/source/mozilla/toolkit/modules/Services.jsm 9:19:31 AM - Fallen: so for example, find places that use Components.classes[ 9:19:54 AM - Fallen: Components.classes["@mozilla.org/download-manager;1"] and replace with Services.downloads and make suer the jsm is imported
Assignee: nobody → mschroeder
Status: NEW → ASSIGNED
I will close this bug because in the meantime: 1) all string and preference getters have been converted 2) the eslint rule "mozilla/use-services" would throw an error if a service available from Services.jsm is retrieved directly
Assignee: mschroeder → nobody
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js]
You need to log in before you can comment on or make changes to this bug.