343173 - Audit calendar code for places to use preference/string/Services helpers

Status: RESOLVED FIXED

(Calendar :: Preferences, defect)

Description

[Joey Minta]

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=

Comment 1

[Joey Minta]

Attachment: everyone loves helpers

Everyone loves helpers (ask shaver for the melody to the song.)

Comment 2

[Joey Minta]

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.

Comment 3

[Mike Shaver (:shaver emeritus)]

Comment on attachment 227737 [details] [diff] [review]
everyone loves helpers

r=shaver.  Doesn't that feel nicer!

Comment 4

[Joey Minta]

"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 5

[Stefan Sitter [:ssitter]]

Attachment: use string helpers for import/export

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 7

[Stefan Sitter [:ssitter]]

Attachment: use string helpers for import/export, v2 [checked in]

Patch with nits addressed, ready for checkin. Carry over r=mvl from Comment #6.

Comment 8

[Matthew (lilmatt) Willis]

Leaving this open

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

This should include both pref/string helpers that are calendar specific (cal.getPrefSafe/cal.calGetString) as well as the Services.jsm helpers.

Comment 10

[chaithanya]

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?

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

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

Comment 12

[Martin Schröder [:mschroeder]]

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