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)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 13.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(1 file, 3 obsolete files)

Convert all files except tests.
Attached patch patch (obsolete) — Splinter Review
Tests (unconverted) seem to pass after these changes.
I can do tests in a follow-up bug.
Attachment #592538 - Flags: review?(mconley)
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-
Attached patch patch v2 (obsolete) — Splinter 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)
Blocks: 726737
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+
Attached patch patch v3 (obsolete) — Splinter 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 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
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
Depends on: 727154
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.
Mike don't forget to reopen the bug when you back things out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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/* :(
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.
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.
(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.
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
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.
Does TB has such -inbound checkin tree (as FF) where patches would be tested and rejected from ever landing if they break tests?
(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.
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
OK, I understand. I do not wish such rights yet.
I'll look into the breakage, you do not need to do anything yet.
I run 'cd $BINDIR/mailnews; make xpcshell-tests'.
Do I need to do the same in /mail to run the other tests?
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
Do I have to run each one individually??
there are other xpcshell tests here:
objdir-tb/mail/base/test

to run the mozmill tests,

cd objdir
make mozmill
you can run the individually, by directory, or all.

here's my cheat sheet:

https://wiki.mozilla.org/User:Bienvenu/Cheat_Sheet
(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).
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
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...
Attached patch patch v4Splinter Review
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)
...I could not...
Status: REOPENED → ASSIGNED
The code looks good - just going to run these tests locally before giving my r+.
Comment on attachment 597527 [details] [diff] [review]
patch v4

Tests pass.  This looks good.

Thanks aceman!
Attachment #597527 - Flags: review?(mconley) → review+
Lets hope it sticks this time :)
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/962931e86468
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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.
It has landed. What now?
> It has landed. What now?
Nothing. You have gained some knowledge and so you should not make the same mistakes in the next patch.
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)?
> 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.
I think I get it, thanks.
You need to log in before you can comment on or make changes to this bug.