The default bug view has changed. See this FAQ.

Convert rest of Account manager files to Services.jsm and mailServices.js

RESOLVED FIXED in Thunderbird 19.0

Status

MailNews Core
Account Manager
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks: 2 bugs)

Trunk
Thunderbird 19.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

14.56 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
am-addressing.js:    gPrefInt = Components.classes["@mozilla.org/preferences-service;1"]
am-copies.js:var gPrefBranch = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);
am-identity-edit.js:    var accountManager = Components.classes["@mozilla.org/messenger/account-manager;1"]
am-identity-edit.js:    var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
am-identity-edit.js:  var smtpService = Components.classes["@mozilla.org/messengercompose/smtp;1"]
am-offline.js:    var prefService = Components.classes["@mozilla.org/preferences-service;1"];
am-server-advanced.js:    gAccountManager = Components.classes["@mozilla.org/messenger/account-manager;1"].getService(Components.interfaces.nsIMsgAccountManager);
am-server-advanced.js:        Components.classes["@mozilla.org/embedcomp/prompt-service;1"].
am-server.js:  gObserver= Components.classes["@mozilla.org/observer-service;1"].
(Assignee)

Comment 1

5 years ago
Created attachment 673722 [details] [diff] [review]
patch

This intentionally skips am-offline.js as that is covered in bug 755885.
Attachment #673722 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Attachment #673722 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED

Comment 2

5 years ago
Comment on attachment 673722 [details] [diff] [review]
patch

>+++ b/mailnews/base/prefs/content/am-addressing.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
am-addressingOverlay.xul loads both am-addressing.js and amUtils.js.
As amUtils.js already imports Services.jsm you don't strictly need to import it here.

>+++ b/mailnews/base/prefs/content/am-identity-edit.js
>+Components.utils.import("resource:///modules/mailServices.js");
>+Components.utils.import("resource://gre/modules/Services.jsm");
am-identity-edit.xul and am-main.xul both loads both am-prefs.js.
As am-prefs.js already imports Services.jsm you don't strictly need to import it here.

>+++ b/mailnews/base/prefs/content/am-server-advanced.js

>       case "2":
>         picker = document.getElementById("deferedServerFolderPicker");
picker doesn't seem to be used, so you could remove it.

>+++ b/mailnews/base/prefs/content/am-server.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
am-server.xul loads amUtils.js.
As amUtils.js already imports Services.jsm you don't strictly need to import it here.

There appears to be an orphaned gPrefBranch here:
http://mxr.mozilla.org/comm-central/source/mailnews/base/content/msgAccountCentral.js#280

Does that need fixing up the same time as the rest of this?
Attachment #673722 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 3

5 years ago
Technically, Account central is not covered by this bug and the gPrefBranch is defined somewhere else (not in am-*.js). It is still defined even after this patch. But yes, I can replace it for simplicity (so we do not wonder where it comes from).

I'll rather leave the 'not strictly needed imports' for now. The inclusion of amUtils.js may change in the future and then suddenly they will be missing. The imports indicate what modules are used in which file. In the future I may make a pass over all the files and optimize the imports: my idea would be to import them only into AccountManages.js and then call them via top.document.Services or something like that. Would that be better? Or is that slower again?
(Assignee)

Comment 4

5 years ago
Created attachment 674311 [details] [diff] [review]
patch v2
Attachment #673722 - Attachment is obsolete: true
Attachment #673722 - Flags: review?(mconley)
Attachment #674311 - Flags: review?(mconley)
Comment on attachment 674311 [details] [diff] [review]
patch v2

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

This looks pretty good - have you run the accounts tests?

::: mailnews/base/prefs/content/am-addressing.js
@@ +53,3 @@
>    }
>  
> +  if (gIdentity && Services.prefs.prefIsLocked("mail.identity." + gIdentity.key + ".directoryServer")) {

Is this under 80 chars? If not, please break it up.

::: mailnews/base/prefs/content/am-server-advanced.js
@@ +23,5 @@
>  }
>  
>  function getLocalFoldersAccount()
>  {
> +  var localFoldersServer = MailServices.accounts.localFoldersServer;

let, not var

@@ +128,3 @@
>          var server = document.getElementById("deferedServerFolderPicker")
>                               .selectedItem._folder.server;
> +        var account = MailServices.accounts.FindAccountForServer(server);

let, not var

::: mailnews/base/prefs/content/am-server.js
@@ -6,3 @@
>  var gServer;
> -var gObserver;
> -const Ci = Components.interfaces;

Why are you dropping Ci?
(Assignee)

Comment 6

5 years ago
(In reply to Mike Conley (:mconley) from comment #5)
> ::: mailnews/base/prefs/content/am-server.js
> @@ -6,3 @@
> >  var gServer;
> > -var gObserver;
> > -const Ci = Components.interfaces;
> 
> Why are you dropping Ci?

It is unused after the patch. Should I leave it with the few occurences it has?
(Assignee)

Comment 7

5 years ago
Created attachment 676707 [details] [diff] [review]
patch v3
Attachment #674311 - Attachment is obsolete: true
Attachment #674311 - Flags: review?(mconley)
Attachment #676707 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/7863603e2121
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 19.0
You need to log in before you can comment on or make changes to this bug.