Closed Bug 1478572 Opened 2 years ago Closed 2 years ago

Turn on ESLint in mail/components

Categories

(Thunderbird :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 64.0

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(16 files, 25 obsolete files)

17.16 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
147.39 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
46.97 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
7.02 KB, patch
aceman
: review+
Details | Diff | Splinter Review
2.91 KB, patch
aceman
: review+
Details | Diff | Splinter Review
125.84 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
2.55 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
180.18 KB, patch
aceman
: review+
Details | Diff | Splinter Review
67.29 KB, patch
aceman
: review+
Details | Diff | Splinter Review
47.69 KB, patch
aceman
: review+
Details | Diff | Splinter Review
112.39 KB, patch
aceman
: review+
Details | Diff | Splinter Review
289.74 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
166.36 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
73.30 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
62.39 KB, patch
darktrojan
: review+
jorgk-bmo
: feedback+
Details | Diff | Splinter Review
31.85 KB, patch
darktrojan
: review+
aceman
: feedback+
Details | Diff | Splinter Review
I'm going to work on this when I have a small amount of spare time that isn't big enough to do other things in. I'll do one patch per directory, or more I guess if it's a big one.
Assignee: nobody → geoff
Status: NEW → ASSIGNED
This is the same patch, but only the bits that weren't done by |mach eslint --fix|.
Comment on attachment 8995087 [details] [diff] [review]
eslint-mail-components-aboutsupport

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

::: mail/components/about-support/aboutSupportUnix.js
@@ -61,3 @@
>          ctypes.size_t.ptr, // out: bytes_read
>          ctypes.size_t.ptr, // out: bytes_written
> -        GError.ptr         // out: error

Destroying these comments seems undesired.
Attachment #8995087 - Attachment is obsolete: true
Attachment #8995088 - Flags: review?(jorgk)
Again, only the things changed by humans.
Attachment #8995111 - Flags: review?(jorgk)
Comment on attachment 8995088 [details] [diff] [review]
eslint-mail-components-aboutsupport-diff.diff

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

Looks OK.

::: b/mail/components/about-support/content/aboutSupport.js
@@ +294,4 @@
>      ));
>    },
>  
> +  /* eslint-disable complexity */

What happened here?

