Convert some remaining cases where MailServices.jsm and Services.jsm can be used in Thunderbird JS files

RESOLVED FIXED in Thunderbird 65.0
(NeedInfo from)

Status

enhancement
P5
normal
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: aceman, Assigned: aceman, NeedInfo)

Tracking

Trunk
Thunderbird 65.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

+++ 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.
Posted patch 1508415.patch (obsolete) — Splinter Review
Attachment #9026616 - Flags: review?(mkmelin+mozilla)
Blocks: 720356, 720358
No longer depends on: 720356, 720358
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
Attachment #9026616 - Flags: review?(mkmelin+mozilla) → review+
Posted patch 1508415.patch v1.1 (obsolete) — Splinter Review
Thanks, updated.
Attachment #9026616 - Attachment is obsolete: true
Attachment #9029139 - Flags: review+
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 65.0
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
Keywords: checkin-needed
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().
Flags: needinfo?(mkmelin+mozilla)
(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.
Flags: needinfo?(neil)
(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.
Flags: needinfo?(mkmelin+mozilla)
Changes as per review comments.
Attachment #9029139 - Attachment is obsolete: true
Attachment #9029516 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.