+++ This bug was initially created as a clone of Bug #720358 +++ Using MailServices.js and Services.jsm will clean up the readability of our code a bit, and should avoid the costs the getService call each time we want a service.

Comment on attachment 9026616 [details] [diff] [review] 1508415.patch Review of attachment 9026616 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thx! r=mkmelin ::: mail/test/mozmill/cloudfile/test-cloudfile-add-account-dialog.js @@ +34,5 @@ > for (let lib of MODULE_REQUIRES) { > collector.getModule(lib).installInto(module); > } > > + gCategoryMan = Services.catMan; are you really using the global anymore? If you are, at least you shouldn't - just use the Services.canMan directly ::: mailnews/test/resources/msgFolderListenerSetup.js @@ +3,5 @@ > var nsIMsgDBHdr = Ci.nsIMsgDBHdr; > var nsIArray = Ci.nsIArray; > var nsIMsgFolder = Ci.nsIMsgFolder; > > +var gMFNService = MailServices.mfn; this looks unused

Thanks, updated.

Comment on attachment 9029139 [details] [diff] [review] 1508415.patch v1.1 Review of attachment 9029139 [details] [diff] [review]: ----------------------------------------------------------------- I'm not landing this :-( ::: mailnews/base/prefs/content/aw-accounttype.js @@ +80,1 @@ > .createBundle("chrome://messenger-newsblog/locale/newsblog.properties") This won't work. ::: mailnews/db/gloda/content/glodacomplete.xml @@ +13,5 @@ > <binding id="glodacomplete-rich-result-popup" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-rich-result-popup"> > <implementation implements="nsIAutoCompletePopup"> > + <constructor> > + <![CDATA[ > + ChromeUtils.import("resource://gre/modules/Services.jsm", this); Why do we sometimes use ChromeUtils.import("resource://gre/modules/Services.jsm", this); and this.Services... and other times: ChromeUtils.import("resource://gre/modules/Services.jsm"); and this.maxLinesBeforeMore = Services.prefs.getIntPref( https://searchfox.org/comm-central/rev/4b8113d7332a0b69c05a800e379fcd0bf82f03b3/mail/base/content/mailWidgets.xml#471? M-C just did something even more complicated: https://hg.mozilla.org/mozilla-central/rev/326ed3b7c894#l1.50

BTW, is this really so efficient getting MailServices.mfn every single time? What's wrong with: +var gMFNService = MailServices.mfn; Magnus said it was unused, but that's not the case, it's used throughout that file. I liked v1 better that don't cause all this churn and added CPU cycles.

I don't know if calling MailServices.mfn is particularly slow, but if it isn't much longer than gMFNService we can just use it directly. Similarly to gCategoryMan. But Magnus could know. Thanks for catching the double dot at createBundle().

(In reply to Jorg K (GMT+1) from comment #5) > ::: mailnews/db/gloda/content/glodacomplete.xml > @@ +13,5 @@ > > <binding id="glodacomplete-rich-result-popup" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-rich-result-popup"> > > <implementation implements="nsIAutoCompletePopup"> > > + <constructor> > > + <![CDATA[ > > + ChromeUtils.import("resource://gre/modules/Services.jsm", this); > > Why do we sometimes use > ChromeUtils.import("resource://gre/modules/Services.jsm", this); > and > this.Services... This causes the Services to only be imported local to the binding and not visible from the document using the binding (using elements that are implemented by this binding). I think it was Neil who taught me to do this :) I think it is no problem if we import a module multiple times in the same document, so I don't remember why this form is better. > and other times: > ChromeUtils.import("resource://gre/modules/Services.jsm"); So this imports the Services globally to the document.

(In reply to :aceman from comment #7) > I don't know if calling MailServices.mfn is particularly slow, Should be as fast as the alternative (well, + object variable access, but that's negligible). Re importing jsms in general, yes please import them into the local scope. But, Services.jsm and MailServices.jsm are really globally there in practice, so there's no point putting them into local scope too.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=153b341dcca5d4efd08ed80dcf5579481fe35d9a without the offending .

Changes as per review comments.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/76501a6119d4 Convert some remaining cases where MailServices.jsm and Services.jsm can be used in Thunderbird JS files. r=mkmelin