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

RESOLVED FIXED in Thunderbird 14.0

Status

Thunderbird
Address Book
--
trivial
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 14.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

46.53 KB, patch
aceman
: review+
Details | Diff | Splinter Review
34.32 KB, image/png
Details
(Assignee)

Description

6 years ago
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"]
(Assignee)

Comment 1

6 years ago
Created attachment 603438 [details] [diff] [review]
patch
Attachment #603438 - Flags: review?(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
Attachment #603438 - Flags: review?(mconley) → review-
(Assignee)

Comment 3

6 years ago
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 :)
(Assignee)

Comment 4

6 years ago
(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

6 years ago
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.
(Assignee)

Comment 6

6 years ago
Created attachment 607726 [details] [diff] [review]
patch v2
Attachment #603438 - Attachment is obsolete: true
Attachment #607726 - Flags: review?(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
Attachment #607726 - Flags: review?(mconley) → review-
(Assignee)

Comment 8

6 years ago
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.
(Assignee)

Comment 9

6 years ago
Created attachment 608857 [details] [diff] [review]
patch v3
Attachment #607726 - Attachment is obsolete: true
Attachment #608857 - Flags: review?(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
Attachment #608857 - Flags: review?(mconley) → review+
(Assignee)

Comment 11

6 years ago
Created attachment 609448 [details] [diff] [review]
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:))
Attachment #608857 - Attachment is obsolete: true
Attachment #609448 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/d3fb8aefb15c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0

Comment 13

5 years ago
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.
(Assignee)

Comment 14

5 years ago
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

5 years ago
Created attachment 640029 [details]
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

5 years ago
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.
(Assignee)

Comment 17

5 years ago
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 archaeopteryx@coole-files.de, I think he may forward you further in AMO matters.
CCing Jorge from AMO and Mark for Thunderbird. I am not sure who is responsible for these tests.
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.
(we also mentioned about the false positives in the AMO email that we sent out).

Comment 21

5 years ago
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

Updated

5 years ago
Blocks: 774069
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.