::: b/mail/components/about-support/content/export.js
@@ +119,5 @@
>        node.remove();
>        node = nextNode;
>        continue;
> +    } else if (aHidePrivateData && classList && classList.contains(CLASS_DATA_PRIVATE)) {
> +      // Replace private data with a blank string

Nit: Since you're touching these comments, you could add the missing full stop at the end.
Attachment #8995088 - Flags: review?(jorgk) → review+
Comment on attachment 8995111 [details] [diff] [review]
eslint-mail-components-accountcreation-diff.diff

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

AM is aceman territory and he's also more a fan of linting.

::: b/mail/components/accountcreation/content/emailWizard.js
@@ +74,4 @@
>  **********************/
>  
>  // To debug, set mail.wizard.logging.dump (or .console)="All" and kDebug = true
> +const kDebug = false;

Where is that used? Not in this file. Oh, appears to be an export.

@@ -536,5 @@
>      this.startSpinner("looking_up_settings_disk");
>      var self = this;
>      this._abortable = fetchConfigFromDisk(domain,
> -      function(config) // success
> -      {

If we disallow this style
  if (x)
  {
we'll cause an awful lot of churn.

::: b/mail/components/accountcreation/content/verifyConfig.js
@@ +107,3 @@
>    } catch (e) {
>      gEmailWizardLogger.error("ERROR: verify logon shouldn't have failed");
> +    // Avoid pref pollution, clear out server prefs.

Why did this move to the catch block?

::: mail/components/accountcreation/content/.eslintrc.js
@@ +37,5 @@
> +    "isLegalIPAddress": true,
> +    "isLegalIPv4Address": true,
> +    "isLegalIPv6Address": true,
> +    "isLegalLocalIPAddress": true,
> +    "kDebug": true,

Why true?
Attachment #8995111 - Flags: review?(jorgk) → review?(acelists)
(In reply to Jorg K (GMT+2) from comment #7)
> What happened here?

The function is too complicated. There is (if I remember right) 37 possible paths through the function. Our rule says 34 is the maximum, with a comment saying it should be reduced to 20. I'm not opposed to having the rule, as it will stop monster functions like this from being created in the future, but I don't think it's worth trying to fix the ones we have. In fact, I think I'll go mark the others I've come across like this instead of disabling the rule for the whole directory.

https://eslint.org/docs/rules/complexity
(In reply to Jorg K (GMT+2) from comment #8)
> If we disallow this style
>   if (x)
>   {
> we'll cause an awful lot of churn.

Yes, but we can limit the churn to one revision per file, and then it is gone forever because we have a rule to prevent it.

> ::: b/mail/components/accountcreation/content/verifyConfig.js
> @@ +107,3 @@
> >    } catch (e) {
> >      gEmailWizardLogger.error("ERROR: verify logon shouldn't have failed");
> > +    // Avoid pref pollution, clear out server prefs.
> 
> Why did this move to the catch block?

Urgh, ESLint complained about using e outside the catch block. I looked sideways at this for a while before realising the try block had an early return. This whole situation was a bit weird.

> Why true?

True means the global exists. False means the global exists and is read-only. (I just learned that.) I've just assumed all the globals were writeable since most of them exist because of scripts in this directory (and I didn't know there was another option). Doesn't really matter, IMO.
Changed to remove the blanked disabling of the complexity rule.
Attachment #8995110 - Attachment is obsolete: true
Attachment #8995111 - Attachment is obsolete: true
Attachment #8995111 - Flags: review?(acelists)
Attachment #8995142 - Flags: review?(acelists)
Comments fixed, ready to land.
Attachment #8995092 - Attachment is obsolete: true
Attachment #8995088 - Attachment is obsolete: true
Comment on attachment 8995142 [details] [diff] [review]
eslint-mail-components-accountcreation-diff.diff - r+

You need to add !mail/components/accountcreation to .eslintignore or we don't get any of the new goodness ;-)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fe8684d0f25a
Turn on ESLint in mail/components/about-support. r=jorgk
Keywords: checkin-needed
You're created more work for yourself ;-)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/common/bindings/preferences.xml:647:30 | Multiple spaces found before '='. (no-multi-spaces)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/common/bindings/preferences.xml:1106:40 | Multiple spaces found before '='. (no-multi-spaces)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/common/bindings/preferences.xml:1107:40 | Multiple spaces found before '='. (no-multi-spaces)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/common/src/viewSource.js:19:25 | Multiple spaces found before '"content"'. (no-multi-spaces)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/common/src/viewSource.js:21:25 | Multiple spaces found before '"viewSourceContextMenu"'. (no-multi-spaces)
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/4b5191f432bc
Fix eslint bustage by re-adding missing exceptions; a=bustage DONTBUILD
Comment on attachment 8995142 [details] [diff] [review]
eslint-mail-components-accountcreation-diff.diff - r+

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

This part is actually the field of BenB, but I like the fixes ESLint has suggested.
Thanks.

Would you be able to finish bug 1426882 ?

::: b/mail/components/accountcreation/content/guessConfig.js
@@ -985,4 @@
>                           .getService(Ci.nsISocketTransportService);
>  
>    // @see NS_NETWORK_SOCKET_CONTRACTID_PREFIX
> -  var socketTypeName = ssl == SSL ? "ssl" : (ssl == TLS ? "starttls" : null);

Is this flagged by ESLint? But yes, it is quite unreadable without brackets.

::: b/mail/components/accountcreation/content/util.js
@@ +112,4 @@
>  
>  function Exception(msg) {
>    this._message = msg;
> +  this.stack = Components.stack;

I don't know what this does.

::: b/mail/components/accountcreation/content/verifyConfig.js
@@ +245,1 @@
>          this.mConfig.outgoing.auth = this.mConfig.incoming.auth;

This would be a candidate for {} to bind it to the if() visually, or at least a blank line after this.
Attachment #8995142 - Flags: review?(acelists) → review+
(In reply to :aceman from comment #17)
> ::: b/mail/components/accountcreation/content/guessConfig.js
> @@ -985,4 @@
> >                           .getService(Ci.nsISocketTransportService);
> >  
> >    // @see NS_NETWORK_SOCKET_CONTRACTID_PREFIX
> > -  var socketTypeName = ssl == SSL ? "ssl" : (ssl == TLS ? "starttls" : null);
> 
> Is this flagged by ESLint? But yes, it is quite unreadable without brackets.

Yes it is. Nested ternary are forbidden.

> ::: b/mail/components/accountcreation/content/util.js
> @@ +112,4 @@
> >  
> >  function Exception(msg) {
> >    this._message = msg;
> > +  this.stack = Components.stack;
> 
> I don't know what this does.

I actually did this wrong. The code is expecting a string representation of the stack here, which is Components.stack.formattedStack. It's the same as throwing an exception and getting the stack from the exception object.

> ::: b/mail/components/accountcreation/content/verifyConfig.js
> @@ +245,1 @@
> >          this.mConfig.outgoing.auth = this.mConfig.incoming.auth;
> 
> This would be a candidate for {} to bind it to the if() visually, or at
> least a blank line after this.

Fixed.
Attachment #8995142 - Attachment is obsolete: true
Attachment #8995845 - Flags: review+
I wanted to interdiff the account changes and sent interdiff to la-la land comparing a 22KB patch with a 162KB patch :-( - In the future, or even here, can we please keep automatic changes and manual changes apart. That has been standard Calendar practise for a while. Formally, even automatic changes need a review, so some second pair of eyes should at least glance over them. For the "about support" patch it wasn't an issue since the entire patch only had 17KB.

Geoff, do you still have the separate patches? Now looking at the merged patch, I find it unfortunate that we need to change
/*************** and
//////////////// to
/** ************* and
// //////////////

I don't think this improves the code quality, in fact, it looks uglier than before. Can we give this another round?
I never had separate patches, which after a few times back and forth, became a really dumb idea.

This is the changes made by eslint --fix.
Attached patch eslint-accountcreation-1478572 (obsolete) — Splinter Review
… and here's the manual bits again. I've removed the ugly comment decorations and used two blank lines above and one below to make them stand out. Also removed other instances of two or more blank lines in those files.
Attachment #8995845 - Attachment is obsolete: true
Attachment #8995919 - Attachment is patch: true
Attachment #8995142 - Attachment description: eslint-mail-components-accountcreation-diff.diff → eslint-mail-components-accountcreation-diff.diff - r+
Attachment #8995144 - Attachment description: eslint-mail-components-aboutsupport.diff → eslint-mail-components-aboutsupport.diff [landed in comment #14]
Comment on attachment 8995911 [details] [diff] [review]
eslint-accountcreation-auto-1478572 [landed in comment #24]

I've looked through those. I decent commit message would be appreciated, also a file extension for Windows users.
Attachment #8995911 - Flags: review+
Tweaked the comments a bit. I hope
  // --------
  // Do stuff
is acceptable.
Attachment #8995919 - Attachment is obsolete: true
Attachment #8996052 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/dbabbfef78c1
Turn on ESLint in mail/components/accountcreation (automatic changes). r=jorgk
https://hg.mozilla.org/comm-central/rev/f536b94ef285
Turn on ESLint in mail/components/accountcreation (manual changes). r=aceman
Keywords: checkin-needed
Before I forget to ask again: How do you run linting locally?
(In reply to Jorg K (GMT+2) from comment #25)
> Before I forget to ask again: How do you run linting locally?

../mach eslint (from comm/)
Attachment #8995911 - Attachment description: eslint-accountcreation-auto-1478572 → eslint-accountcreation-auto-1478572 [landed in comment #24]
Attachment #8996052 - Attachment description: eslint-accountcreation-1478572.patch → eslint-accountcreation-1478572.patch [landed in comment #24]
This one should go a little easier. :)
Attachment #8996607 - Flags: review?(acelists)
So should this. Just some semicolons and a comment.
Attachment #8996611 - Flags: review?(acelists)
Comment on attachment 8996607 [details] [diff] [review]
1478572-eslint-devtools-1.diff [landed in comment #31]

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

Thanks :)
Attachment #8996607 - Flags: review?(acelists) → review+
Comment on attachment 8996611 [details] [diff] [review]
1478572-eslint-downloads-1.diff [landed in comment #31]

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

Nice
Attachment #8996611 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/970e484cd962
Turn on ESLint in mail/components/devtools. r=aceman
https://hg.mozilla.org/comm-central/rev/98f869c1bcd5
Turn on ESLint in mail/components/downloads. r=aceman DONTBUILD
Attachment #8996607 - Attachment description: 1478572-eslint-devtools-1.diff → 1478572-eslint-devtools-1.diff [landed in comment #31]
Attachment #8996611 - Attachment description: 1478572-eslint-downloads-1.diff → 1478572-eslint-downloads-1.diff [landed in comment #31]
Comment on attachment 9003725 [details] [diff] [review]
1478572-eslint-addrbook-auto-1.diff

Bit of a monster, I'm afraid, but it's mostly whitespace. This patch was created by mach eslint --fix.
Attachment #9003725 - Flags: review?(acelists)
Attachment #9003726 - Attachment is obsolete: true
Attachment #9004125 - Flags: review?(acelists)
Comment on attachment 9003725 [details] [diff] [review]
1478572-eslint-addrbook-auto-1.diff

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

Thanks, nice.

::: mail/components/addrbook/content/abCard.js
@@ +110,1 @@
>          gEditCard.selectedAB = window.arguments[0].selectedAB;

I think the else branch should have {} too here. Unless eslint complains about that.

@@ +1090,5 @@
>      if (!elPhotoType)
>        elPhotoType = document.getElementById("PhotoType");
>      if (!elProgressContainer)
>        elProgressContainer = document.getElementById("ProgressContainer");
> +  });

Why this change?

::: mail/components/addrbook/content/abCardView.js
@@ +166,5 @@
>    if (addressList) {
>      var total = addressList.length;
>      if (total > 0)
>        addresses = addressList.queryElementAt(0, Ci.nsIAbCard).primaryEmail;
> +    for (var i = 1; i < total; i++ ) {

I would use 'let' here and kill the space after ++.

@@ +293,1 @@
>        dateStr = year;

{} around the branch.

@@ +474,5 @@
>        if (total > 0) {
>          while (node.hasChildNodes()) {
>            node.lastChild.remove();
>          }
> +        for (i = 0; i < total; i++ ) {

Where is 'i' declared? Also the space after ++.

@@ +502,1 @@
>    if ( visible )

The spaces :)

@@ +551,4 @@
>    var type = aCard.getProperty("PhotoType", "");
>    if (!gPhotoDisplayHandlers[type] ||
>        !gPhotoDisplayHandlers[type](aCard, aImg))
> +    gPhotoDisplayHandlers.generic(aCard, aImg);

If you revert this change, will eslint complain? Yes it did it itself, but whether it will flag it as problem. The lines above also use gPhotoDisplayHandlers[] so I would find it clearer to use gPhotoDisplayHandlers[] here too.

@@ +570,5 @@
>   * The following display handlers are for the generic, file and
>   * web photo types.
>   */
>  
> +var genericPhotoDisplayHandler = function(aCard, aImg) {

Could these be changed to normal functions (function genericPhotoDisplayHandler(aCard, aImg)) ? 
Or would the later statement of "registerPhotoDisplayHandler("generic", genericPhotoDisplayHandler);" not work then?

::: mail/components/addrbook/content/abCommon.js
@@ +612,5 @@
>  
>    // if the user clicks on the header / trecol, do nothing
>    if (event.originalTarget.localName == "treecol") {
>      event.stopPropagation();
> +

I would keep the return. Maybe there want's to be more code after the function in the future :)

@@ +747,1 @@
>      email = card.primaryEmail;

{} for the branch

@@ +993,2 @@
>                  callbackError(ERROR_UNAVAILABLE);
>                }

The indentation is now wrong.

@@ +1001,5 @@
>      };
>  
>      let source;
>      try {
> +      source = Services.io.newURI(aURI);

How does it know the arguments are optional and default to null?

::: mail/components/addrbook/content/abTrees.js
@@ +223,5 @@
>    // nsIAbListener interfaces
>    onItemAdded: function dtv_onItemAdded(aParent, aItem) {
>      if (!(aItem instanceof Ci.nsIAbDirectory))
>        return;
> +    // xxx we can optimize this later

Use XXX while there :)

@@ +241,5 @@
>  
>    onItemRemoved: function dtv_onItemRemoved(aParent, aItem) {
>      if (!(aItem instanceof Ci.nsIAbDirectory))
>        return;
> +    // xxx we can optimize this later

Use XXX while there :)

@@ +267,5 @@
>    onItemPropertyChanged: function dtv_onItemProp(aItem, aProp, aOld, aNew) {
>      if (!(aItem instanceof Ci.nsIAbDirectory))
>        return;
>  
> +    for (var i in this._rowMap) {

'let' while touching it.

::: mail/components/addrbook/content/addressbook.js
@@ +261,3 @@
>    var prefValue;
>  
> +  switch ( cmd ) {

no spaces

@@ +577,5 @@
>        if (cardViewBoxEmail1)
>          cardViewBoxEmail1.focus();
>        else
>          cardViewBox.focus();
> +    } else if (focusedElement != dirTree && !IsDirPaneCollapsed())

Now the elses do not align vertically. Please add {} for other branches if at least one has them.
Attachment #9003725 - Flags: review?(acelists) → review+
Comment on attachment 9004125 [details] [diff] [review]
1478572-eslint-addrbook-manual-2.diff

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

Thanks :)

::: mail/components/addrbook/content/abCard.js
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
> +var { MailServices } = ChromeUtils.import("resource:///modules/mailServices.js", null);

Why is this better?

@@ +747,5 @@
>      // get the difference between today and the age (the year is offset by 1970)
>      var difference = new Date(today - date);
>      datepicker.year = yearElem.value = difference.getFullYear() - 1970;
> +  } catch (e) {
> +    // the above code may throw an invalid year exception.  If that happens, set

Kill the double space. Add leading capital and trailing dot.

@@ +807,5 @@
>      }
>      var max = (aField == this.monthField) ? 11 : kMaxYear;
>      // make sure the value isn't too high
>      if (aValue > max)
> +      return aNoWrap ? max : aValue - max;

Why?

::: mail/components/addrbook/content/abCardView.js
@@ +475,5 @@
>        if (total > 0) {
>          while (node.hasChildNodes()) {
>            node.lastChild.remove();
>          }
> +        for (let i = 0; i < total; i++) {

Aha, you already fixed this here;)

::: mail/components/addrbook/content/abCommon.js
@@ +648,5 @@
>    let uri = getSelectedDirectoryURI();
>    // clear out the search box when changing folders...
>    onAbClearSearch(false);
>    if (gDirTree && gDirTree.view.selection && gDirTree.view.selection.count == 1) {
> +    gPreviousDirTreeIndex = gDirTree.currentIndex; // eslint-disable-line no-native-reassign

What's the problem here?

@@ +716,5 @@
>  function setSortByMenuItemCheckState(id, value) {
> +  var menuitem = document.getElementById(id);
> +  if (menuitem) {
> +    menuitem.setAttribute("checked", value);
> +  }

Here the {} could go.

::: mail/components/addrbook/content/addressbook.js
@@ +433,2 @@
>  
> +  AbExport(selectedDirURI);

Why these changes? I would rather we make those functions return useful success values (true/false).

@@ +770,5 @@
>    // I can't think of a better way though.
>  
>    // See if the pref exists, if so, then we need to delete the address book
> +  var uriPresent = Preferences.get(kOSXPrefBase + ".uri", null) == kOSXDirectoryURI;
> +  var position = Preferences.get(kOSXPrefBase + ".position", 1);

Default of 1?
Attachment #9004125 - Flags: review?(acelists) → review+
Comment on attachment 9004125 [details] [diff] [review]
1478572-eslint-addrbook-manual-2.diff

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

::: mail/components/addrbook/content/abCard.js
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
> +var { MailServices } = ChromeUtils.import("resource:///modules/mailServices.js", null);

We need to be explicit that we have an object called MailServices and not one called mailServices, which eslint assumes because that's what the module's name is.

That's what bug 1480393 was about. In time we'll probably end up doing the same for all other imports.

@@ +807,5 @@
>      }
>      var max = (aField == this.monthField) ? 11 : kMaxYear;
>      // make sure the value isn't too high
>      if (aValue > max)
> +      return aNoWrap ? max : aValue - max;

I think I did this wrong, now that I've looked again at where this code came from.

::: mail/components/addrbook/content/abCommon.js
@@ +648,5 @@
>    let uri = getSelectedDirectoryURI();
>    // clear out the search box when changing folders...
>    onAbClearSearch(false);
>    if (gDirTree && gDirTree.view.selection && gDirTree.view.selection.count == 1) {
> +    gPreviousDirTreeIndex = gDirTree.currentIndex; // eslint-disable-line no-native-reassign

gPreviousDirTreeIndex is a global, it shouldn't be redefined here.

::: mail/components/addrbook/content/addressbook.js
@@ +433,2 @@
>  
> +  AbExport(selectedDirURI);

The problem is that some branches could return a value (actually AbExportAll returns nothing, but eslint ignores that) and some don't.

We could make these functions return a success or failure result but I think that's outside the scope of this bug.

@@ +770,5 @@
>    // I can't think of a better way though.
>  
>    // See if the pref exists, if so, then we need to delete the address book
> +  var uriPresent = Preferences.get(kOSXPrefBase + ".uri", null) == kOSXDirectoryURI;
> +  var position = Preferences.get(kOSXPrefBase + ".position", 1);

I don't know why, that's what the old code did.
Comment on attachment 9003725 [details] [diff] [review]
1478572-eslint-addrbook-auto-1.diff

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

::: mail/components/addrbook/content/abCard.js
@@ +1090,5 @@
>      if (!elPhotoType)
>        elPhotoType = document.getElementById("PhotoType");
>      if (!elProgressContainer)
>        elProgressContainer = document.getElementById("ProgressContainer");
> +  });

It's in addEventListener, which doesn't need the third argument if it's false.

::: mail/components/addrbook/content/abCardView.js
@@ +551,4 @@
>    var type = aCard.getProperty("PhotoType", "");
>    if (!gPhotoDisplayHandlers[type] ||
>        !gPhotoDisplayHandlers[type](aCard, aImg))
> +    gPhotoDisplayHandlers.generic(aCard, aImg);

Yes it will.

::: mail/components/addrbook/content/abCommon.js
@@ +1001,5 @@
>      };
>  
>      let source;
>      try {
> +      source = Services.io.newURI(aURI);

https://searchfox.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-useless-parameters.js#105
Just the changes in response to the review. I didn't realise that replying to review comments using the review tool didn't copy the original message, but they should be readable by going back to the review tool. :-/
Attachment #9004167 - Flags: review?(acelists)
Comment on attachment 9004167 [details] [diff] [review]
1478572-eslint-addrbook-manual-3.diff

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

Thanks.
Attachment #9004167 - Flags: review?(acelists) → review+
Gear down, flaps set, prepare for landing. :)
Attachment #9003725 - Attachment is obsolete: true
Attachment #9004125 - Attachment is obsolete: true
Attachment #9004167 - Attachment is obsolete: true
Attachment #9004169 - Flags: review+
Attachment #9004169 - Attachment description: 1478572-eslint-addrbook-4.diff [landed in comment #41] → 1478572-eslint-addrbook-4.diff [landed in comment #42]
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/dc872eb2bea0
Turn on ESLint in mail/components/addrbook; r=aceman
Why did you merge the two patches? Now it's hard to see who made the mistakes :-(

Indentation wrong here:
https://hg.mozilla.org/comm-central/rev/dc872eb2bea0#l5.233
+  } else if (Services.prefs.getCharPref("mail.collect_addressbook") == aURI &&
+      Services.prefs.getBoolPref("mail.collect_email_address_outgoing")) {
Not sure why you removed this comment: // It's an address book: check which type.

Unless I'm missing something, this is wrong:
https://hg.mozilla.org/comm-central/rev/dc872eb2bea0#l3.395
-    preferDisplayNameEl.checked = cardproperty.getProperty("PreferDisplayName", true) != false;
+    preferDisplayNameEl.checked = !cardproperty.getProperty("PreferDisplayName", true);
That last change isn't in any patch that was reviewed :-(

I'm still puzzled by this code:
https://hg.mozilla.org/comm-central/rev/dc872eb2bea0#l3.648
+    var min = (aField == this.monthField) ? 0 : 1;
     var max = (aField == this.monthField) ? 11 : kMaxYear;
     // make sure the value isn't too high
     if (aValue > max)
       return aNoWrap ? max : min;
     return aValue;

The original in datetimepicker.xml also has:
            if (aValue < min)
              return aNoWrap ? min : max;

Also, this appears to be a bug since min wasn't defined. So perhaps we should backport that fix, no?
Flags: needinfo?(geoff)
I've done it for you so you have more time for bug 1486051 ;-)

(In reply to Jorg K (GMT+2) from comment #44)
> Not sure why you removed this comment: // It's an address book: check which
> type.
That comment doesn't fit anywhere now, agreed.

> The original in datetimepicker.xml also has:
>             if (aValue < min)
>               return aNoWrap ? min : max;
Apparently not needed according to the preceding comment:
  // this modified constrain value function ignores values less than the minimum
  // to let the value be blank (null)

So I just fixed the logic error and the indentation.
Flags: needinfo?(geoff)
Attachment #9004334 - Flags: review?(geoff)
Comment on attachment 9004334 [details] [diff] [review]
1478572-addressbook-follow-up.patch [landed in comment 49]

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

If you start nit-picking indentation matters you'll be here a long time. :-)
Attachment #9004334 - Flags: review?(geoff) → review+
Right, but there was a logic error as well which led to the checkbox not being shown correctly :-(
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a03add67bee9
Turn on ESLint in mail/components/addrbook, follow-up: fix logic error. r=darktrojan
(In reply to Jorg K (GMT+2) from comment #44)
> Also, this appears to be a bug since min wasn't defined. So perhaps we
> should backport that fix, no?

https://hg.mozilla.org/releases/comm-esr60/rev/4e33c625292efe1a110ecffbe0cb8dc39e1c4510
I had some spare time so I took on mail/components/preferences. It's a bit of a monster, >1000 errors.
There's a lot of changes I've made because I've already modified the lines in question. I think it should land as one patch again. Probably best to have both patches open at once when reviewing.

As for dsn.js/xul, these files aren't referenced anywhere, even in the build, so I removed them.
Attachment #9004334 - Attachment description: 1478572-addressbook-follow-up.patch → 1478572-addressbook-follow-up.patch [landed in comment 49]
I thought I'd dealt with the new comma-dangle rule, but apparently I had not.
Attachment #9005913 - Attachment is obsolete: true
Attachment #9005915 - Attachment is obsolete: true
Updated because of bug 1488445.
Attachment #9005955 - Attachment is obsolete: true
Comment on attachment 9005954 [details] [diff] [review]
1478572-eslint-preferences-auto-2.diff [landed in comment #63]

See comment 51.
Attachment #9005954 - Flags: review?(acelists)
Attachment #9006432 - Flags: review?(acelists)
Comment on attachment 9005954 [details] [diff] [review]
1478572-eslint-preferences-auto-2.diff [landed in comment #63]

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

Thanks.

::: mail/components/preferences/actionsshared.js
@@ +12,4 @@
>  }
>  FileAction.prototype = {
> +  type: "",
> +  extension: "",

Why this? Does eslint not like these aligned initializations?

@@ +23,5 @@
> +  customHandler: "",
> +  handleMode: false,
> +  pluginAvailable: false,
> +  pluginEnabled: false,
> +  handledOnlyByPlugin: false,

Does it want the comma after last member?

::: mail/components/preferences/applications.js
@@ +2,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +//* ***************************************************************************//

Really, is there a rule for this (number of stars or the space) ?

@@ +841,1 @@
>                      .getService(Ci.nsIMIMEService),

These have now lost the vertical alignment.

::: mail/components/preferences/attachmentReminder.js
@@ +22,5 @@
>      if (!keywordsInCsv)
>        return;
>      var keywordsInCsv = keywordsInCsv.data;
>      var keywordsInArr = keywordsInCsv.split(",");
> +    for (var i = 0; i < keywordsInArr.length; i++) {

Let.

::: mail/components/preferences/chat.js
@@ +18,1 @@
>        broadcaster.setAttribute("disabled", "true");

{} around the branch.

::: mail/components/preferences/compose.js
@@ +164,2 @@
>        var localFonts = enumerator.EnumerateAllFonts(localFontCount);
> +      for (var i = 0; i < localFonts.length; ++i) {

let

::: mail/components/preferences/cookies.js
@@ +52,1 @@
>          this.filter();

This needs {} and reindenting now.

@@ +118,1 @@
>            rowIndex += hostItem.cookies.length;

{}

@@ +200,5 @@
>          if (count == aIndex)
>            return currHost;
>          hostIndex = count;
>  
> +        var cacheEntry = { "start": i, "count": count };

I wonder why it didn't make this just '{ start: i, count };' as it did in other places. Do the quotes have any different meaning?

@@ +228,1 @@
>            ++count;

{}

@@ +290,3 @@
>            return this._filterSet[aIndex].rawHost;
>          else if (aColumn.id == "nameCol")
>            return ("name" in this._filterSet[aIndex]) ? this._filterSet[aIndex].name : "";

This looks weird now.

@@ +467,1 @@
>          break;

{}

::: mail/components/preferences/offline.js
@@ +17,1 @@
>          offlineStartupStatePref.value = kRememberLastState;

{} now

::: mail/components/preferences/preferencesTab.js
@@ +51,5 @@
>      gPrefTab = null;
>    },
>  
>    openTab: function onTabOpened(aTab, aArgs) {
> +    if (!("contentPage" in aArgs)) {

Does this change (fix) behaviour) ?

::: mail/components/preferences/security.js
@@ +72,1 @@
>         window.opener && typeof(window.opener.ReloadMessage) == "function")

Reindent now?

@@ +72,2 @@
>         window.opener && typeof(window.opener.ReloadMessage) == "function")
>        window.opener.ReloadMessage();

Maybe this would deserve {} now.

@@ +146,5 @@
>      gSubDialog.open("chrome://passwordmgr/content/passwordManager.xul");
>    },
>  
> +  updateDownloadedPhishingListState() {
> +    document.getElementById("useDownloadedList").disabled = !document.getElementById("enablePhishingDetector").checked;

Probably break this line at the = ?

::: mail/components/preferences/sendoptions.js
@@ +26,5 @@
>      var listbox = aHTML ? this.mHTMLListBox : this.mPlainTextListBox;
>      var num_domains = 0;
>      var pref_string = "";
>  
> +    for (var item = listbox.firstChild; item != null; item = item.nextSibling) {

let

@@ +48,1 @@
>      if (arrayOfPrefs)

This if() could use {} around the block.
Attachment #9005954 - Flags: review?(acelists) → review+
Comment on attachment 9006432 [details] [diff] [review]
1478572-eslint-preferences-manual-3.diff

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

::: mail/components/preferences/advanced.js
@@ +80,3 @@
>        let shellSvc = Cc["@mozilla.org/mail/shell-service;1"]
>                         .getService(Ci.nsIShellService);
> +      /* eslint-enable no-unused-vars */

You could kill the variable instead if it isn't used. But the getting of the service is needed to get the exception if the service does not exist.

@@ +217,5 @@
>  
>      actualSizeLabel.value = prefStrBundle.getString("actualDiskCacheSizeCalculated");
>  
>      try {
> +      Services.cache2.asyncGetDiskConsumption(this.observer);

Nice.

@@ +263,5 @@
>      } catch (ex) {}
>      this.updateActualCacheSize();
>    },
>  
>    updateButtons(aButtonID, aPreferenceID) {

Is this function actually used?

::: mail/components/preferences/cookies.js
@@ +196,5 @@
>          count = hostIndex = cacheItem.count;
>        }
>  
> +      for (let i = start; i < gCookiesWindow._hostOrder.length; ++i) {
> +        var currHost = gCookiesWindow._hosts[gCookiesWindow._hostOrder[i]];

'let' when you are touching this.

@@ +210,5 @@
>            if (count < aIndex && aIndex <= (count + currHost.cookies.length)) {
>              // We are looking for an entry within this host's children,
>              // enumerate them looking for the index.
>              ++count;
> +            for (let j = 0; j < currHost.cookies.length; ++j) {

Interesting. Did this even work?

@@ +215,2 @@
>                if (count == aIndex) {
> +                var cookie = currHost.cookies[j];

'let' when you are touching this.

@@ +234,1 @@
>            this._cacheItems[j] = cacheEntry;

Add {} for the loop even when oneliner.

@@ +287,2 @@
>            return "";
> +        }

Are the new {} needed?

@@ +674,5 @@
>      if (Services.prefs.prefHasUserValue("network.cookie.blockFutureCookies"))
>        blockFutureCookies = Services.prefs
>          .getBoolPref("network.cookie.blockFutureCookies");
>      for (i = 0; i < deleteItems.length; ++i) {
> +      let item = deleteItems[i];

Can this just be 'for (let item of deleteItems)' ?

@@ +789,5 @@
>    _filterCookies(aFilterValue) {
>      this._view._filterValue = aFilterValue;
>      var cookies = [];
> +    for (let i = 0; i < gCookiesWindow._hostOrder.length; ++i) {
> +      var currHost = gCookiesWindow._hosts[gCookiesWindow._hostOrder[i]];

'let' when touching it.

::: mail/components/preferences/dsn.js
@@ -1,1 @@
> -# -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 4 -*-

Why is all dns.* removed?

::: mail/components/preferences/messagestyle.js
@@ -55,5 @@
>      previewObserver.conv = conv;
>  
>      let themeName = document.getElementById("messagestyle-themename");
> -    if (themeName.value && !themeName.selectedItem)
> -      themeName.value = themeName.value;

May this be a typo (wanted to be something else on one side of =)? Can you track the history of this line? See https://hg.mozilla.org/comm-central/annotate/91f8f791b3f3/im/content/instantbird/preferences/messagestyle.js#l163 . It may be intentional to initialize the menulist when there is no selection in it.

::: mail/components/preferences/permissions.js
@@ +377,5 @@
>    },
>  
>    _loadPermissions() {
>      this._tree = document.getElementById("permissionsTree");
>      this._permissions = [];

This wants to be 'this._permissions.length = 0;'

::: mail/components/preferences/sendoptions.js
@@ +43,5 @@
>    },
>  
>    loadDomains(aPrefString, aListBox) {
>      var arrayOfPrefs = aPrefString.split(",");
>      if (arrayOfPrefs)

What does split() return if the string is empty? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/split says it is "an array containing one empty string". Is this check then correct? Also it could use {}.
Attachment #9006432 - Flags: feedback+
Attachment #9006432 - Attachment is obsolete: true
Attachment #9006432 - Flags: review?(acelists)
Attachment #9006727 - Flags: review?(acelists)
I'm going to stop posting these as separate patches. It's wasting time for both of us.

(In reply to :aceman from comment #57)
> Why this? Does eslint not like these aligned initializations?

Correct. I don't like them either, but that's personal.

> Does it want the comma after last member?

As of a few days ago, yes.

> > +        var cacheEntry = { "start": i, "count": count };
> 
> I wonder why it didn't make this just '{ start: i, count };' as it did in
> other places. Do the quotes have any different meaning?

Yeah, that's odd.

> > +    if (!("contentPage" in aArgs)) {
> 
> Does this change (fix) behaviour) ?

I don't *think* this changes anything, but given that tried to figure it out and I'm still not sure, it's probably best to be explicit.

(In reply to :aceman from comment #58)
> >    updateButtons(aButtonID, aPreferenceID) {
> 
> Is this function actually used?

Apparently not.

> ::: mail/components/preferences/dsn.js
> @@ -1,1 @@
> > -# -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> 
> Why is all dns.* removed?

I already explained this in comment 51.

> > -    if (themeName.value && !themeName.selectedItem)
> > -      themeName.value = themeName.value;
> 
> May this be a typo (wanted to be something else on one side of =)? Can you
> track the history of this line? See
> https://hg.mozilla.org/comm-central/annotate/91f8f791b3f3/im/content/
> instantbird/preferences/messagestyle.js#l163 . It may be intentional to
> initialize the menulist when there is no selection in it.

If it did something once it doesn't now. Perhaps it was to initialise the preview properly, but that works fine.

> >      this._permissions = [];
> 
> This wants to be 'this._permissions.length = 0;'

I'm not here to fix code I'm not already touching. I haven't got forever. :-)

> >    loadDomains(aPrefString, aListBox) {
> >      var arrayOfPrefs = aPrefString.split(",");
> >      if (arrayOfPrefs)
> 
> What does split() return if the string is empty?
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/String/split says it is "an array containing one empty
> string". Is this check then correct? Also it could use {}.

Yeah, that's never going to be false.
(In reply to Geoff Lankow (:darktrojan) from comment #60)
> I'm going to stop posting these as separate patches. It's wasting time for
> both of us.

Yes that will be better.
Comment on attachment 9006727 [details] [diff] [review]
1478572-eslint-preferences-manual-4.diff [landed in comment #63]

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

Thanks
Attachment #9006727 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0cac668a566b
Turn on ESLint in mail/components/preferences (automatic changes). r=aceman
https://hg.mozilla.org/comm-central/rev/2a29ee0adb31
Turn on ESLint in mail/components/preferences (manual changes). r=aceman
Keywords: checkin-needed
Comment on attachment 9006727 [details] [diff] [review]
1478572-eslint-preferences-manual-4.diff [landed in comment #63]

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

::: mail/components/preferences/applications.js
@@ -1406,5 @@
>        menu.selectedItem = askMenuItem;
>      else switch (handlerInfo.preferredAction) {
> -      case Ci.nsIHandlerInfo.handleInternally:
> -        menu.selectedItem = internalMenuItem;
> -        break;

Why is it OK to just remove the defective code here. Sure, 'internalMenuItem' is not defined.

However, FF and SM do something else:
https://searchfox.org/comm-central/search?q=internalMenuItem&case=false&regexp=false&path=
The reviewer didn't ask the question, so I'm asking ;-)
Flags: needinfo?(geoff)
That case can never be hit. It'd only be handleInternally if we'd set it that way earlier, using a menuitem we don't have. Besides, Thunderbird doesn't do the action in question. It looks like I'm not the first person to come across this: https://searchfox.org/comm-central/source/mail/components/preferences/applications.js#1173
Flags: needinfo?(geoff)
Attachment #9005954 - Attachment description: 1478572-eslint-preferences-auto-2.diff → 1478572-eslint-preferences-auto-2.diff [landed in comment #63]
Attachment #9006727 - Attachment description: 1478572-eslint-preferences-manual-4.diff → 1478572-eslint-preferences-manual-4.diff [landed in comment #63]
The number of folders we don't lint is now smaller than the ones we do lint, so I've flipped the exception list and linted all the stray files from around the place.
Attachment #9009076 - Flags: review?(acelists)
Comment on attachment 9009076 [details] [diff] [review]
1478572-eslint-mail-components-1.diff

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

Nothing exiting here. Nice to revert the list. Am I missing something or is there room for improvement in nsMailDefaultHandler.js. I checked some of the variables and most were used once or not at all.

::: mail/components/nsMailDefaultHandler.js
@@ +8,4 @@
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  ChromeUtils.import("resource:///modules/MailServices.jsm");
>  
>  var nsISupports              = Ci.nsISupports;

Unused.

@@ +11,5 @@
>  var nsISupports              = Ci.nsISupports;
>  
>  var nsICommandLine           = Ci.nsICommandLine;
>  var nsICommandLineHandler    = Ci.nsICommandLineHandler;
>  var nsICommandLineValidator  = Ci.nsICommandLineValidator;

Used once.

@@ +12,5 @@
>  
>  var nsICommandLine           = Ci.nsICommandLine;
>  var nsICommandLineHandler    = Ci.nsICommandLineHandler;
>  var nsICommandLineValidator  = Ci.nsICommandLineValidator;
> +var nsIDOMWindow             = Ci.nsIDOMWindow;

Use once.

@@ +17,1 @@
>  var nsIFactory               = Ci.nsIFactory;

Use once.

@@ +17,3 @@
>  var nsIFactory               = Ci.nsIFactory;
>  var nsIFileURL               = Ci.nsIFileURL;
>  var nsINetUtil               = Ci.nsINetUtil;

Used once.

@@ +17,5 @@
>  var nsIFactory               = Ci.nsIFactory;
>  var nsIFileURL               = Ci.nsIFileURL;
>  var nsINetUtil               = Ci.nsINetUtil;
>  var nsISupportsString        = Ci.nsISupportsString;
>  var nsIURILoader             = Ci.nsIURILoader;

Unused.
Comment on attachment 9009076 [details] [diff] [review]
1478572-eslint-mail-components-1.diff

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

Great, thanks for doing this cleanup.

::: mail/components/migration/content/migration.js
@@ +68,5 @@
>      this._wiz.canAdvance = false;
>  
>      // Figure out what source apps are are available to import from:
>      var group = document.getElementById("importSourceGroup");
> +    for (let i = 0; i < group.childNodes.length; ++i) {

for..of could work here as we do not need 'i' inside the loop.

@@ +81,5 @@
>        }
>      }
>  
>      var firstNonDisabled = null;
> +    for (let i = 0; i < group.childNodes.length; ++i) {

for..of could work here as we do not need 'i' inside the loop.

::: mail/components/nsMailDefaultHandler.js
@@ +317,3 @@
>            // If feed handling is not installed, do nothing
>          }
> +      } else if (uri.toLowerCase().endsWith(".mozeml") || uri.toLowerCase().endsWith(".wdseml")) {

Is this line too long ?

::: mail/components/wintaskbar/windowsJumpLists.js
@@ +6,5 @@
>  var EXPORTED_SYMBOLS = ["WinTaskbarJumpList"];
>  
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
> +const { toXPCOMArray } = ChromeUtils.import("resource:///modules/iteratorUtils.jsm", null);

If somebody uses fixIterator in this file, will he need to add it to this line?
Why wasn't this change needed in other files using iteratorUtils.jsm ?
Attachment #9009076 - Flags: review?(acelists) → review+
(In reply to Jorg K (GMT+2) from comment #68)
> ::: mail/components/nsMailDefaultHandler.js
> @@ +8,4 @@
> >  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> >  ChromeUtils.import("resource:///modules/MailServices.jsm");
> >  
> >  var nsISupports              = Ci.nsISupports;
> 
> Unused.

Yeah, these aren't as useful these days, now that Ci. is globally available.

Also the NS_ERROR_ABORT isn't used instead of Cr.NS_ERROR_ABORT thoughout the file.

So I agree with maybe dropping all of them, in this bug or another one.
Round 2.

(In reply to :aceman from comment #69)
> ::: mail/components/wintaskbar/windowsJumpLists.js
> @@ +6,5 @@
> >  var EXPORTED_SYMBOLS = ["WinTaskbarJumpList"];
> >  
> >  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> >  ChromeUtils.import("resource://gre/modules/Services.jsm");
> > +const { toXPCOMArray } = ChromeUtils.import("resource:///modules/iteratorUtils.jsm", null);
> 
> If somebody uses fixIterator in this file, will he need to add it to this
> line?
> Why wasn't this change needed in other files using iteratorUtils.jsm ?

That's correct. There's only one other use of iteratorUtils still in files I've done so far, … and I did it the lazy way. Fixed that here too.
Attachment #9009076 - Attachment is obsolete: true
Attachment #9009289 - Flags: review?(acelists)
Comment on attachment 9009289 [details] [diff] [review]
1478572-eslint-mail-components-2.diff landed in comment #74]

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

::: mail/components/nsMailDefaultHandler.js
@@ +301,4 @@
>            // If feed handling is not installed, do nothing
>          }
> +      } else if (uri.toLowerCase().endsWith(".mozeml") ||
> +          uri.toLowerCase().endsWith(".wdseml")) {

I was told not to mention indentation. I can fix this when landing.
Comment on attachment 9009289 [details] [diff] [review]
1478572-eslint-mail-components-2.diff landed in comment #74]

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

Thanks.
Attachment #9009289 - Flags: review?(acelists) → review+
https://hg.mozilla.org/comm-central/rev/09e76a937c343d2762a740fc5ba6a0529fabb008
Turn on ESLint by default in mail/components with some exceptions. r=aceman DONTBUILD
Attachment #9009289 - Attachment description: 1478572-eslint-mail-components-2.diff → 1478572-eslint-mail-components-2.diff landed in comment #74]
Attached patch 1478572-eslint-activity-1.diff (obsolete) — Splinter Review
I haven't been well this week so I thought I'd do something light… apparently this was not it.

Go on, complain that I've removed a bunch of state-the-obvious comments. :-)
Attachment #9010191 - Flags: review?(acelists)
Comment on attachment 9010191 [details] [diff] [review]
1478572-eslint-activity-1.diff

I've looked through the entire thing, no complaints. So why wasn't it light?
Attachment #9010191 - Flags: feedback+
Comment on attachment 9010191 [details] [diff] [review]
1478572-eslint-activity-1.diff

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

Looks fine, thank you.

::: mail/components/activity/content/activity.xml
@@ +178,5 @@
>        <property name="isOrphan">
>          <getter>
>          <![CDATA[
>            try {
> +           let activityManager = Cc["@mozilla.org/activity-manager;1"]

Did it find that activityManager is undefined here? Shouldn't it be in some global property (field) of the binding?

::: mail/components/activity/modules/activityModules.jsm
@@ +18,2 @@
>  autosyncModule.init();
> +const { alertHook } = ChromeUtils.import("resource:///modules/activity/alertHook.jsm", null);

Why doesn't it infer "alertHook" automatically? It matches the name of the file.

::: mail/components/activity/nsActivityManagerUI.js
@@ +8,2 @@
>  var ACTIVITY_MANAGER_URL = "chrome://messenger/content/activity.xul";
>  var PREF_FLASH_COUNT = "messenger.activity.manager.flashCount";

Constants and they are not even 'const' :)
Attachment #9010191 - Flags: review?(acelists) → review+
I made a few changes, you might want to check them.

(In reply to :aceman from comment #77)
> ::: mail/components/activity/modules/activityModules.jsm
> @@ +18,2 @@
> >  autosyncModule.init();
> > +const { alertHook } = ChromeUtils.import("resource:///modules/activity/alertHook.jsm", null);
> 
> Why doesn't it infer "alertHook" automatically? It matches the name of the
> file.

Because I just did the same thing to all the lines here? Why is this one named differently though? Who knows?! :-)
Attachment #9010191 - Attachment is obsolete: true
Attachment #9010469 - Flags: review?(acelists)
Comment on attachment 9010469 [details] [diff] [review]
1478572-eslint-activity-2.diff [landed in comment #80]

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

Thanks
Attachment #9010469 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9567ebfd9783
Turn on ESLint in mail/components/activity. r=aceman
Keywords: checkin-needed
Attached patch 1478572-eslint-compose-1.diff (obsolete) — Splinter Review
Warning: monster patch!

A few things of note here:

* There's a few big blocks where the indentation needs fixing. That can be done before landing, as it makes the diff even bigger and hides actual changes.
* I've broken up some nested ternary operations and some crazy if conditions. Please check I've done it right.
Attachment #9011330 - Flags: review?(jorgk)
Attachment #9010469 - Attachment description: 1478572-eslint-activity-2.diff → 1478572-eslint-activity-2.diff [landed in comment #80]
Attached patch 1478572-eslint-compose-2.diff (obsolete) — Splinter Review
Okay, who was the smart person who wrote this?!

> var nsIPlaintextEditorMail = Ci.nsIPlaintextEditor;
Attachment #9011330 - Attachment is obsolete: true
Attachment #9011330 - Flags: review?(jorgk)
Attachment #9011339 - Flags: review?(jorgk)
Comment on attachment 9011339 [details] [diff] [review]
1478572-eslint-compose-2.diff

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

I've checked it all, including the ternary and 'if' replacements (one 'if' case only).
Only some comments. not sure whether Aceman wants to see it.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1738,1 @@
>        listener.onStopRequest(null, null, ex.result);

Let's hope it never got here with the variable undefined previously :-(

@@ +2896,5 @@
>      gAutoSaveTimeout = setTimeout(AutoSave, gAutoSaveInterval);
>  
>    gAutoSaveKickedIn = false;
>  }
> +/* eslint-enable complexity */

Where did that one start? Al 2485? 500 complex lines? The entire function, yes?

@@ +3160,5 @@
>                              getComposeBundle().getString("attachmentReminderMsg"),
>                              flags,
>                              getComposeBundle().getString("attachmentReminderFalseAlarm"),
>                              getComposeBundle().getString("attachmentReminderYesIForgot"),
> +                            null, null, {value: 0});

Nit: bad indent (I can fix).

@@ +3322,5 @@
>    }
>    if (gMsgCompose && originalCharset != gMsgCompose.compFields.characterSet)
>      SetDocumentCharacterSet(gMsgCompose.compFields.characterSet);
>  }
> +/* eslint-enable complexity */

Where did that start? 3082?

@@ +5588,5 @@
> +        Services.prompt.BUTTON_POS_1 * Services.prompt.BUTTON_TITLE_CANCEL +
> +        Services.prompt.BUTTON_POS_1_DEFAULT,
> +        null, null, null,
> +        getComposeBundle().getString("customizeFromAddressIgnore"),
> +        check) != 0)

Nit: I'd revert that indentation change.

@@ +5980,2 @@
>      let SaveDlgTitle = getComposeBundle().getString("SaveDialogTitle");
> +    let bundle = getComposeBundle();

You can move that one line up and used it in:
  let SaveDlgTitle =

@@ +5986,3 @@
>      Services.prompt
>              .alertCheck(window, SaveDlgTitle, dlgMsg,
>                          getComposeBundle().getString("CheckMsg"), checkbox);

use 'bundle'.

@@ +6473,5 @@
> +         gComposeType == Ci.nsIMsgCompType.Draft ||
> +         gComposeType == Ci.nsIMsgCompType.Template ||
> +         gComposeType == Ci.nsIMsgCompType.EditTemplate ||
> +         gComposeType == Ci.nsIMsgCompType.EditAsNew ||
> +         gComposeType == Ci.nsIMsgCompType.MailToUrl))))

Nit: bad indent.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ -3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -ChromeUtils.import("resource://gre/modules/Services.jsm");
> -ChromeUtils.import("resource:///modules/MailServices.jsm");

Not needed?

@@ -19,5 @@
>  
>  var test_addresses_sequence = false;
>  
> -try {
> -  if (sPrefs)

What was 'sPrefs' meant to be?
Attachment #9011339 - Flags: review?(jorgk) → review+
Attachment #9011339 - Flags: review?(acelists)
Comment on attachment 9011339 [details] [diff] [review]
1478572-eslint-compose-2.diff

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

Thank you.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4508,5 @@
>                        getComposeBundle().getString("attachPageDlogTitle"),
>                        getComposeBundle().getString("attachPageDlogMessage"),
>                        result,
>                        null,
> +                      {value: 0})) {

How ugly ;)

@@ +5360,3 @@
>      if (iid.equals(Ci.nsIDOMWindow)) {
>        return window;
>      } else if (iid.equals(Ci.nsIDocShell)) {

I think this can be split, without the 'else'.

@@ +5587,5 @@
> +        Services.prompt.BUTTON_POS_0 * Services.prompt.BUTTON_TITLE_OK +
> +        Services.prompt.BUTTON_POS_1 * Services.prompt.BUTTON_TITLE_CANCEL +
> +        Services.prompt.BUTTON_POS_1_DEFAULT,
> +        null, null, null,
> +        getComposeBundle().getString("customizeFromAddressIgnore"),

This time you didn't pull out the getComposeBundle().

@@ +6098,5 @@
>    let msgAttachmentElement = GetMsgAttachmentElement();
>    let abContactsPanelElement = sidebarDocumentGetElementById("abContactsPanel");
>  
> +  if (top.document.commandDispatcher.focusedWindow == window.content)
> +    return window.content;

Where does this fix come from? Does it fix some behaviour?

@@ +6505,4 @@
>    },
>  
>    timer: Cc["@mozilla.org/timer;1"]
> +           .createInstance(Ci.nsITimer),

Can be moved to previous line.

::: mail/components/compose/content/bigFileObserver.js
@@ +289,3 @@
>      let bucket = document.getElementById("attachmentBucket");
>      let rowCount = bucket.getRowCount();
> +    for (let i = 0; i < rowCount; ++i) {

Probably just inline rowCount here.
Attachment #9011339 - Flags: review?(acelists) → review+
(In reply to Jorg K (GMT+2) from comment #83)
> Comment on attachment 9011339 [details] [diff] [review]
> 1478572-eslint-compose-2.diff

> @@ +2896,5 @@
> >      gAutoSaveTimeout = setTimeout(AutoSave, gAutoSaveInterval);
> >  
> >    gAutoSaveKickedIn = false;
> >  }
> > +/* eslint-enable complexity */
> 
> Where did that one start? Al 2485? 500 complex lines? The entire function,
> yes?

"Function 'ComposeStartup' has a complexity of 89." I guess that's why it's so long.

> @@ +3322,5 @@
> >    }
> >    if (gMsgCompose && originalCharset != gMsgCompose.compFields.characterSet)
> >      SetDocumentCharacterSet(gMsgCompose.compFields.characterSet);
> >  }
> > +/* eslint-enable complexity */
> 
> Where did that start? 3082?

"Function 'GenericSendMessage' has a complexity of 50." Ideally we should be making some effort to split this stuff up. In fact, I think the whole directory could use a bit of splitting up, but it works and we've got bigger problems right now.

> ::: mail/components/compose/content/addressingWidgetOverlay.js
> @@ -3,5 @@
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > -ChromeUtils.import("resource://gre/modules/Services.jsm");
> > -ChromeUtils.import("resource:///modules/MailServices.jsm");
> 
> Not needed?

The import-globals-from brings them from MsgComposeCommands.js as far as the linter's concerned. Both files are loaded in the same window. Side note: addressingWidgetOverlay.js is also used from the address book, in windows that also provide Services and MailServices. But most of it isn't used from there and could be broken off into another file.
 
> @@ -19,5 @@
> > -try {
> > -  if (sPrefs)
> 
> What was 'sPrefs' meant to be?

No idea. It's the only instance of the word "sPrefs" in the tree.
(In reply to :aceman from comment #84)
> Comment on attachment 9011339 [details] [diff] [review]
> 1478572-eslint-compose-2.diff

> > +  if (top.document.commandDispatcher.focusedWindow == window.content)
> > +    return window.content;
> 
> Where does this fix come from? Does it fix some behaviour?

window.content is equal to document.getElementsByTagName('editor')[0].contentWindow, although how it becomes a member of window, I don't actually know.
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/49bf37634232
Turn on ESLint in mail/components/compose; r=jorgk, aceman
https://hg.mozilla.org/comm-central/rev/9a567595d18a
Turn on ESLint in mail/components/compose, mass whitespace changes; rs=whitespace-only
I land white-space changes with [skip-blame] to avoid losing the history, see for example here:
https://hg.mozilla.org/comm-central/rev/1e38647496ae
What difference does that make? I don't see anything different in the blame
Don't you?
https://hg.mozilla.org/comm-central/annotate/2a29ee0adb310b54a6a2df72034953fed8f2b043/mailnews/addrbook/src/nsAddrDatabase.cpp#l134
That's close to nsAddrDatabase::~nsAddrDatabase().

My changeset doesn't show although the whole thing was indented here:
https://hg.mozilla.org/comm-central/rev/1e38647496ae#l1.12
Ah, there's a "Ignore whitespace changes" box you have to check
OK, well, there seems to be quite some ignorance in this field, including mine. When linting was first discussed, one possible drawback was destroying history. Then someone discovered [skip-blame]. And now you're saying, it's not an issue at all, just check the box. So where does that leave us? Forget about [skip-blame] and tell people to check the box?
Never noticed the box before, perhaps it's new?

Anyway, https://hg.mozilla.org/comm-central/annotate/1e38647496ae/mailnews/addrbook/src/nsAddrDatabase.cpp by default do show the change, and when checking the box it's not shown. I don't know if your annotation made a difference or not.
Right, looks like [skip-blame] has no effect. My comment #91 was wrong, the annotated link predated the white-space changes, so surely the changeset landed later didn't show.
Attachment #9011980 - Attachment description: 1478572-eslint-compose-3.diff → 1478572-eslint-compose-3.diff [landed in comment #88]
Attached patch 1478572-eslint-im-1.diff (obsolete) — Splinter Review
* I don't know why we have an "im" component and a "chat" top-level directory. There's probably a good reason, but I don't know what it is.

* I renamed some JSMs to have the .jsm extension. I renamed a JSON file to have the .json extension.

* I don't like imServices.jsm. It feels bad to add things to toolkit's Services object instead of creating a new object (ImServices?) that has only our things. Compare MailServices.jsm. I haven't fixed this though as it's a job for another bug.

* Similar concerns about imXPCOMUtils.jsm.
Attachment #9013191 - Flags: review?(acelists)
(In reply to Geoff Lankow (:darktrojan) from comment #97)
> * I don't know why we have an "im" component and a "chat" top-level
> directory. There's probably a good reason, but I don't know what it is.

The top level "chat" code was shared with a standalone IM application that resided in /im top-level dir. That one was just recently removed.
 
> * I renamed some JSMs to have the .jsm extension. I renamed a JSON file to
> have the .json extension.

But think of the addons :)
 
> * I don't like imServices.jsm. It feels bad to add things to toolkit's
> Services object instead of creating a new object (ImServices?) that has only
> our things. Compare MailServices.jsm. I haven't fixed this though as it's a
> job for another bug.
> * Similar concerns about imXPCOMUtils.jsm.

Please file that separately so that the IM guys can decide.
Comment on attachment 9013191 [details] [diff] [review]
1478572-eslint-im-1.diff

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

Looks OK to me, but I would rather the IM guys to confirm the renamings.

::: mail/components/im/content/imcontact.xml
@@ +219,5 @@
>           if (!this.hasAttribute("aliasing"))
>             return;
>  
>           if (Services.focus.activeWindow == document.defaultView)
> +           this.finishAliasing(true);

Interesting, was this wrong?

::: mail/components/im/content/imconversation.xml
@@ +919,5 @@
>       <method name="browserKeyPress">
>       <parameter name="event"/>
>        <body>
>        <![CDATA[
> +        var accelKeyPressed = AppConstants.platform == "macosx" ? event.metaKey : event.ctrlKey;

Nice, always good to remove preprocessing a file.
Is AppConstants.jsm imported into this file?
Attachment #9013191 - Flags: review?(florian)
Attachment #9013191 - Flags: review?(clokep)
Attachment #9013191 - Flags: review?(acelists)
Attachment #9013191 - Flags: feedback+
(In reply to :aceman from comment #99)
> > +           this.finishAliasing(true);
> 
> Interesting, was this wrong?

It looks wrong, and the linter says it's wrong, but surely it can't be. I think there must be some XBL voodoo I don't understand going on here. This only appeared in <handler>s, and I don't really know how they work.

> > +        var accelKeyPressed = AppConstants.platform == "macosx" ? event.metaKey : event.ctrlKey;
> 
> Nice, always good to remove preprocessing a file.
> Is AppConstants.jsm imported into this file?

Elements with this binding live in the main Thunderbird document, so yes. At least I don't think they occur elsewhere.
Comment on attachment 9013191 [details] [diff] [review]
1478572-eslint-im-1.diff

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

Found some mistakes. Fixing my copy now…

::: mail/components/im/content/imAccounts.js
@@ +12,2 @@
>  ChromeUtils.import("resource:///modules/MailServices.jsm");
> +ChromeUtils.import("resource:///modules/Preferences.jsm");

Oops, that should be resource://gre/modules/Preferences.jsm.

::: mail/components/im/content/imStatusSelector.js
@@ -1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -ChromeUtils.import("resource:///modules/imStatusUtils.jsm");

It turns out this is needed. I wonder why I decided I didn't need this, but I DID need imServices.jsm. Probably two different iterations on the same file.
Comment on attachment 9013191 [details] [diff] [review]
1478572-eslint-im-1.diff

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

Looks good to me, thanks for all the cleanups!

::: chat/modules/imSmileys.jsm
@@ +20,5 @@
>    "getSmileyList" // used to display a list of smileys in the UI
>  ];
>  
>  var kEmoticonsThemePref = "messenger.options.emoticonsTheme";
> +var kThemeFile = "theme.json";

This is an API change that breaks compat with existing Adium emoticon sets. Probably doesn't matter as Adium emoticon sets already needed repackaging (to be in a .xpi file), and I don't think any is currently offered on AMO. (There were probably a few on addons.instantbird.org for Instantbird back when we implemented support for emoticon themes).

::: mail/components/im/content/imAccounts.js
@@ +571,5 @@
>  
>    // A preference already exists to define scroll speed, let's use it.
>    get SCROLL_SPEED() {
>      delete this.SCROLL_SPEED;
> +    this.SCROLL_SPEED = Preferences.get("toolkit.scrollbox.scrollIncrement", 20);

Let's not introduce more use of the deprecated Preferences.jsm.

   this.SCROLL_SPEED =
     Services.prefs.getIntPref("toolkit.scrollbox.scrollIncrement", 20);

::: mail/components/im/content/imcontact.xml
@@ +219,5 @@
>           if (!this.hasAttribute("aliasing"))
>             return;
>  
>           if (Services.focus.activeWindow == document.defaultView)
> +           this.finishAliasing(true);

It worked through an obscure quirk of XBL. I think XBL handlers have the binding as their global object. Adding "this." is more explicit anyway.
Attachment #9013191 - Flags: review?(florian)
Attachment #9013191 - Flags: review?(clokep)
Attachment #9013191 - Flags: review+
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/826d12190290
Turn on ESLint in mail/components/im; r=florian
Keywords: checkin-needed
Attachment #9013462 - Attachment description: 1478572-eslint-im-2.diff → 1478572-eslint-im-2.diff [landed in comment #104]
Attached patch 1478572-eslint-cloudfile-1.diff (obsolete) — Splinter Review
This was an easy one. I did it in less time than it takes to run mach build. :)
Attachment #9013539 - Flags: review?(acelists)
Comment on attachment 9013539 [details] [diff] [review]
1478572-eslint-cloudfile-1.diff

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

::: mail/components/cloudfile/cloudFileAccounts.js
@@ +70,4 @@
>      if (typeof aKeyOrAccount == "string")
>        return aKeyOrAccount;
>      else if ("accountKey" in aKeyOrAccount)
>        return aKeyOrAccount.accountKey;

can you take care of this else after return while you're at it
Comment on attachment 9013539 [details] [diff] [review]
1478572-eslint-cloudfile-1.diff

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

Thanks.

::: mail/components/cloudfile/cloudFileAccounts.js
@@ +153,3 @@
>      let key = this._ensureKey(aKeyOrAccount);
>  
> +    Services.prefs.QueryInterface(Ci.nsIPrefBranch).deleteBranch(ACCOUNT_ROOT + key);

Is this QI needed? This only occurs a few times in the cloudfile files, but nowhere else in m-c/c-c. Services.prefs.deleteBranch() is called directly.

::: mail/components/cloudfile/nsBox.js
@@ +227,5 @@
>      }
>  
> +    if (!this._userInfo) {
> +      this._getUserInfo(onGetUserInfoSuccess, onAuthFailure);
> +      return;

Another solution would be to make all these functions return true for now in case there will be some useful value returning in the future.

@@ +652,5 @@
>      }
>    },
>  
>    get _cachedFolderId() {
> +    return this._prefBranch.getCharPref("folderid", "");

Nice, I didn't know they made this not throw and take a default value. Bug 1338306.
We should go in a new bug and simplify the needless try{}catch() blocks around get*Pref().
Attachment #9013539 - Flags: review?(acelists) → review+
Any idea why in MozMill I see:
JavaScript error: resource:///modules/chatHandler.jsm, line 22: NS_ERROR_FILE_NOT_FOUND:

That's:
var ChatCore = {
  initialized: false,
  _initializing: false,
  init() {
    if (this._initializing)
      return;
    this._initializing = true;

22  ChromeUtils.import("resource:///modules/index_im.jsm", null);

I was running this one:
mozmake SOLO_TEST=folder-pane/test-folder-pane.js mozmill-one
Flags: needinfo?(geoff)
The file was renamed and needs a build to pick up the changes. I expect you've done a build though, so… hmmm.
Flags: needinfo?(geoff)
I do builds all the time, last bustage was 6 hours ago :-(
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/dd917dc324b6
Turn on ESLint in mail/components/cloudfile; r=aceman
Only two directories remaining!
Attachment #9013539 - Attachment is obsolete: true
Attachment #9013938 - Flags: review+
I have a patch for search as well, to complete the set, but I'm going to change things around in there because it's a hive of unnecessarily pre-processed files.
Attachment #9015003 - Flags: review?(acelists)
Attached patch 1478572-eslint-search-1.diff (obsolete) — Splinter Review
Here it is. I've made some packaging changes so that this folder is more sensibly organised. Hopefully not broken anything.
Attachment #9015011 - Flags: review?(acelists)
(In reply to Geoff Lankow (:darktrojan) from comment #114)
> Hopefully not broken anything.

Spoke too soon, I missed up the jar.mn, and because I'm not using Windows or Mac, didn't notice. Fixed in my copy.
Comment on attachment 9015003 [details] [diff] [review]
1478572-eslint-newmailaccount-1.diff

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

Thanks.

::: mail/components/newmailaccount/content/accountProvisioner.js
@@ +14,5 @@
>  // To debug, set mail.provider.logging.dump (or .console)="All"
>  var gLog = Log4Moz.getConfiguredLogger("mail.provider");
>  var stringBundle = new StringBundle("chrome://messenger/locale/newmailaccount/accountProvisioner.properties");
>  
> +var isOSX = Services.appinfo.OS == "Darwin";

I liked the brackets better. But if eslint removes them...

@@ +605,5 @@
>        gLog.info("Got provider list JSON.");
>      };
>      request.timeout = CONNECTION_TIMEOUT;
>      request.ontimeout = () => {
> +      gLog.info("Timeout of XMLHttpRequest fetching provider list.");

Nice.
Attachment #9015003 - Flags: review?(acelists) → review+
Comment on attachment 9015011 [details] [diff] [review]
1478572-eslint-search-1.diff

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

Thanks, nice reorganization in the SearchIntegration. However I can't test it on either Win oe Mac.
Jorg, can you try on Windows?
Attachment #9015011 - Flags: review?(acelists)
Attachment #9015011 - Flags: review+
Attachment #9015011 - Flags: feedback?(jorgk)
Comment on attachment 9015011 [details] [diff] [review]
1478572-eslint-search-1.diff

I tried on a new account and there seem to be a few problems:

Chrome file doesn't exist: C:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\bin\chrome\messenger\content\messenger\WinSearchIntegration.js
JavaScript error: , line 0: uncaught exception: Error opening input stream (invalid filename?): chrome://messenger/content/WinSearchIntegration.js

I never saw the "System Integration" dialogue come up, I tried with TB 60 and there is was.

I switched on Windows search integration in the options, created a folder and moved a message into it. I didn't see any of the search integration folders/files which are .mozmsgs folders and .wdseml files. There was also no prompt from Windows to enable WSEnable.exe. So I guess the patch breaks search integration. That's an f-, right?

I tried with TB 60 and I was asked to authorise WSEnable.exe and those folders/files were created.
Attachment #9015011 - Flags: feedback?(jorgk) → feedback-
Try this not-broken version.
Attachment #9015011 - Attachment is obsolete: true
Attachment #9015151 - Flags: review+
Attachment #9015151 - Flags: feedback?(jorgk)
Is this better aceman? Tests for OS X the way we should do it, and has some brackets! I know they're not needed but it makes things clearer.
Attachment #9015003 - Attachment is obsolete: true
Attachment #9015152 - Flags: review+
Attachment #9015152 - Flags: feedback?(acelists)
NOW it has some brackets. How did I not notice that I hadn't saved the file?
Attachment #9015152 - Attachment is obsolete: true
Attachment #9015152 - Flags: feedback?(acelists)
Attachment #9015154 - Flags: review+
Attachment #9015154 - Flags: feedback?(acelists)
Comment on attachment 9015151 [details] [diff] [review]
1478572-eslint-search-2.diff

Yes, that works again.
Attachment #9015151 - Flags: feedback?(jorgk) → feedback+
Comment on attachment 9015154 [details] [diff] [review]
1478572-eslint-newmailaccount-3.diff

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

::: mail/components/newmailaccount/content/accountProvisioner.js
@@ +19,5 @@
>  var RETRY_TIMEOUT = 5000; // 5 seconds
>  var CONNECTION_TIMEOUT = 15000; // 15 seconds
>  
> +function isAccel(event) {
> +  return (AppConstants.platform == "macosx" && event.metaKey) || event.ctrlKey;

I don't have a preference between Services.appinfo.OS and AppConstants.platform so both are fine.

But I would prefer this:
return AppConstants.platform == "macosx" ? event.metaKey : event.ctrlKey;

However that is not the same. The original version seems to return true when either meta or ctrl is pressed. Do both exist on OS X? And is that what was wanted?
Attachment #9015154 - Flags: feedback?(acelists) → feedback+
You're right, and the old code is wrong.
Is it? I was just going to land it. Mac has two "special" keys. So don't we need to detect both?
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/da7cae0efc9d
Turn on ESLint in mail/components/newmailaccount. r=aceman
https://hg.mozilla.org/comm-central/rev/3a92dfd67030
Turn on ESLint in mail/components/search. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Landed with
  return AppConstants.platform == "macosx" ? event.metaKey : event.ctrlKey;
as discussed on IRC.
Target Milestone: --- → Thunderbird 64.0
Type: enhancement → task
Blocks: 1322934
You need to log in before you can comment on or make changes to this bug.