Closed Bug 795152 Opened 12 years ago Closed 11 years ago

Switch to Services.jsm: What's left

Categories

(Thunderbird :: General, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 20.0

People

(Reporter: aryx, Assigned: aryx)

References

Details

Attachments

(5 files, 10 obsolete files)

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
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)
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)
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-
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.
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+
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.
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)
Assignee: nobody → archaeopteryx
Attachment #665679 - Attachment is obsolete: true
Attachment #665682 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #682118 - Flags: review?(mbanner)
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 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 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+
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+
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
Depends on: 842008
You need to log in before you can comment on or make changes to this bug.