Last Comment Bug 804004 - Convert rest of Account manager files to Services.jsm and mailServices.js
: Convert rest of Account manager files to Services.jsm and mailServices.js
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 19.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: 720356 720358
  Show dependency treegraph
 
Reported: 2012-10-21 11:28 PDT by :aceman
Modified: 2012-10-30 14:54 PDT (History)
2 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (14.52 KB, patch)
2012-10-21 12:29 PDT, :aceman
iann_bugzilla: review+
Details | Diff | Review
patch v2 (15.20 KB, patch)
2012-10-23 12:06 PDT, :aceman
no flags Details | Diff | Review
patch v3 (14.56 KB, patch)
2012-10-30 12:28 PDT, :aceman
acelists: review+
Details | Diff | Review

Description :aceman 2012-10-21 11:28:11 PDT
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"].
Comment 1 :aceman 2012-10-21 12:29:22 PDT
Created attachment 673722 [details] [diff] [review]
patch

This intentionally skips am-offline.js as that is covered in bug 755885.
Comment 2 Ian Neal 2012-10-23 10:39:58 PDT
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?
Comment 3 :aceman 2012-10-23 12:03:13 PDT
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?
Comment 4 :aceman 2012-10-23 12:06:03 PDT
Created attachment 674311 [details] [diff] [review]
patch v2
Comment 5 Mike Conley (:mconley) - (Needinfo me!) 2012-10-29 19:05:36 PDT
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?
Comment 6 :aceman 2012-10-30 00:25:38 PDT
(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?
Comment 7 :aceman 2012-10-30 12:28:13 PDT
Created attachment 676707 [details] [diff] [review]
patch v3
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-10-30 14:54:17 PDT
https://hg.mozilla.org/comm-central/rev/7863603e2121

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