Closed
Bug 795152
Opened 12 years ago
Closed 11 years ago
Switch to Services.jsm: What's left
Categories
(Thunderbird :: General, defect)
Thunderbird
General
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)
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #665678 -
Flags: review?(florian)
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #665679 -
Flags: review?(mconley)
Assignee | ||
Comment 3•12 years ago
|
||
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)
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #665682 -
Flags: review?(mbanner)
Assignee | ||
Comment 5•12 years ago
|
||
For the try build and its failing tests, see https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=209b0867d343
Comment 6•12 years ago
|
||
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•12 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.
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 :)
Assignee | ||
Comment 9•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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 12•12 years ago
|
||
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+
Comment 13•12 years ago
|
||
(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•12 years ago
|
||
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•12 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.
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #665677 -
Attachment is obsolete: true
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #665678 -
Attachment is obsolete: true
Attachment #682112 -
Flags: review?(florian)
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #665680 -
Attachment is obsolete: true
Attachment #665680 -
Flags: review?(squibblyflabbetydoo)
Attachment #682115 -
Flags: review?(squibblyflabbetydoo)
Assignee | ||
Updated•12 years ago
|
Attachment #682112 -
Attachment description: 665678: Switch to Services.jsm: Chat, v2 (review comments addressed) → Switch to Services.jsm: Chat, v2 (review comments addressed)
Assignee | ||
Comment 19•12 years ago
|
||
Assignee | ||
Comment 20•12 years ago
|
||
Assignee: nobody → archaeopteryx
Attachment #665679 -
Attachment is obsolete: true
Attachment #665682 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #682118 -
Flags: review?(mbanner)
Assignee | ||
Comment 21•12 years ago
|
||
These patches passed on Thunderbird-Try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=cb4ec8328a50
Comment 22•12 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 23•12 years ago
|
||
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•12 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•12 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+
Comment 26•11 years ago
|
||
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+
Assignee | ||
Comment 27•11 years ago
|
||
Attachment #682112 -
Attachment is obsolete: true
Assignee | ||
Comment 28•11 years ago
|
||
Attachment #682115 -
Attachment is obsolete: true
Assignee | ||
Comment 29•11 years ago
|
||
Attachment #682118 -
Attachment is obsolete: true
Assignee | ||
Comment 30•11 years ago
|
||
Attachment #682110 -
Attachment is obsolete: true
Assignee | ||
Comment 31•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #682116 -
Attachment is obsolete: true
Assignee | ||
Comment 32•11 years ago
|
||
Successful Try run (with the usual oranges): https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=5ee5ad756203
Keywords: checkin-needed
Comment 33•11 years ago
|
||
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
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
Comment 34•11 years ago
|
||
(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
You need to log in
before you can comment on or make changes to this bug.
Description
•