733496 - convert mail/components/addrbook/content to Services.jsm

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[:aceman]

abCardOverlay.js:        Components.classes["@mozilla.org/abmanager;1"]
abCardOverlay.js:      .classes["@mozilla.org/embedcomp/prompt-service;1"]
abCardOverlay.js:      .classes["@mozilla.org/embedcomp/prompt-service;1"]
abCardOverlay.js:      var file = Components.classes["@mozilla.org/network/io-service;1"]
abCardOverlay.js:      var value = Components.classes["@mozilla.org/network/io-service;1"]
abCardOverlay.js:    var photoURI = Components.classes["@mozilla.org/network/io-service;1"]
abCardViewOverlay.js:var gPrefs = Components.classes["@mozilla.org/preferences-service;1"];
abCardViewOverlay.js:var gIOService = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService);
abCardViewOverlay.js:        var dirService = Components.classes["@mozilla.org/file/directory_service;1"]
abCommon.js:var gPrefs = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);
abCommon.js:var gHeaderParser = Components.classes["@mozilla.org/messenger/headerparser;1"].getService(Components.interfaces.nsIMsgHeaderParser);
abCommon.js:  var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService);
abCommon.js:  var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService);
abCommon.js:  var msgComposeService = Components.classes["@mozilla.org/messengercompose;1"].getService();
abCommon.js:              Components.classes["@mozilla.org/network/io-service;1"]
abCommon.js:  var file = Components.classes["@mozilla.org/file/directory_service;1"]
abCommon.js:  return Components.classes["@mozilla.org/network/io-service;1"]
abCommon.js:  var ios = Components.classes["@mozilla.org/network/io-service;1"]
abTrees.js:      let file = Cc["@mozilla.org/file/directory_service;1"]
abTrees.js:      let file = Cc["@mozilla.org/file/directory_service;1"]
addressbook.js:  Components.classes["@mozilla.org/messenger/services/session;1"]
addressbook.js:  Components.classes["@mozilla.org/messenger/services/session;1"]
addressbook.js:    var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService);
addressbook.js:  var windowManager = Components.classes['@mozilla.org/appshell/window-mediator;1'].
addressbook.js:  var prefSvc = Components.classes["@mozilla.org/preferences-service;1"]

Comment 1

[:aceman]

Attachment: patch

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 603438 [details] [diff] [review]
patch

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

This looks pretty good - just a few small nits.

Also, does this break any of our address book tests?

-Mike

::: mail/components/addrbook/content/abCardViewOverlay.js
@@ +42,5 @@
>  //NOTE: gAddressBookBundle must be defined and set or this Overlay won't work
>  
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +var gPrefs = Services.prefs;

Is there a reason to stick with gPrefs, or can we just use Services.prefs throughout this file?

::: mail/components/addrbook/content/abCommon.js
@@ +49,5 @@
>  var gAbResultsTree = null;
>  var gAbView = null;
>  var gAddressBookBundle;
>  
> +var gPrefs = Services.prefs;

Same as above re: gPrefs.  Can we just use Services.prefs instead of aliasing it?

@@ +692,5 @@
>          // fill in the session params if there is a session
>          //
>          if (LDAPSession) {
>              let url =
>                gPrefs.getComplexValue(autocompleteDirectory +".uri",

While you're here - please put a space after "+".

@@ +986,5 @@
>    // Get the photos directory and check that it exists
>    var file = getPhotosDir();
>  
>    // Create a channel from the URI and open it as an input stream
> +  var channel = Services.io.newChannelFromURI(Services.io.newURI(aUri, null, null));

let instead of var

Comment 3

[:aceman]

As discussed in other bugs, I can run xpcshell tests and they pass after this. I am not sure how to handle mozmill tests so can't say for those. I mean they run but I don't know where the results are :)

Comment 4

[:aceman]

(In reply to Mike Conley (:mconley) from comment #2)
> Same as above re: gPrefs.  Can we just use Services.prefs instead of
> aliasing it?

The only reason is that it is shorter and I do not need to touch and reindent many lines.
I just understand it that the most gain is from removing Components.classes["@mozilla.org/preferences-service;1"] into Services.prefs, removing the variables may not gain that much. I'd think having alias would even be faster (no getting prefs object from Services every time) but I am told otherwise.
So should I remove the gPrefs?

Comment 5

[Magnus Melin [:mkmelin]]

You run mozmill from the command line, whey make mozmill is done it shows staistics of what what was successful (if successful at least...) - see https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing

And yes, please remove gPrefs, the problem with global variables is you need to know they happen to be defined in the context you are using them. The code is way more self contained without them.

Comment 6

[:aceman]

Attachment: patch v2

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 607726 [details] [diff] [review]
patch v2

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

Hm - two things:

1)  I just tried this patch, and the address book UI doesn't seem to load my address books...not sure why, nothing in my Error Console.
2)  Just a few style nits wrt indentation. Nothing major.

