821619 - Switch to mailServices.js in /mail/components/compose

Status: RESOLVED DUPLICATE of bug 852690

(Thunderbird :: Message Compose Window, enhancement)

Description

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

Attachment: convert compose code to use mailServices.js, patch v1

mailServices.js: http://mxr.mozilla.org/comm-central/source/mailnews/base/util/mailServices.js?raw=1
Successful try run: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=6de8e20a6189

Comment 1

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

Comment on attachment 692216 [details] [diff] [review]
convert compose code to use mailServices.js, patch v1

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

Just a few nits, but nothing major. Thanks for this!

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2264,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;

While you're here, switch var to let.

@@ +2719,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);

While you're here, switch var to let.

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

Might look better to break this up like this:

let numAddresses =
  MailServices.headerParser.parseHeadersWithArray(aAddressesToAdd,
                                                  emailAddresses,
                                                  names,
                                                  fullNames);

@@ +3090,5 @@
>      var tokenizedNames = new Array();
>  
>      // each name could consist of multiple word delimited by either commas or spaces. i.e. Green Lantern
>      // or Lantern,Green. Tokenize on comma first, then tokenize again on spaces.
> +    for (var name in names.value)

Good catch.

@@ +3320,5 @@
>  }
>  
>  function getIdentityForKey(key)
>  {
> +    return MailServices.accounts.getIdentity(key);

While you're here, reduce this to two spaces indentation.

@@ +4249,5 @@
>              item.flavour.contentType == "application/x-moz-file")
>          {
>            if (item.flavour.contentType == "application/x-moz-file")
>            {
> +            var fileHandler = Services.io

Replace this var with let

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +1033,4 @@
>    var addresses = {};
>    var names = {};
>    var fullNames = {};
> +  var numAddresses = MailServices.headerParser

Replace var with let. Also, try breaking it up like this:

let numAddresses =
  MailServices.headerParser.parseHeadersWithArray(strippedAddresses,
                                                  addresses,
                                                  names,
                                                  fullNames);

Comment 2

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

Attachment: convert compose code to use mailServices.js, patch v2

Addressed the review comments.

Sorry, the last patch didn't contain all patches from the patch queue. Try run: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=a5c332f2232a Non-greens are the same like for other (successful) Try pushes.

Comment 4

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

Comment on attachment 696899 [details] [diff] [review]
convert compose code to use mailServices.js, patch v2

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

This looks pretty good! Glad to see some modern query selectors being used. Good to see the code getting cleaned up.

Just a few more nits, and then it's r=me. Thanks,

-Mike

::: mail/components/compose/content/MsgComposeCommands.js
@@ +332,5 @@
>        // Calculate percentage.
>        var percent;
>        if ( aMaxTotalProgress > 0 )
>        {
> +        percent = Math.min(Math.round( 100 * aCurTotalProgress / aMaxTotalProgress ), 100);

This is fine - just remove the space before 100 and after aMaxTotalProgress.

@@ +2058,5 @@
>      let keywordsInCsv = Services.prefs.getComplexValue(
>        "mail.compose.attachment_reminder_keywords",
>        Components.interfaces.nsIPrefLocalizedString).data;
>      let mailBody = document.getElementById("content-frame")
> +                           .contentDocument.querySelector("body");

I'd prefer:

let mailBody = document.getElementById("content-frame")
                       .contentDocument
                       .querySelector("body");

@@ +2666,5 @@
>      }
>  
>      // Strip trailing spaces and long consecutive WSP sequences from the
>      // subject line to prevent getting only WSP chars on a folded line.
> +    var fixedSubject = subject.replace(/\s{74,}/g, "    ").trimRight();

We prefer let instead of var

@@ +3088,3 @@
>      if (!names)
>        return;
> +    let tokenizedNames = new Array();

[] instead of new Array();.

@@ +3314,5 @@
>  
>  function getCurrentAccountKey()
>  {
> +  // get the accounts key
> +  var identityList = document.getElementById("msgIdentity");

We prefer let over var

@@ +3320,5 @@
>  }
>  
>  function getIdentityForKey(key)
>  {
> +    return MailServices.accounts.getIdentity(key);

Nit: two space indent

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +154,5 @@
>      var listbox = document.getElementById('addressingWidget');
>      var newListBoxNode = listbox.cloneNode(false);
>      var listBoxColsClone = listbox.firstChild.cloneNode(true);
>      newListBoxNode.appendChild(listBoxColsClone);
> +    var templateNode = listbox.querySelector("listitem");

While you're here, we prefer let over var.

@@ +237,5 @@
>  
>      var newNode = templateNode.cloneNode(true);
>      parentNode.appendChild(newNode); // we need to insert the new node before we set the value of the select element!
>  
> +    var input = newNode.querySelector(awInputElementName());

let over var

@@ +282,5 @@
>  
>    var recipientArray = msgCompFields.splitRecipients(recipientsList, false, {});
>  
>    for (var index = 0; index < recipientArray.length; index++)
> +    for (var row = 1; row <= top.MAX_RECIPIENTS; row++)

let instead of var

@@ +311,5 @@
>  // this was broken out of awAddRecipients so it can be re-used...adds a new row matching recipientType and
>  // drops in the single address.
>  function awAddRecipient(recipientType, address)
>  {
> +  for (var row = 1; row <= top.MAX_RECIPIENTS; row++)

let instead of var

@@ +356,5 @@
>    {
>      for (var i = 1; i <= listitems.length; i ++)
>      {
>        var item = listitems [i - 1];
> +      var inputID = item.querySelector(awInputElementName()).getAttribute("id").split("#")[1];

let instead of var for these two

@@ +389,5 @@
>  {
>    var maxRecipients = top.MAX_RECIPIENTS;
>    var rowID = 1;
>  
> +  for (var row = 1; row <= maxRecipients; row++)

let instead of var

@@ +511,5 @@
>        listbox.appendChild(newNode);
>  
>      top.MAX_RECIPIENTS++;
>  
> +    var input = newNode.querySelector(awInputElementName());

let instead of var

@@ +512,5 @@
>  
>      top.MAX_RECIPIENTS++;
>  
> +    var input = newNode.querySelector(awInputElementName());
> +    if ( input )

No extra spaces needed around "input"

@@ +539,2 @@
>      }
> +    var select = newNode.querySelector(awSelectElementName());

let instead of var

@@ +539,3 @@
>      }
> +    var select = newNode.querySelector(awSelectElementName());
> +    if ( select )

No extra spaces needed around "select"

@@ +708,5 @@
>  
>  function DragOverAddressingWidget(event)
>  {
>    var validFlavor = false;
> +  var dragSession = gDragService.getCurrentSession();

let instead of var

@@ +878,5 @@
>      /* Warning, the listboxElement.selectedItems will change everytime we delete a row */
>      var selItems = listboxElement.selectedItems;
>      var length = listboxElement.selectedItems.length;
>      for (var i = 1; i <= length; i++) {
> +      var input = listboxElement.selectedItems[0].querySelector(awInputElementName());

let instead of var

@@ +879,5 @@
>      var selItems = listboxElement.selectedItems;
>      var length = listboxElement.selectedItems.length;
>      for (var i = 1; i <= length; i++) {
> +      var input = listboxElement.selectedItems[0].querySelector(awInputElementName());
> +      if (input )

no space after input

Comment 5

[:aceman]

Aryx, will you finish the nits? However, it looks like some of the changes went in with bug 852690 as I do not see any old mail services in compose code today...

Comment 6

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

Yes, I got impatient and convert all /mail and /mailnews code in bug 852690.