Closed
Bug 722187
Opened 12 years ago
Closed 12 years ago
convert mailnews/addrbook/ to Services.jsm and MailServices.js
Categories
(MailNews Core :: Address Book, defect)
MailNews Core
Address Book
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 13.0
People
(Reporter: aceman, Assigned: aceman)
References
Details
Attachments
(1 file, 3 obsolete files)
32.29 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
Convert all files except tests.
Tests (unconverted) seem to pass after these changes. I can do tests in a follow-up bug.
Attachment #592538 -
Flags: review?(mconley)
Comment 2•12 years ago
|
||
Comment on attachment 592538 [details] [diff] [review] patch Review of attachment 592538 [details] [diff] [review]: ----------------------------------------------------------------- Hey aceman, Thanks for the patch! This looks good. Just a few questions / comments, -Mike ::: mailnews/addrbook/content/abMailListDialog.js @@ +40,5 @@ > > var gListCard; > var gEditList; > var oldListName = ""; > +var gHeaderParser = MailServices.headerParser; If we use MailServices.headerParser, do we even need gHeaderParser anymore? Why alias? ::: mailnews/addrbook/prefs/content/pref-directory-add.js @@ +29,5 @@ > } > > function Startup() > { > + gPrefInt = Services.prefs; Why do we need to alias to gPrefInt, if we can just use Services.prefs? @@ -319,4 +313,4 @@ > > if (!errorValue) { > > // XXX Due to the LDAP c-sdk pass a dummy url to the IO service, then > > // update the parts (bug 473351). > > - var ldapUrl = Components.classes["@mozilla.org/network/io-service;1"] > > + var ldapUrl = Services.io.newURI( While you're here, we prefer let over var now. ::: mailnews/addrbook/prefs/content/pref-editdirectories.js @@ -98,4 +94,4 @@ > > abList.removeChild(abList.lastChild); > > > > // Init the address book list > > - var addressBooks = Components.classes["@mozilla.org/abmanager;1"] > > + var addressBooks = MailServices.ab.directories; Do we really need to alias to addressBooks? If so, use let instead of var. @@ -132,4 +126,4 @@ > > // If the disable delete button pref for the selected directory is set, > > // disable the delete button for that directory. > > var disable = false; > > - var prefs = Components.classes["@mozilla.org/preferences-service;1"] > > + var ab = MailServices.ab.getDirectory(abList.value); We prefer let over var. @@ -169,4 +159,4 @@ > > > > if (abList && abList.selectedItem) { > > var abURI = abList.value; > > - var ab = Components.classes["@mozilla.org/abmanager;1"] > > + var ab = MailServices.ab.getDirectory(abURI); We prefer let over var.
Attachment #592538 -
Flags: review?(mconley) → review-
I have left it because some of the statements were shorter that way. But if resolving the full 'Services.<service>' is equally fast then I have no problem with that. Patch updated.
Attachment #592538 -
Attachment is obsolete: true
Attachment #596747 -
Flags: review?(mconley)
Comment 4•12 years ago
|
||
Comment on attachment 596747 [details] [diff] [review] patch v2 Review of attachment 596747 [details] [diff] [review]: ----------------------------------------------------------------- Hey aceman, Thanks for this - I think I made a mistake with my last review with regards to aliasing in one part. I've pointed out where / why below. Also, there are two places where we should be switching "var" to "let". Modulo those changes, and assuming we haven't regressed horribly, r+=me. -Mike ::: mailnews/addrbook/prefs/content/pref-directory-add.js @@ +310,5 @@ > errorValue = "invalidResults"; > if (!errorValue) { > // XXX Due to the LDAP c-sdk pass a dummy url to the IO service, then > // update the parts (bug 473351). > + var ldapUrl = Services.io.newURI( This should be: let ldapUrl = Services... ::: mailnews/addrbook/prefs/content/pref-editdirectories.js @@ -104,2 +97,2 @@ > > var holdingArray = []; > > - > > + while (MailServices.ab.directories && MailServices.ab.directories.hasMoreElements()) { Hm - on second thought, I think I made a mistake here. MailServices.ab.directories gives us an enumerator. If I'm not mistaken, doing direct calls on MailServices.ab.directories will place those calls on new enumerators each time. So, in this case, we *do* want an alias. Sorry about that. ::: mailnews/addrbook/src/nsAbLDAPAttributeMap.js @@ +152,2 @@ > // get the right pref branch > + var branch = Services.prefs.getBranch(aPrefBranchName + "."); let instead of var.
Attachment #596747 -
Flags: review?(mconley) → review+
You are right, there is the same iteration over directories in mailnews/addrbook/content/addrbookWidgets.xml .
Attachment #596747 -
Attachment is obsolete: true
Attachment #596773 -
Flags: review?(mconley)
Comment 6•12 years ago
|
||
Comment on attachment 596773 [details] [diff] [review] patch v3 Review of attachment 596773 [details] [diff] [review]: ----------------------------------------------------------------- Looks good - thanks for your work, -Mike
Attachment #596773 -
Flags: review?(mconley) → review+
Keywords: checkin-needed
Comment 7•12 years ago
|
||
Checked in: http://hg.mozilla.org/comm-central/rev/59943cb43b16
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
Comment 8•12 years ago
|
||
I've backed out the changeset since it was causing test failures (see bug 727154). Aceman - could you investigate why this patch causes failures, locally?
Sorry about that. I was sure no test was failing for me. Will check again.
Comment 10•12 years ago
|
||
Mike don't forget to reopen the bug when you back things out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #8) > I've backed out the changeset since it was causing test failures (see bug > 727154). Well, I am not sure I am running tests in /mail/test/* , maybe I am running only tests in /mailnews/* :(
Comment 12•12 years ago
|
||
Ludo: Whoops - my bad - thought I'd done that, and I guess I didn't. Aceman: No worries. :) Our tests can be a bit confusing, since they're spread between two directories (mail / mailnews), and two frameworks (XPCShell and Mozmill). XPCShell allows us to test XPCOM components without a UI layer. Mozmill allows us to "drive" Thunderbird with Javascript. Think of it like Selenium, but for Mozilla applications. We can automate clicks, user input, etc, for end-to-end testing. To run the subset of tests that was failing on trunk, use this command from your objdir: make SOLO_TEST=folder-widget/test-message-filters.js mozmill-one This will run the test-message-filters.js test. This was the failure trace: SUMMARY-UNEXPECTED-FAIL | test-message-filters.js | test-message-filters.js::test_address_books_appear_in_message_filter_dropdown EXCEPTION: Did not display the correct number of address books in the filter menu list.: '2' != '0'. at: test-folder-display-helpers.js line 2842 assert_true(false,"Did not display the correct number of address books in the filter menu list.: '2' != '0'.") test-folder-display-helpers.js 2842 assert_equals(2,0,"Did not display the correct number of address books in the filter menu list.") test-folder-display-helpers.js 2829 filterEditorOpened([object Object]) test-message-filters.js 230 frame.js 552 WindowWatcher_notify([object XPCWrappedNative_NoHelper]) test-window-helpers.js 365 So it sounds like the address books aren't displaying properly in the Message Filter dialog when creating a new filter. Steps to reproduce: 1) Open up the Message Filter dialog, and choose to create a new filter. 2) Click on the "Subject" drop down, and change it to "From" 3) Click on the "contains" drop down and change it to "is in my address book" What's expected? The dropdown to the right of "is in my address book" should contain a list of all address books. What happens? According to the Mozmill test output, the list is empty. aceman - let me know if you need more help looking into this.
Assignee | ||
Comment 13•12 years ago
|
||
Thanks, I think I can work from here. Maybe you could help me how to run the tests in parallel. It appears to me they are running serially and thus take very long.
Comment 14•12 years ago
|
||
(In reply to :aceman from comment #13) > Thanks, I think I can work from here. > > Maybe you could help me how to run the tests in parallel. It appears to me > they are running serially and thus take very long. mozmill tests run Thunderbird, and rely on profile state, so they can't be run in parallel. Nor can xpcshell-tests, since they also rely on profile state.
Comment 15•12 years ago
|
||
aceman: Are you sure you're only running test-message-filters.js? It takes me approximately 20 seconds to run those tests from start to finish. Make sure you're using: make SOLO_TEST=folder-widget/test-message-filters.js mozmill-one in your objdir. Alternatively, you can temporarily rename the test functions that don't concern this orange, so that they don't start with "test". That way, when running this command, only your test will be run. This *may* affect the outcome of the test, since state is preserved from test to test within a single script. It's a sloppy hack, but it helps make the tests fly by faster when you're focusing on just one of them. Just don't forget to rename them back. :) -Mike
Assignee | ||
Comment 16•12 years ago
|
||
No, I was asking generally. I wanted to speed up running of all tests. It takes several minutes (when just using the subset I was running). If I have to run some more now (in /mail) then it gets worse.
Assignee | ||
Comment 17•12 years ago
|
||
Does TB has such -inbound checkin tree (as FF) where patches would be tested and rejected from ever landing if they break tests?
Comment 18•12 years ago
|
||
(In reply to :aceman from comment #17) > Does TB has such -inbound checkin tree (as FF) where patches would be tested > and rejected from ever landing if they break tests? we don't we use try servers for that. Maybe mike can launch a try build if it helps. I think you should request an hg account with try build bits so you could use it yourself.
Comment 19•12 years ago
|
||
aceman: Yes, we use try-builds for that sort of thing. I'd be happy to kick off a try build for you. But getting you committer access (level 1) would be better in the long run. http://www.mozilla.org/hacking/committer/ -Mike
Assignee | ||
Comment 20•12 years ago
|
||
OK, I understand. I do not wish such rights yet. I'll look into the breakage, you do not need to do anything yet.
Assignee | ||
Comment 21•12 years ago
|
||
I run 'cd $BINDIR/mailnews; make xpcshell-tests'. Do I need to do the same in /mail to run the other tests?
Comment 22•12 years ago
|
||
Hey aceman, In order to run the Mozmill tests, you need to run: make SOLO_TEST=folder-widget/test-message-filters.js mozmill-one From your objdir -Mike
Assignee | ||
Comment 23•12 years ago
|
||
Do I have to run each one individually??
Comment 24•12 years ago
|
||
there are other xpcshell tests here: objdir-tb/mail/base/test to run the mozmill tests, cd objdir make mozmill
Comment 25•12 years ago
|
||
you can run the individually, by directory, or all. here's my cheat sheet: https://wiki.mozilla.org/User:Bienvenu/Cheat_Sheet
Assignee | ||
Comment 26•12 years ago
|
||
(In reply to David :Bienvenu from comment #25) > you can run the individually, by directory, or all. > here's my cheat sheet: > https://wiki.mozilla.org/User:Bienvenu/Cheat_Sheet Thanks, I use this list but it is not obvious from that how to run the mozmill tests. It also diffest from what you write in comment 24 (in directory paths etc).
Assignee | ||
Comment 27•12 years ago
|
||
Also, running the one test does not produce the error, as far as I can see: make SOLO_TEST=folder-widget/test-message-filters.js mozmill-one unset PYTHONHOME && cd ./mozilla/_tests/mozmill && MACOSX_DEPLOYMENT_TARGET= \ /var/SSD/TB-hg/tbird-bin/./mozilla/_tests/mozmill/../mozmill-virtualenv/bin/python runtest.py \ --test=/var/SSD/TB-hg/mail/test/mozmill/folder-widget/test-message-filters.js \ --binary=../../.././mozilla/dist/bin/thunderbird \ --symbols-path=/var/SSD/TB-hg/tbird-bin/./mozilla/dist/crashreporter-symbols \ Using profile dir: /var/SSD/TB-hg/tbird-bin/mozilla/_tests/mozmill/mozmillprofile WARNING: Failed to setup pref service.: file /var/SSD/TB-hg/mozilla/toolkit/xre/nsXREDirProvider.cpp, line 742 WARNING: GetDefaultCharsetForLocale: need to add multi locale support: file /var/SSD/TB-hg/mozilla/intl/locale/src/unix/nsUNIXCharset.cpp, line 150 ++DOCSHELL 0xb00c6400 == 1 Warning: unrecognized command line flag -jsbridge Warning: unrecognized command line flag -foreground ++DOCSHELL 0xaff99c00 == 2 ++DOMWINDOW == 1 (0xb34b48c8) [serial = 1] [outer = (nil)] ++DOMWINDOW == 2 (0xb34b4a68) [serial = 2] [outer = 0xb34b4880] ++DOMWINDOW == 3 (0xb34b5908) [serial = 3] [outer = (nil)] ++DOMWINDOW == 4 (0xb34b5aa8) [serial = 4] [outer = 0xb34b58c0] ++DOCSHELL 0xad359000 == 3 ++DOMWINDOW == 5 (0xb34b77e8) [serial = 5] [outer = (nil)] ++DOCSHELL 0xad35bc00 == 4 ++DOMWINDOW == 6 (0xb34b7988) [serial = 6] [outer = (nil)] ++DOCSHELL 0xad35d000 == 5 ++DOMWINDOW == 7 (0xb34b7b28) [serial = 7] [outer = (nil)] ++DOCSHELL 0xad493800 == 6 ++DOMWINDOW == 8 (0xb34b7cc8) [serial = 8] [outer = (nil)] Chrome file doesn't exist: /var/SSD/TB-hg/tbird-bin/mozilla/dist/bin/chrome/toolkit/content/global/printUtils.js ++DOCSHELL 0xace83800 == 7 ++DOMWINDOW == 9 (0xacfd2548) [serial = 9] [outer = (nil)] ++DOCSHELL 0xacec1000 == 8 ++DOMWINDOW == 10 (0xacfd26e8) [serial = 10] [outer = (nil)] ++DOCSHELL 0xacec2800 == 9 ++DOMWINDOW == 11 (0xacfd2888) [serial = 11] [outer = (nil)] ++DOMWINDOW == 12 (0xacfd5468) [serial = 12] [outer = 0xacfd2840] ++DOMWINDOW == 13 (0xacfd5608) [serial = 13] [outer = 0xacfd2500] ++DOMWINDOW == 14 (0xacfd5948) [serial = 14] [outer = 0xb34b77a0] WARNING: Subdocument container has no frame: file /var/SSD/TB-hg/mozilla/layout/base/nsDocumentViewer.cpp, line 2432 ++DOMWINDOW == 15 (0xacfd5ae8) [serial = 15] [outer = 0xb34b7940] ++DOMWINDOW == 16 (0xacfd5c88) [serial = 16] [outer = 0xb34b7ae0] WARNING: Subdocument container has no frame: file /var/SSD/TB-hg/mozilla/layout/base/nsDocumentViewer.cpp, line 2432 ++DOMWINDOW == 17 (0xacfd5e28) [serial = 17] [outer = 0xb34b7c80] ++DOMWINDOW == 18 (0xacfd5fc8) [serial = 18] [outer = 0xacfd2500] ++DOMWINDOW == 19 (0xacfd6168) [serial = 19] [outer = 0xacfd26a0] ++DOMWINDOW == 20 (0xacfd6648) [serial = 20] [outer = 0xacfd2840] JavaScript strict warning: chrome://messenger/content/folderWidgets.xml, line 56: reference to undefined property node.id JavaScript strict warning: chrome://messenger/content/tabmail.xml, line 375: reference to undefined property aTabType.panelId JavaScript strict warning: chrome://global/content/bindings/popup.xml, line 0: reference to undefined property this.popupBoxObject.popupState pldhash: for the table at address 0xac7a31c4, the given entrySize of 52 probably favors chaining over double hashing. ++DOMWINDOW == 21 (0xacfd7008) [serial = 21] [outer = 0xb34b77a0] Failed to load file:///var/SSD/TB-hg/tbird-bin/mozilla/dist/bin/chrome/messenger/content/messenger/AccountManager.js ++DOMWINDOW == 22 (0xacfd71a8) [serial = 22] [outer = 0xb34b77a0] WARNING: NS_ENSURE_TRUE(mScriptGlobalObject) failed: file /var/SSD/TB-hg/mozilla/content/xul/document/src/nsXULDocument.cpp, line 3622 --DOMWINDOW == 21 (0xacfd5608) [serial = 13] [outer = 0xacfd2500] [url = about:blank] --DOMWINDOW == 20 (0xacfd5468) [serial = 12] [outer = 0xacfd2840] [url = about:blank] --DOMWINDOW == 19 (0xacfd7008) [serial = 21] [outer = 0xb34b77a0] [url = chrome://messenger/content/msgAccountCentral.xul] --DOMWINDOW == 18 (0xacfd5948) [serial = 14] [outer = 0xb34b77a0] [url = about:blank] Traceback (most recent call last): File "runtest.py", line 514, in <module> ThunderTestCLI().run() File "/var/SSD/TB-hg/tbird-bin/mozilla/_tests/mozmill-virtualenv/lib/python2.6/site-packages/mozmill/__init__.py", line 793, in run self.mozmill.start(runner=runner, profile=runner.profile) File "/var/SSD/TB-hg/tbird-bin/mozilla/_tests/mozmill-virtualenv/lib/python2.6/site-packages/mozmill/__init__.py", line 231, in start self.create_network() File "/var/SSD/TB-hg/tbird-bin/mozilla/_tests/mozmill-virtualenv/lib/python2.6/site-packages/mozmill/__init__.py", line 205, in create_network self.jsbridge_port) File "/var/SSD/TB-hg/tbird-bin/mozilla/_tests/mozmill-virtualenv/lib/python2.6/site-packages/jsbridge/__init__.py", line 72, in wait_and_create_network raise Exception("Sorry, cannot connect to jsbridge extension, port %s" % port) Exception: Sorry, cannot connect to jsbridge extension, port 24242 make: *** [mozmill-one] Error 1
Assignee | ||
Comment 28•12 years ago
|
||
But I found the problem through the manual STR and in the Error console. Error: MailServices is not defined Source File: chrome://messenger/content/addressbook/addrbookWidgets.xml Line: 96 I am not sure how to include the mailservices module into this file, it is not pure javascript...
Assignee | ||
Comment 29•12 years ago
|
||
So I could make the mozmill test running properly (to show the failure), but after manual tests this fixes the problem for me.
Attachment #596773 -
Attachment is obsolete: true
Attachment #597527 -
Flags: review?(mconley)
Comment 31•12 years ago
|
||
The code looks good - just going to run these tests locally before giving my r+.
Comment 32•12 years ago
|
||
Comment on attachment 597527 [details] [diff] [review] patch v4 Tests pass. This looks good. Thanks aceman!
Attachment #597527 -
Flags: review?(mconley) → review+
Comment 34•12 years ago
|
||
http://hg.mozilla.org/comm-central/rev/962931e86468
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Comment 35•12 years ago
|
||
Drive by post landing review. 2.14 getSupportedFlavours: function () 2.15 - { 2.16 - return null; 2.17 - } 2.18 + { 2.19 + return null; 2.20 + } 2.21 }; Since you're changing the indentation anyway. You should use the current style guide recommendations. e.g. getSupportedFlavours: function () { return null; } 2.77 - if ( dataObj ) 2.78 + 2.79 + if ( dataObj ) No spaces in the if clause e.g. if (dataObj) 4.10 <constructor> 4.11 <![CDATA[ 4.12 + Components.utils.import("resource:///modules/mailServices.js"); You only need to import this once in the constructor. In addition you can limit the scope. e.g.: <field name="MailServices">null</field> <constructor> Components.utils.import("resource:///modules/mailServices.js", this); .... Then refer to MailServices as |this.MailServices| within the XBL.
Assignee | ||
Comment 36•12 years ago
|
||
It has landed. What now?
Comment 37•12 years ago
|
||
> It has landed. What now?
Nothing. You have gained some knowledge and so you should not make the same mistakes in the next patch.
Assignee | ||
Comment 38•12 years ago
|
||
Well, I do not consider them mistakes, I just followed the rule to preserve the prevailing coding style in the file/function. If I should break the style and introduce new one (maybe better), then the reviewer should say that.
I did not understand this part:
>4.10 <constructor>
>4.11 <![CDATA[
>4.12 + Components.utils.import("resource:///modules /mailServices.js");
>You only need to import this once in the constructor. In addition you can limit the scope. e.g.:
><field name="MailServices">null</field>
><constructor>
> Components.utils.import("resource:///modules/mailServices.js", this);
>....
>Then refer to MailServices as |this.MailServices| within the XBL.
I did it in the constructor. Can you expand on it? It is in the destructor too. Should that one somehow reuse that declaration (as you say)?
Comment 39•12 years ago
|
||
> I just followed the rule to preserve the > prevailing coding style in the file/function. If I should break the style and introduce > new one (maybe better), then the reviewer should say that. The file does not have a prevailing coding style. The older parts have the indentation you preserved. If you scroll down the newer functions added more recently follow the current coding style. The rule of thumb is that with a new file or a file with no prevailing style you should use the current style guide. And since you're reformatting the whole function anyway, you might as well follow the newer style. > Components.utils.import("resource:///modules/mailServices.js"); Without the second parameter you are importing mailServices into the global scope of the window. So importing it again in the destructor is redundant. > Components.utils.import("resource:///modules/mailServices.js", this); The "this" in the above example imports the module into the scope of the binding. So anywhere within the binding (including any of the methods and the destructor) you can refer to the js module using |this.MailServices|. I suggest that any further discussion takes place in mozilla.dev.thunderbird.
Assignee | ||
Comment 40•12 years ago
|
||
I think I get it, thanks.
You need to log in
before you can comment on or make changes to this bug.
Description
•