So, due to (1), giving this an r-.  Can you figure out what's wrong?

-Mike

::: mail/components/addrbook/content/abCommon.js
@@ +691,5 @@
>          //
>          if (LDAPSession) {
>              let url =
> +              Services.prefs.getComplexValue(autocompleteDirectory + ".uri",
> +                                     Components.interfaces.nsISupportsString).data;

It seems that, throughout the rest of the file, when it comes to long lines like this, we indent the second line two (or four...we're not really consistent in here it seems) spaces.  Maybe do that with line 695.  Or, alternatively, alias Components.interfaces.nsISupportsString to something shorter.

@@ +750,1 @@
>                                        ".autoComplete.nameFormat",

Same as above, re: indentation of lines 750 and 751.

@@ +762,1 @@
>                                        ".autoComplete.addressFormat",

Same as above, re: indentation for lines 762 and 763.

@@ +781,5 @@
>  
>                  case 1:
>                      // use the name of this directory
>                      //
> +                    ldapFormatter.commentFormat = Services.prefs.getComplexValue(

Same as above, re: indentation for lines 786, 787.

@@ +796,1 @@
>                                          ".autoComplete.commentFormat",

Same as above, re: indentation for lines 796, 797

@@ +826,1 @@
>                                        ".autoComplete.outputFormat",

Same as above, re: indentation for lines 826, 827

Comment 8

[:aceman]

It seems /suite has a copy of some of these files. I can't patch suite too but maybe they will be interested in applying something from the patch.

Comment 9

[:aceman]

Attachment: patch v3

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 608857 [details] [diff] [review]
patch v3

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

Hey - just one formatting nit (see below).

I also notice some formatting issues in function setupLdapAutocompleteSession(), where it looks like we're using 4 spaces instead of 2.  Bleh.  I won't ask you fix that here though.

Otherwise, this looks great.  Modulo the nit I found, r=me.

Thanks so much for doing this, and for your patience for my glacial-speed reviews.

-Mike

::: mail/components/addrbook/content/addressbook.js
@@ +508,1 @@
>                                                Components.interfaces.nsIPrefLocalizedString).data;

Fix alignment of this line

Comment 11

[:aceman]

Attachment: patch v4

There are more places where the indentation is inconsistent. You could setup a bug for it like bug 654933 (and assign it to me:))

Comment 12

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/d3fb8aefb15c

Comment 13

[Arivald]

I have problem. Because of this bug, code to automatically test extension was changed. Now it detects legit "loadHTMLMsgPrefs();" function as invalid, and block automatic version bump. 

Anyone know how to contact people responsible for this code? 

Thanks in advance.

Comment 14

[:aceman]

Which bug do you mean? This "bug" is only an code update to newer infrastructure and should not change any behaviour. If you have found some change, then I am the person responsible for the new code (in the patch).

Can you be more specific what problems you get? What code detects the function as invalid? Is the problem in the addressbook or Thunderbird?

Comment 15

[Arivald]

Attachment: AMO error

"bug" means this bugzilla entity, with number 733496.

I have problem with AMO automatic tests for extensions. Someone use regex pattern "(g|cv)(Prefs|IOService|HeaderParser)" to find all extensions that use JS objectsd removed by this bug. 
Problem is it also find my calls to "loadHTMLMsgPrefs();", and report it as error.

Details here, on attached image.

Comment 16

[Arivald]

My problem is that I do not know who make AMO automated tests, how I could contact them. Google did not help.
Filling new bugzilla bug is not best solution, it could reach developers tomorrow, or in month or in years. Or never. But i need solution fast, best if before TB 14 release.

BTW, You may reply to my email, there is no need to pollute bugzilla.

Comment 17

[:aceman]

I think I understand now. It is true this bug here removes some gPrefs global variables that extensions may have been using (but only if they touch the AB files in question). I do not know who may be doing hat regex on AMO. It seems it matches your function because it does contain 'gPrefs' (the regex does not seem to only match from begging of identifier). So if you can't contact the person, maybe the quickest hack would be to rename your function. Try finding the guy [e-mail], I think he may forward you further in AMO matters.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

CCing Jorge from AMO and Mark for Thunderbird. I am not sure who is responsible for these tests.

Comment 19

[Mark Banner (:standard8)]

Arivald: Despite the AMO automated tests failing, you should still be able to bump your add-on max version manually.

We were aware there were a few false positives, but decided to run it anyway given the late run for this release. We will be correcting the automated tests next week and re-running the bump, so if you haven't manually updated by then, you'll be automatically updated.

Comment 20

[Mark Banner (:standard8)]

(we also mentioned about the false positives in the AMO email that we sent out).

Comment 21

[Arivald]

Thanks You, fixing test was my concern.
Yes, I updated version manually to 14, proven to work without problems (my personal TB is in beta channel). Still it is better if this happen automatically. 
EOT