Last Comment Bug 795152 - Switch to Services.jsm: What's left
: Switch to Services.jsm: What's left
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 20.0
Assigned To: Sebastian Hengst [:aryx][:archaeopteryx]
:
Mentors:
Depends on: 823449 842008
Blocks: 720356 819770
  Show dependency treegraph
 
Reported: 2012-09-27 15:22 PDT by Sebastian Hengst [:aryx][:archaeopteryx]
Modified: 2013-02-21 04:17 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Switch to Services.jsm: Addressbook, v1 (30.37 KB, patch)
2012-09-27 15:22 PDT, Sebastian Hengst [:aryx][:archaeopteryx]
mconley: review+
Details | Diff | Splinter Review
Switch to Services.jsm: Chat, v1 (work in progress) (17.68 KB, patch)
2012-09-27 15:23 PDT, Sebastian Hengst [:aryx][:archaeopteryx]
florian: review-
Details | Diff | Splinter Review
Switch to Services.jsm: Cloudfile, v1 (work in progress) (3.85 KB, patch)
2012-09-27 15:24 PDT, Sebastian Hengst [:aryx][:archaeopteryx]
mconley: review+
Details | Diff | Splinter Review
Switch to Services.jsm: Message views, v1 (work in progress) (159.31 KB, patch)
2012-09-27 15:26 PDT, Sebastian Hengst [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
Switch to Services.jsm: Other code, v1 (work in progress) (82.53 KB, patch)
2012-09-27 15:29 PDT, Sebastian Hengst [:aryx][:archaeopteryx]
standard8: feedback+
Details | Diff | Splinter Review
Switch to Services.jsm: Addressbook, v2 (review comments addressed) (29.72 KB, patch)
2012-11-15 11:56 PST, Sebastian Hengst [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
Switch to Services.jsm: Chat, v2 (review comments addressed) (12.98 KB, patch)
2012-11-15 11:57 PST, Sebastian Hengst [:aryx][:archaeopteryx]
florian: review+
Details | Diff | Splinter Review
Switch to Services.jsm: Message views, v2 (fixed issues found by Try) (157.45 KB, patch)
2012-11-15 12:01 PST, Sebastian Hengst [:aryx][:archaeopteryx]
squibblyflabbetydoo: review+
Details | Diff | Splinter Review
Switch to Services.jsm: Cloudfile, v2 (review comments addressed) (3.85 KB, patch)
2012-11-15 12:03 PST, Sebastian Hengst [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
Switch to Services.jsm: Other code, v2 (not anymore WIP) (80.34 KB, patch)
2012-11-15 12:04 PST, Sebastian Hengst [:aryx][:archaeopteryx]
mkmelin+mozilla: review+
Details | Diff | Splinter Review
Switch to Services.jsm: Chat, v5. r=florian (13.79 KB, patch)
2012-12-14 08:26 PST, Sebastian Hengst [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
Switch to Services.jsm: Message views, v4. r=squib (202.85 KB, patch)
2012-12-14 08:52 PST, Sebastian Hengst [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
Switch to Services.jsm: Other code, v4. r=mmelin (80.61 KB, patch)
2012-12-14 08:56 PST, Sebastian Hengst [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
Switch to Services.jsm: Addressbook, v6. r=mconley (29.99 KB, patch)
2012-12-14 08:59 PST, Sebastian Hengst [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
Switch to Services.jsm: Cloudfile, v3. r=mconley (4.12 KB, patch)
2012-12-14 09:02 PST, Sebastian Hengst [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review

Description Sebastian Hengst [:aryx][:archaeopteryx] 2012-09-27 15:22:04 PDT
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.
Comment 1 Sebastian Hengst [:aryx][:archaeopteryx] 2012-09-27 15:23:04 PDT
Created attachment 665678 [details] [diff] [review]
Switch to Services.jsm: Chat, v1 (work in progress)
Comment 2 Sebastian Hengst [:aryx][:archaeopteryx] 2012-09-27 15:24:08 PDT
Created attachment 665679 [details] [diff] [review]
Switch to Services.jsm: Cloudfile, v1 (work in progress)
Comment 3 Sebastian Hengst [:aryx][:archaeopteryx] 2012-09-27 15:26:26 PDT
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
Comment 4 Sebastian Hengst [:aryx][:archaeopteryx] 2012-09-27 15:29:36 PDT
Created attachment 665682 [details] [diff] [review]
Switch to Services.jsm: Other code, v1 (work in progress)
Comment 5 Sebastian Hengst [:aryx][:archaeopteryx] 2012-09-27 15:30:19 PDT
For the try build and its failing tests, see https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=209b0867d343
Comment 6 Florian Quèze [:florian] [:flo] 2012-09-27 15:36:00 PDT
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.
Comment 7 Jim Porter (:squib) 2012-09-27 15:37:32 PDT
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 :aceman 2012-09-27 23:12:38 PDT
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 :)
Comment 9 Sebastian Hengst [:aryx][:archaeopteryx] 2012-09-28 02:52:06 PDT
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 10 Mike Conley (:mconley) - (Needinfo me!) 2012-10-02 13:37:50 PDT
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 11 Mike Conley (:mconley) - (Needinfo me!) 2012-10-02 13:39:15 PDT
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.
Comment 12 Mark Banner (:standard8) 2012-10-03 08:50:24 PDT
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");
Comment 13 Mike Conley (:mconley) - (Needinfo me!) 2012-10-10 10:10:33 PDT
(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 14 Mike Conley (:mconley) - (Needinfo me!) 2012-10-16 11:43:07 PDT
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
Comment 15 :aceman 2012-11-06 13:53:53 PST
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.
Comment 16 Sebastian Hengst [:aryx][:archaeopteryx] 2012-11-15 11:56:02 PST
Created attachment 682110 [details] [diff] [review]
Switch to Services.jsm: Addressbook, v2 (review comments addressed)
Comment 17 Sebastian Hengst [:aryx][:archaeopteryx] 2012-11-15 11:57:42 PST
Created attachment 682112 [details] [diff] [review]
Switch to Services.jsm: Chat, v2 (review comments addressed)
Comment 18 Sebastian Hengst [:aryx][:archaeopteryx] 2012-11-15 12:01:39 PST
Created attachment 682115 [details] [diff] [review]
Switch to Services.jsm: Message views, v2 (fixed issues found by Try)
Comment 19 Sebastian Hengst [:aryx][:archaeopteryx] 2012-11-15 12:03:14 PST
Created attachment 682116 [details] [diff] [review]
Switch to Services.jsm: Cloudfile, v2 (review comments addressed)
Comment 20 Sebastian Hengst [:aryx][:archaeopteryx] 2012-11-15 12:04:35 PST
Created attachment 682118 [details] [diff] [review]
Switch to Services.jsm: Other code, v2 (not anymore WIP)
Comment 21 Sebastian Hengst [:aryx][:archaeopteryx] 2012-11-15 12:05:54 PST
These patches passed on Thunderbird-Try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=cb4ec8328a50
Comment 22 Jim Porter (:squib) 2012-11-19 13:14:40 PST
Just so everyone is kept up-to-date, I'm about halfway through code review. I should be done today or tomorrow.
Comment 23 Mark Banner (:standard8) 2012-11-21 00:48:09 PST
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.
Comment 24 Magnus Melin 2012-11-22 12:23:42 PST
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.
Comment 25 Jim Porter (:squib) 2012-11-22 17:43:31 PST
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?
Comment 26 Florian Quèze [:florian] [:flo] 2012-12-14 07:19:36 PST
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.
Comment 27 Sebastian Hengst [:aryx][:archaeopteryx] 2012-12-14 08:26:33 PST
Created attachment 692317 [details] [diff] [review]
Switch to Services.jsm: Chat, v5. r=florian
Comment 28 Sebastian Hengst [:aryx][:archaeopteryx] 2012-12-14 08:52:36 PST
Created attachment 692325 [details] [diff] [review]
Switch to Services.jsm: Message views, v4. r=squib
Comment 29 Sebastian Hengst [:aryx][:archaeopteryx] 2012-12-14 08:56:05 PST
Created attachment 692326 [details] [diff] [review]
Switch to Services.jsm: Other code, v4. r=mmelin
Comment 30 Sebastian Hengst [:aryx][:archaeopteryx] 2012-12-14 08:59:48 PST
Created attachment 692328 [details] [diff] [review]
Switch to Services.jsm: Addressbook, v6. r=mconley
Comment 31 Sebastian Hengst [:aryx][:archaeopteryx] 2012-12-14 09:02:05 PST
Created attachment 692329 [details] [diff] [review]
Switch to Services.jsm: Cloudfile, v3. r=mconley
Comment 32 Sebastian Hengst [:aryx][:archaeopteryx] 2012-12-14 16:10:28 PST
Successful Try run (with the usual oranges): https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=5ee5ad756203
Comment 34 Patrick Cloke [:clokep] 2013-02-17 16:17:05 PST
(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

Note You need to log in before you can comment on or make changes to this bug.