The default bug view has changed. See this FAQ.

Switch to Services.jsm: What's left

RESOLVED FIXED in Thunderbird 20.0

Status

Thunderbird
General
--
minor
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: aryx, Assigned: aryx)

Tracking

(Blocks: 2 bugs)

Trunk
Thunderbird 20.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 10 obsolete attachments)

13.79 KB, patch
Details | Diff | Splinter Review
202.85 KB, patch
Details | Diff | Splinter Review
80.61 KB, patch
Details | Diff | Splinter Review
29.99 KB, patch
Details | Diff | Splinter Review
4.12 KB, patch
Details | Diff | Splinter Review
Created attachment 665677 [details] [diff] [review]
Switch to Services.jsm: Addressbook, v1

Using 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.

For compose code, see bug 794909.

A few tests are failing, if you are familiar with these, please investigate this.
Attachment #665677 - Flags: review?(mconley)
Created attachment 665678 [details] [diff] [review]
Switch to Services.jsm: Chat, v1 (work in progress)
Attachment #665678 - Flags: review?(florian)
Created attachment 665679 [details] [diff] [review]
Switch to Services.jsm: Cloudfile, v1 (work in progress)
Attachment #665679 - Flags: review?(mconley)
Created attachment 665680 [details] [diff] [review]
Switch to Services.jsm: Message views, v1 (work in progress)

Excluded sub-trees:
\mail\app\profile\extensions\tbtestpilot@labs.mozilla.com
\mail\base\content\FilterListDialog.js (bug 748965) (aceman)
\mail\base\content\SearchDialog.js (bug 748965) (aceman)
\mail\components\preferences (bug 763106) (aceman)
\mail\test\
\mailnews\base\prefs (aceman)
\suite
Attachment #665680 - Flags: review?(squibblyflabbetydoo)
Created attachment 665682 [details] [diff] [review]
Switch to Services.jsm: Other code, v1 (work in progress)
Attachment #665682 - Flags: review?(mbanner)
For the try build and its failing tests, see https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=209b0867d343
Comment on attachment 665678 [details] [diff] [review]
Switch to Services.jsm: Chat, v1 (work in progress)

>diff --git a/chat/components/src/imAccounts.js b/chat/components/src/imAccounts.js
>--- a/chat/components/src/imAccounts.js
>+++ b/chat/components/src/imAccounts.js
>@@ -1,35 +1,32 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> Cu.import("resource:///modules/imXPCOMUtils.jsm");
> Cu.import("resource:///modules/imServices.jsm");
>+Cu.import("resource://gre/modules/Services.jsm");

imServices.jsm already imports and reexports the Services object from Services.jsm, so please don't import Services.jsm when imServices.jsm is already imported (I don't think it would break anything, but it's duplicated code).


