Last Comment Bug 722187 - convert mailnews/addrbook/ to Services.jsm and MailServices.js
: convert mailnews/addrbook/ to Services.jsm and MailServices.js
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Address Book (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 13.0
Assigned To: :aceman
:
Mentors:
Depends on: 727154
Blocks: 720356 720358 714604 726737
  Show dependency treegraph
 
Reported: 2012-01-29 13:06 PST by :aceman
Modified: 2012-03-03 12:36 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (28.32 KB, patch)
2012-01-29 13:30 PST, :aceman
mconley: review-
Details | Diff | Review
patch v2 (32.06 KB, patch)
2012-02-13 12:23 PST, :aceman
mconley: review+
Details | Diff | Review
patch v3 (32.10 KB, patch)
2012-02-13 13:18 PST, :aceman
mconley: review+
Details | Diff | Review
patch v4 (32.29 KB, patch)
2012-02-15 12:37 PST, :aceman
mconley: review+
Details | Diff | Review

Description :aceman 2012-01-29 13:06:18 PST
Convert all files except tests.
Comment 1 :aceman 2012-01-29 13:30:36 PST
Created attachment 592538 [details] [diff] [review]
patch

Tests (unconverted) seem to pass after these changes.
I can do tests in a follow-up bug.
Comment 2 Mike Conley (:mconley) - (Needinfo me!) 2012-02-13 09:06:54 PST
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.
Comment 3 :aceman 2012-02-13 12:23:58 PST
Created attachment 596747 [details] [diff] [review]
patch v2

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.
Comment 4 Mike Conley (:mconley) - (Needinfo me!) 2012-02-13 12:41:35 PST
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.
Comment 5 :aceman 2012-02-13 13:18:26 PST
Created attachment 596773 [details] [diff] [review]
patch v3

You are right, there is the same iteration over directories in mailnews/addrbook/content/addrbookWidgets.xml .
Comment 6 Mike Conley (:mconley) - (Needinfo me!) 2012-02-13 13:23:19 PST
Comment on attachment 596773 [details] [diff] [review]
patch v3

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

Looks good - thanks for your work,

-Mike
Comment 7 Mark Banner (:standard8) 2012-02-14 04:39:38 PST
Checked in: http://hg.mozilla.org/comm-central/rev/59943cb43b16
Comment 8 Mike Conley (:mconley) - (Needinfo me!) 2012-02-14 10:53:54 PST
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?
Comment 9 :aceman 2012-02-14 11:01:08 PST
Sorry about that.
I was sure no test was failing for me. Will check again.
Comment 10 Ludovic Hirlimann [:Usul] 2012-02-15 04:51:03 PST
Mike don't forget to reopen the bug when you back things out.
Comment 11 :aceman 2012-02-15 06:23:18 PST
(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 Mike Conley (:mconley) - (Needinfo me!) 2012-02-15 06:30:22 PST
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.
Comment 13 :aceman 2012-02-15 07:00:08 PST
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 David :Bienvenu 2012-02-15 07:29:46 PST
(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 Mike Conley (:mconley) - (Needinfo me!) 2012-02-15 07:31:42 PST
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
Comment 16 :aceman 2012-02-15 07:38:58 PST
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.
Comment 17 :aceman 2012-02-15 07:40:01 PST
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 Ludovic Hirlimann [:Usul] 2012-02-15 07:45:58 PST
(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 Mike Conley (:mconley) - (Needinfo me!) 2012-02-15 07:56:58 PST
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
Comment 20 :aceman 2012-02-15 08:01:03 PST
OK, I understand. I do not wish such rights yet.
I'll look into the breakage, you do not need to do anything yet.
Comment 21 :aceman 2012-02-15 11:30:03 PST
I run 'cd $BINDIR/mailnews; make xpcshell-tests'.
Do I need to do the same in /mail to run the other tests?
Comment 22 Mike Conley (:mconley) - (Needinfo me!) 2012-02-15 11:31:29 PST
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
Comment 23 :aceman 2012-02-15 11:37:51 PST
Do I have to run each one individually??
Comment 24 David :Bienvenu 2012-02-15 11:47:13 PST
there are other xpcshell tests here:
objdir-tb/mail/base/test

to run the mozmill tests,

cd objdir
make mozmill
Comment 25 David :Bienvenu 2012-02-15 11:48:14 PST
you can run the individually, by directory, or all.

here's my cheat sheet:

https://wiki.mozilla.org/User:Bienvenu/Cheat_Sheet
Comment 26 :aceman 2012-02-15 11:58:44 PST
(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).
Comment 27 :aceman 2012-02-15 12:00:14 PST
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
Comment 28 :aceman 2012-02-15 12:01:14 PST
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...
Comment 29 :aceman 2012-02-15 12:37:16 PST
Created attachment 597527 [details] [diff] [review]
patch v4

So I could make the mozmill test running properly (to show the failure), but after manual tests this fixes the problem for me.
Comment 30 :aceman 2012-02-16 14:31:57 PST
...I could not...
Comment 31 Mike Conley (:mconley) - (Needinfo me!) 2012-03-02 13:05:19 PST
The code looks good - just going to run these tests locally before giving my r+.
Comment 32 Mike Conley (:mconley) - (Needinfo me!) 2012-03-02 13:38:34 PST
Comment on attachment 597527 [details] [diff] [review]
patch v4

Tests pass.  This looks good.

Thanks aceman!
Comment 33 :aceman 2012-03-02 13:50:04 PST
Lets hope it sticks this time :)
Comment 34 Ryan VanderMeulen [:RyanVM] 2012-03-02 14:21:08 PST
http://hg.mozilla.org/comm-central/rev/962931e86468
Comment 35 Philip Chee 2012-03-02 21:08:43 PST
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.
Comment 36 :aceman 2012-03-03 05:45:19 PST
It has landed. What now?
Comment 37 Philip Chee 2012-03-03 08:35:48 PST
> It has landed. What now?
Nothing. You have gained some knowledge and so you should not make the same mistakes in the next patch.
Comment 38 :aceman 2012-03-03 08:54:38 PST
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 Philip Chee 2012-03-03 12:31:03 PST
> 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.
Comment 40 :aceman 2012-03-03 12:36:50 PST
I think I get it, thanks.

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