Closed
Bug 821619
Opened 12 years ago
Closed 12 years ago
Switch to mailServices.js in /mail/components/compose
Categories
(Thunderbird :: Message Compose Window, enhancement)
Thunderbird
Message Compose Window
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 852690
People
(Reporter: aryx, Assigned: aryx)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
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
Attachment #692216 -
Flags: review?(mconley)
Comment 1•12 years ago
|
||
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);
Attachment #692216 -
Flags: review?(mconley)
Assignee | ||
Comment 2•12 years ago
|
||
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.
Attachment #696899 -
Flags: review?(mconley)
Comment 4•12 years ago
|
||
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
Attachment #696899 -
Flags: review?(mconley) → review+
Attachment #692216 -
Attachment is obsolete: true
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...
Assignee | ||
Comment 6•12 years ago
|
||
Yes, I got impatient and convert all /mail and /mailnews code in bug 852690.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•