>diff --git a/chat/content/convbrowser.xml b/chat/content/convbrowser.xml
>--- a/chat/content/convbrowser.xml
>+++ b/chat/content/convbrowser.xml
>@@ -604,34 +604,32 @@
> 
>       <!-- nsIObserver implementation -->
>       <method name="observe">
>         <parameter name="aSubject"/>
>         <parameter name="aTopic"/>
>         <parameter name="aData"/>
>         <body>
>         <![CDATA[
>+          Components.utils.import("resource://gre/modules/Services.jsm");

I don't really like these imports in several methods of XBL bindings, especially in methods that are likely going to be called several times.

Is it reasonable to assume that the window using the XBL binding has already imported Services.jsm?

Other changes seem fine.
Attachment #665678 - Flags: review?(florian) → review-

Comment 7

5 years ago
Just an FYI, I will probably not get to my review until October 2-3 at the earliest, since it's currently crunch time for Gaia.

Comment 8

5 years ago
There is also the possibility of using 
Components.utils.import("resource://gre/modules/Services.jsm", this); in those .xml files and then referencing it as this.Services. This should make the import local only to the binding in which it appears. But if there is already a global import as florian says then by all means utilize it :)
The Services object gets cached, so having a to assure the import is pretty cheap. I ran the following performance test in latest Daily with the Workspace extension.

let startTime = Date.now();
for (let i=0; i<10000; i++)
{
  Components.utils.import("resource://gre/modules/Services.jsm");
}
let endTime = Date.now();
Services.console.logStringMessage("10000 imports: " + (endTime - startTime));
Components.utils.unload("resource://gre/modules/Services.jsm");
let startTime = Date.now();
for (let i=0; i<1000; i++)
{
  Components.utils.import("resource://gre/modules/Services.jsm");
  Components.utils.unload("resource://gre/modules/Services.jsm");
}
let endTime = Date.now();
Services.console.logStringMessage("1000 imports and unloads: " + (endTime - startTime));

10000 imports: 2016
1000 imports and unloads: 4552

I don't know much about the unload, but would expect that it deletes the reference to Services.jsm and leaves the real deletion to the garbage collector. But as you can see, the performance of a possible repeated Services.jsm import is very good.
Comment on attachment 665677 [details] [diff] [review]
Switch to Services.jsm: Addressbook, v1

Review of attachment 665677 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK to me (minus the trailing whitespace nit).

Did the gloda tests run ok after this patch?  If so, I'll give my r+.

::: mail/base/content/ABSearchDialog.js
@@ +59,2 @@
>    gSearchPhoneticName =
> +        Services.prefs.getComplexValue("mail.addr_book.show_phonetic_fields", 

Trailing whitespace
Comment on attachment 665679 [details] [diff] [review]
Switch to Services.jsm: Cloudfile, v1 (work in progress)

Review of attachment 665679 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the whitespace fixed.

::: mail/components/cloudfile/nsYouSendIt.js
@@ +260,5 @@
>      let cancel = Ci.nsIMsgCloudFileProvider.uploadCanceled;
>  
>      let args = {storage: aStorageSize};
>      args.wrappedJSObject = args;
> +    Services.ww.openWindow(null, 

While you're here, please strip the trailing whitespace.
Attachment #665679 - Flags: review?(mconley) → review+
Comment on attachment 665682 [details] [diff] [review]
Switch to Services.jsm: Other code, v1 (work in progress)

Review of attachment 665682 [details] [diff] [review]:
-----------------------------------------------------------------

I've only taken a quick look as you've marked this as work in progress, and I didn't spot anything obvious on the quick look.

::: mail/base/modules/MailUtils.js
@@ +237,5 @@
>      let numMessages = aMsgHdrs.length;
>  
>      if ((openWindowWarning > 1) && (numMessages >= openWindowWarning)) {
> +      let bundle = Services.strings.createBundle(
> +                            "chrome://messenger/locale/messenger.properties");

nit: please use 2-space indent for this line, or if it isn't too wide use:

let bundle =
  Services.strings.createBundle("chrome://messenger/locale/messenger.properties");
Attachment #665682 - Flags: review?(mbanner) → feedback+
(In reply to Mike Conley (:mconley) from comment #10)
> Comment on attachment 665677 [details] [diff] [review]
> Switch to Services.jsm: Addressbook, v1
> 
> Review of attachment 665677 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks OK to me (minus the trailing whitespace nit).
> 
> Did the gloda tests run ok after this patch?  If so, I'll give my r+.
> 

Archaeopteryx:

Any update on this?

-Mike
Comment on attachment 665677 [details] [diff] [review]
Switch to Services.jsm: Addressbook, v1

Review of attachment 665677 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK to me - and you mentioned that the Gloda tests passed in IRC. So r=me.

::: mail/base/content/ABSearchDialog.js
@@ +59,2 @@
>    gSearchPhoneticName =
> +        Services.prefs.getComplexValue("mail.addr_book.show_phonetic_fields", 

Trailing whitespace
Attachment #665677 - Flags: review?(mconley) → review+

Comment 15

4 years ago
aryx, you can also import the Services into 'this' in the <constructor> of the proper binding in those XBL files so that they are not reimported on each method call. That may solve florian's problem.
Created attachment 682110 [details] [diff] [review]
Switch to Services.jsm: Addressbook, v2 (review comments addressed)
Attachment #665677 - Attachment is obsolete: true
Created attachment 682112 [details] [diff] [review]
Switch to Services.jsm: Chat, v2 (review comments addressed)
Attachment #665678 - Attachment is obsolete: true
Attachment #682112 - Flags: review?(florian)
Created attachment 682115 [details] [diff] [review]
Switch to Services.jsm: Message views, v2 (fixed issues found by Try)
Attachment #665680 - Attachment is obsolete: true
Attachment #665680 - Flags: review?(squibblyflabbetydoo)
Attachment #682115 - Flags: review?(squibblyflabbetydoo)
Attachment #682112 - Attachment description: 665678: Switch to Services.jsm: Chat, v2 (review comments addressed) → Switch to Services.jsm: Chat, v2 (review comments addressed)
Created attachment 682116 [details] [diff] [review]
Switch to Services.jsm: Cloudfile, v2 (review comments addressed)
Created attachment 682118 [details] [diff] [review]
Switch to Services.jsm: Other code, v2 (not anymore WIP)
Assignee: nobody → archaeopteryx
Attachment #665679 - Attachment is obsolete: true
Attachment #665682 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #682118 - Flags: review?(mbanner)
These patches passed on Thunderbird-Try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=cb4ec8328a50

Comment 22

4 years ago
Just so everyone is kept up-to-date, I'm about halfway through code review. I should be done today or tomorrow.
Comment on attachment 682118 [details] [diff] [review]
Switch to Services.jsm: Other code, v2 (not anymore WIP)

Not going to have time for this for a few days, so passing on to see if anyone else will.
Attachment #682118 - Flags: review?(mbanner) → review?(mkmelin+mozilla)

Comment 24

4 years ago
Comment on attachment 682118 [details] [diff] [review]
Switch to Services.jsm: Other code, v2 (not anymore WIP)

Review of attachment 682118 [details] [diff] [review]:
-----------------------------------------------------------------

Didn't find much to complain about :) 
r=mkmelin

::: mailnews/base/util/mailnewsMigrator.js
@@ +17,1 @@
>  //Components.utils.import("resource:///modules/Services.js");

Remove the commented out line while you're at it.
Attachment #682118 - Flags: review?(mkmelin+mozilla) → review+

Comment 25

4 years ago
Comment on attachment 682115 [details] [diff] [review]
Switch to Services.jsm: Message views, v2 (fixed issues found by Try)

Review of attachment 682115 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, this looks good, aside from my questions/comments below. r=me with those addressed.

::: mail/base/content/folderPane.js
@@ +1952,5 @@
>        let iter;
>        try {
>          iter = fixIterator(this._folder.subFolders, Ci.nsIMsgFolder);
>        } catch (ex) {
> +        Services.console.logStringMessage("Discovering children for " + this._folder.URI + 

Please remove the trailing whitespace here.

::: mail/base/content/mailWidgets.xml
@@ +1557,5 @@
>             extends="chrome://messenger/content/mailWidgets.xml#search-menulist-abstract">
>      <implementation>
>        <field name="stringBundle">
>          <![CDATA[
> +          Components.utils.import("resource://gre/modules/Services.jsm");

Are we sure that multiple imports are better than just using Components.classes?

::: mail/base/content/msgMail3PaneWindow.js
@@ +1214,5 @@
> +        Services.prefs.setBoolPref("mail.spam.logging.enabled",
> +          Services.prefs.getBoolPref(prefix + "spamLoggingEnabled"));
> +      if (Services.prefs.prefHasUserValue(prefix + "markAsReadOnSpam"))
> +        Services.prefs.setBoolPref("mail.spam.markAsReadOnSpam",
> +          Services.prefs.getBoolPref(prefix + "markAsReadOnSpam"));

Could you put curly braces around these if blocks? It's a little confusing to see multi-line if blocks without braces.

::: mail/base/content/tabmail.xml
@@ +1831,5 @@
>            }
>            this.mTabstripClosebutton.collapsed = this.mCloseButtons != 3;
>          ]]></body>
>        </method>
>          

Can you remove this trailing whitespace while you're here?
Attachment #682115 - Flags: review?(squibblyflabbetydoo) → review+

Updated

4 years ago
Blocks: 819770
Comment on attachment 682112 [details] [diff] [review]
Switch to Services.jsm: Chat, v2 (review comments addressed)

Any reason for removing the "Cu.import("resource://gre/modules/Services.jsm");" line from chat/components/src/smileProtocolHandler.js since attachment 665678 [details] [diff] [review]?

r=me with that line added back or with a good explanation of why it's not needed there.
Attachment #682112 - Flags: review?(florian) → review+
Created attachment 692317 [details] [diff] [review]
Switch to Services.jsm: Chat, v5. r=florian
Attachment #682112 - Attachment is obsolete: true
Created attachment 692325 [details] [diff] [review]
Switch to Services.jsm: Message views, v4. r=squib
Attachment #682115 - Attachment is obsolete: true
Created attachment 692326 [details] [diff] [review]
Switch to Services.jsm: Other code, v4. r=mmelin
Attachment #682118 - Attachment is obsolete: true
Created attachment 692328 [details] [diff] [review]
Switch to Services.jsm: Addressbook, v6. r=mconley
Attachment #682110 - Attachment is obsolete: true
Created attachment 692329 [details] [diff] [review]
Switch to Services.jsm: Cloudfile, v3. r=mconley
Attachment #682116 - Attachment is obsolete: true
Successful Try run (with the usual oranges): https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=5ee5ad756203
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/4379140e4ffc
https://hg.mozilla.org/comm-central/rev/df332995c5b4
https://hg.mozilla.org/comm-central/rev/880f9d2706e0
https://hg.mozilla.org/comm-central/rev/4733c850b67c
https://hg.mozilla.org/comm-central/rev/5dc92dfb08ad
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
Depends on: 823449
(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> Comment on attachment 682112 [details] [diff] [review]
> Switch to Services.jsm: Chat, v2 (review comments addressed)
> 
> Any reason for removing the
> "Cu.import("resource://gre/modules/Services.jsm");" line from
> chat/components/src/smileProtocolHandler.js since attachment 665678 [details] [diff] [review]
> [details] [diff] [review]?
> 
> r=me with that line added back or with a good explanation of why it's not
> needed there.

Also committed for Instantbird http://hg.instantbird.org/instantbird/rev/2ab307d5880b

Updated

4 years ago
Depends on: 842008
You need to log in before you can comment on or make changes to this bug.