Closed Bug 852690 Opened 11 years ago Closed 11 years ago

Remaining conversion to mailServices.js in /mail/ and /mailnews/

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 22.0

People

(Reporter: aryx, Assigned: aryx)

References

Details

Attachments

(17 files, 17 obsolete files)

8.49 KB, patch
Details | Diff | Splinter Review
18.86 KB, patch
Details | Diff | Splinter Review
34.00 KB, patch
Details | Diff | Splinter Review
52.57 KB, patch
Details | Diff | Splinter Review
7.89 KB, patch
Details | Diff | Splinter Review
21.86 KB, patch
Details | Diff | Splinter Review
2.56 KB, patch
Details | Diff | Splinter Review
11.51 KB, patch
Details | Diff | Splinter Review
2.01 KB, patch
Details | Diff | Splinter Review
20.22 KB, patch
Details | Diff | Splinter Review
9.91 KB, patch
Details | Diff | Splinter Review
27.45 KB, patch
Details | Diff | Splinter Review
49.83 KB, patch
Details | Diff | Splinter Review
73.27 KB, patch
Details | Diff | Splinter Review
31.03 KB, patch
Details | Diff | Splinter Review
83.61 KB, patch
Details | Diff | Splinter Review
32.99 KB, patch
Details | Diff | Splinter Review
Attached patch accounts, v1 (obsolete) — Splinter Review
Using mailServices.js will clean up the readability of our code a bit, and should avoid the costs the getService call each time we want a service.

Some patches are blocked by bug 824150 and I won't request review for them until their dependencies in that bug got checked in.

There was a successful Try run one month ago, but tbpl doesn't show anymore the results: https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=e150a67bfeff
Attachment #726864 - Flags: review?(mconley)
Attached patch mime, v1 (obsolete) — Splinter Review
Attachment #726875 - Flags: review?(Pidgeot18)
Attached patch news, v1 (obsolete) — Splinter Review
Attachment #726878 - Flags: review?(Pidgeot18)
Attachment #726868 - Flags: review?(florian) → review+
Keywords: checkin-needed
Whiteboard: [leave open][push patch 'im' to comm-central]
Attachment #727202 - Attachment description: im, v2, r=florian → im, v2, r=florian [ready for check-in]
Comment on attachment 727202 [details] [diff] [review]
im, v2, r=florian [checkin: comment 11]

https://hg.mozilla.org/comm-central/rev/8a50d60dd7a0
Attachment #727202 - Attachment description: im, v2, r=florian [ready for check-in] → im, v2, r=florian [checkin: comment 11]
Keywords: checkin-needed
Whiteboard: [leave open][push patch 'im' to comm-central] → [leave open]
Comment on attachment 726880 [details] [diff] [review]
spam filter, v1

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

Looks good, thanks for the patch.
Attachment #726880 - Flags: review?(kent) → review+
Keywords: checkin-needed
Whiteboard: [leave open] → [leave open][push patch 'spam filter' to comm-central]
Comment on attachment 727637 [details] [diff] [review]
spam filter, v2, r=rkent [checkin: comment 14]

https://hg.mozilla.org/comm-central/rev/75432cb9b21a
Attachment #727637 - Attachment description: spam filter, v2, r=rkent [ready for check-in] → spam filter, v2, r=rkent [checkin: comment 14]
Keywords: checkin-needed
Whiteboard: [leave open][push patch 'spam filter' to comm-central] → [leave open]
Comment on attachment 726875 [details] [diff] [review]
mime, v1

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

You are just trying to bitrot all of my patches aren't you?

Then again, I suppose it would be my fault for rewriting all of these tests..
Attachment #726875 - Flags: review?(Pidgeot18) → review+
Comment on attachment 726878 [details] [diff] [review]
news, v1

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

It's too bad that the method mailServices uses to generate their getters can't be used to specify having multiple interfaces.
Attachment #726878 - Flags: review?(Pidgeot18) → review+
Attachment #726869 - Flags: review?(mbanner) → review+
Attachment #726872 - Flags: review?(mbanner) → review+
Attachment #726877 - Flags: review?(mbanner) → review+
Comment on attachment 726864 [details] [diff] [review]
accounts, v1

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

r=me with the following changes. Thanks!

::: mailnews/base/prefs/content/AccountWizard.js
@@ +403,2 @@
>    // Create an account.
> +  var account = MailServices.accounts.createAccount();

let, not var

@@ +411,2 @@
>      // Create an identity.
> +    var identity = MailServices.accounts.createIdentity();

let, not var

@@ +488,5 @@
>         */
>        if (destIdentity.attachSignature)
>        {
>            var sigFileName = accountData.signatureFileName;
> +          var sigFile = MailServices.mailSession.getDataFilesDir("messenger");

let, not var

@@ +497,5 @@
>        if (accountData.smtp.hostname && !destIdentity.smtpServerKey)
>        {
>            // hostname + no key => create a new SMTP server.
>  
> +          var smtpServer = MailServices.smtp.createServer();

let, not var

@@ +619,5 @@
>        copiesAndFoldersServer = server;
>      }
>      else 
>      {
> +      if (!MailServices.accounts.localFoldersServer) 

Please trim trailing whitespace.

@@ +775,5 @@
>      var skipPanelsPrefStr = "mail.identity." + identity.key + ".wizardSkipPanels";
>      accountData.wizardSkipPanels = Services.prefs.getCharPref(skipPanelsPrefStr);
>  
>      if (identity.smtpServerKey) {
> +      var smtpServer = MailServices.smtp.getServerByKey(identity.smtpServerKey);

let, not var

::: mailnews/base/prefs/content/accountcreation/createInBackend.js
@@ +20,2 @@
>    // incoming server
> +  var inServer = MailServices.accounts.createIncomingServer(

let, not var

@@ +121,5 @@
>    }
>  
>    // identity
>    // TODO accounts without identity?
> +  var identity = MailServices.accounts.createIdentity();

let, not var

@@ +139,5 @@
>    // Note: Setting incomingServer will cause the AccountManager to refresh
>    // itself, which could be a problem if we came from it and we haven't set
>    // the identity (see bug 521955), so make sure everything else on the
>    // account is set up before you set the incomingServer.
> +  var account = MailServices.accounts.createAccount();

let, not var

@@ +244,5 @@
>   */
>  function checkOutgoingServerAlreadyExists(config)
>  {
>    assert(config instanceof AccountConfig);
> +  var smtpServers = MailServices.smtp.servers;

let, not var

::: mailnews/base/prefs/content/accountcreation/emailWizard.js
@@ +199,5 @@
>  
>      // Populate SMTP server dropdown with already configured SMTP servers from
>      // other accounts.
>      var menulist = e("outgoing_hostname");
> +    var smtpServers = MailServices.smtp.servers;

let, not var

::: mailnews/base/prefs/content/accountcreation/verifyConfig.js
@@ +49,5 @@
>      return;
>    }
>  
>    // incoming server
>    var inServer =

let, not var
Attachment #726864 - Flags: review?(mconley) → review+
Comment on attachment 726873 [details] [diff] [review]
mdn, v1

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

That was easy. :) r=me.
Attachment #726873 - Flags: review?(mconley) → review+
Comment on attachment 726865 [details] [diff] [review]
compose, v1

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

This looks really good - I'm just concerned that we don't need the gMimeHeaderParser global in addressingWidgetOverlay.js. Can you try to get rid of that in a follow-up?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2273,5 @@
>  
>    // " <>" is an empty identity, and most likely not valid
>    if (!params.identity || params.identity.identityName == " <>") {
>      // no pre selected identity, so use the default account
> +    var identities = MailServices.accounts.defaultAccount.identities;

let, not var

@@ +2729,5 @@
>  
>      // Check if the user tries to send a message to a newsgroup through a mail
>      // account.
>      var currentAccountKey = getCurrentAccountKey();
> +    var account = MailServices.accounts.getAccount(currentAccountKey);

let, not var

@@ +3093,4 @@
>      var emailAddresses = {};
>      var names = {};
>      var fullNames = {};
> +    var numAddresses = MailServices.headerParser.parseHeadersWithArray(aAddressesToAdd, emailAddresses, names, fullNames);

let, not var

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +94,5 @@
>      var reply_Sep = "";
>      var ng_Sep = "";
>      var follow_Sep = "";
>  
> +    gMimeHeaderParser = MailServices.headerParser;

Why do we need this alias?

@@ +157,5 @@
>  
>  function CompFields2Recipients(msgCompFields)
>  {
>    if (msgCompFields) {
> +    gMimeHeaderParser = MailServices.headerParser;

Again, do we need this?

@@ +1035,4 @@
>    var addresses = {};
>    var names = {};
>    var fullNames = {};
> +  var numAddresses = MailServices.headerParser.parseHeadersWithArray(strippedAddresses, addresses, names, fullNames);

let, not var

::: mailnews/compose/test/unit/head_compose.js
@@ +37,2 @@
>    // Set up the identity
> +  var identity = MailServices.accounts.createIdentity();

let, not var

::: mailnews/compose/test/unit/test_bug474774.js
@@ +178,2 @@
>  
> +  var account = MailServices.accounts.createAccount();

let, not var for these two lines.

::: mailnews/compose/test/unit/test_sendBackground.js
@@ +103,2 @@
>  
> +  var account = MailServices.accounts.createAccount();

let, not var for these two.

::: mailnews/compose/test/unit/test_sendMessageLater.js
@@ +185,2 @@
>  
> +  var account = MailServices.accounts.createAccount();

let, not var for these two

::: mailnews/compose/test/unit/test_sendMessageLater2.js
@@ +229,2 @@
>  
> +  var account = MailServices.accounts.createAccount();

let, not var for these two

::: mailnews/compose/test/unit/test_sendMessageLater3.js
@@ +132,2 @@
>  
> +  var account = MailServices.accounts.createAccount();

let, not var for these two

::: mail/test/mozmill/message-header/test-reply-to-list-from-address-selection.js
@@ +68,5 @@
>    return aFolder.msgDatabase.getMsgHdrForMessageID(msgId);
>  }
>  
>  var addIdentitiesAndFolder = function() {
> +  var identity2 = MailServices.accounts.createIdentity();

let, not var for each of these changes.
Attachment #726865 - Flags: review?(mconley) → review+
Keywords: checkin-needed
Whiteboard: [leave open] → [leave open][push to comm-central]
Attachment #729108 - Attachment description: import, v2, r=Standard8 [ready for check-in] → import, v2, r=Standard8 [checkin: comment 28]
Attachment #729109 - Attachment description: /mailnews/test/, v2, r=Standard8 [ready for check-in] → /mailnews/test/, v2, r=Standard8 [checkin: comment 28]
Attachment #729110 - Attachment description: mdn, v2, r=mconley [ready for check-in] → mdn, v2, r=mconley [checkin: comment 28]
Attachment #729111 - Attachment description: mime, v2, r=jcranmer [ready for check-in] → mime, v2, r=jcranmer [checkin: comment 28]
Attachment #729113 - Attachment description: newsblog, v2, r=Standard8 [ready for check-in] → newsblog, v2, r=Standard8 [checkin: comment 28]
Attachment #729116 - Attachment description: news, v2, r=jcranmer [ready for check-in] → news, v2, r=jcranmer [checkin: comment 28]
Attachment #729106 - Attachment description: accounts, v2, r=mconley [ready for check-in] → accounts, v2, r=mconley [checkin: comment 28]
Attachment #729107 - Attachment description: compose, v2, r=mconley [ready for check-in] → compose, v2, r=mconley [checkin: comment 28]
Attachment #730249 - Flags: review?(mconley) → review+
Attachment #730254 - Flags: review?(mbanner) → review+
Attachment #730269 - Flags: review?(mbanner) → review+
Attachment #730283 - Flags: review?(mconley) → review+
Attachment #730258 - Flags: review?(mbanner) → review+
Attachment #730266 - Flags: review?(mbanner) → review+
Attachment #730281 - Flags: review?(mconley) → review+
Keywords: checkin-needed
Whiteboard: [push to comm-central]
Attachment #731092 - Attachment description: addressbook, v3, r=Standard8 → addressbook, v3, r=Standard8 [checkin: comment 43]
Attachment #731093 - Attachment description: gloda, v3, r=Standard8 → gloda, v3, r=Standard8 [checkin: comment 43]
Attachment #731094 - Attachment description: imap, v3, r=Standard8 → imap, v3, r=Standard8 [checkin: comment 43]
Attachment #731095 - Attachment description: internals, v3, r=Standard8 → internals, v3, r=Standard8 [checkin: comment 43]
Attachment #731097 - Attachment description: local, v3, r=Standard8 → local, v3, r=Standard8 [checkin: comment 43]
Attachment #731098 - Attachment description: message views, v3, r=Standard8 → message views, v3, r=Standard8 [checkin: comment 43]
Attachment #731100 - Attachment description: search, v3, r=Standard8 → search, v3, r=Standard8 [checkin: comment 43]
Depends on: 856350
You need to log in before you can comment on or make changes to this bug.