1478572 - Turn on ESLint in mail/components

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Geoff Lankow (:darktrojan)]

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.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: eslint-mail-components-aboutsupport

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: eslint-mail-components-aboutsupport-diff.diff

This is the same patch, but only the bits that weren't done by |mach eslint --fix|.

Comment 3

[Jorg K (CEST = GMT+2)]

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.

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: eslint-mail-components-aboutsupport

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: eslint-mail-components-accountcreation

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: eslint-mail-components-accountcreation-diff.diff

Again, only the things changed by humans.

Comment 7

[Jorg K (CEST = GMT+2)]

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.

Comment 8

[Jorg K (CEST = GMT+2)]

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?

Comment 9

[Geoff Lankow (:darktrojan)]

(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

Comment 10

[Geoff Lankow (:darktrojan)]

(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.

Comment 11

[Geoff Lankow (:darktrojan)]

Attachment: eslint-mail-components-accountcreation-diff.diff - r+

Changed to remove the blanked disabling of the complexity rule.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: eslint-mail-components-aboutsupport.diff [landed in comment #14]

Comments fixed, ready to land.

Comment 13

[Jorg K (CEST = GMT+2)]

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 ;-)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fe8684d0f25a
Turn on ESLint in mail/components/about-support. r=jorgk

Comment 15

[Jorg K (CEST = GMT+2)]

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)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4b5191f432bc
Fix eslint bustage by re-adding missing exceptions; a=bustage DONTBUILD

Comment 17

[:aceman]

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.

Comment 18

[Geoff Lankow (:darktrojan)]

Attachment: eslint-mail-components-accountcreation

(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.

Comment 19

[Jorg K (CEST = GMT+2)]

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?

Comment 20

[Geoff Lankow (:darktrojan)]

Attachment: eslint-accountcreation-auto-1478572 [landed in comment #24]

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.

Comment 21

[Geoff Lankow (:darktrojan)]

Attachment: eslint-accountcreation-1478572

… 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.

Comment 22

[Jorg K (CEST = GMT+2)]

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.

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: eslint-accountcreation-1478572.patch [landed in comment #24]

Tweaked the comments a bit. I hope
  // --------
  // Do stuff
is acceptable.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

Comment 25

[Jorg K (CEST = GMT+2)]

Before I forget to ask again: How do you run linting locally?

Comment 26

[Geoff Lankow (:darktrojan)]

(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/)

Comment 27

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-devtools-1.diff [landed in comment #31]

This one should go a little easier. :)

Comment 28

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-downloads-1.diff [landed in comment #31]

So should this. Just some semicolons and a comment.

Comment 29

[:aceman]

Comment on attachment 8996607 [details] [diff] [review]
1478572-eslint-devtools-1.diff [landed in comment #31]

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

Thanks :)

Comment 30

[:aceman]

Comment on attachment 8996611 [details] [diff] [review]
1478572-eslint-downloads-1.diff [landed in comment #31]

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

Nice

Comment 31

[Pulsebot]

Pushed by [e-mail]:
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

Comment 32

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-addrbook-auto-1.diff

Comment 33

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-addrbook-manual-1.diff

Comment 34

[Geoff Lankow (:darktrojan)]

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.

Comment 35

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-addrbook-manual-2.diff

Comment 36

[:aceman]

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.

Comment 37

[:aceman]

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?

Comment 38

[Geoff Lankow (:darktrojan)]

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 39

[Geoff Lankow (:darktrojan)]

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

Comment 40

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-addrbook-manual-3.diff

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. :-/

Comment 41

[:aceman]

Comment on attachment 9004167 [details] [diff] [review]
1478572-eslint-addrbook-manual-3.diff

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

Thanks.

Comment 42

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-addrbook-4.diff [landed in comment #42]

Gear down, flaps set, prepare for landing. :)

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dc872eb2bea0
Turn on ESLint in mail/components/addrbook; r=aceman

Comment 44

[Jorg K (CEST = GMT+2)]

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?

Comment 45

[Jorg K (CEST = GMT+2)]

Attachment: 1478572-addressbook-follow-up.patch [landed in comment 49]

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.

Comment 46

[Geoff Lankow (:darktrojan)]

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. :-)

Comment 47

[Jorg K (CEST = GMT+2)]

Right, but there was a logic error as well which led to the checkbox not being shown correctly :-(

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a03add67bee9
Turn on ESLint in mail/components/addrbook, follow-up: fix logic error. r=darktrojan

Comment 49

[Jorg K (CEST = GMT+2)]

(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

Comment 50

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-preferences-auto-1.diff

I had some spare time so I took on mail/components/preferences. It's a bit of a monster, >1000 errors.

Comment 51

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-preferences-manual-1.diff

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.

Comment 52

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-preferences-auto-2.diff [landed in comment #63]

I thought I'd dealt with the new comma-dangle rule, but apparently I had not.

Comment 53

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-preferences-manual-2.diff

Comment 54

[Jorg K (CEST = GMT+2)]

Backport of fix in abCard.js, TB 60 beta 11:
https://hg.mozilla.org/releases/comm-beta/rev/9c11a5d238e1b4a7753add4cee7d9bed4c1d932a

Comment 55

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-preferences-manual-3.diff

Updated because of bug 1488445.

Comment 56

[Geoff Lankow (:darktrojan)]

Comment on attachment 9005954 [details] [diff] [review]
1478572-eslint-preferences-auto-2.diff [landed in comment #63]

See comment 51.

Comment 57

[:aceman]

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.

Comment 58

[:aceman]

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 {}.

Comment 59

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-preferences-manual-4.diff [landed in comment #63]

Comment 60

[Geoff Lankow (:darktrojan)]

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.

Comment 61

[:aceman]

(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 62

[:aceman]

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

Comment 63

[Pulsebot]

Pushed by [e-mail]:
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

Comment 64

[Jorg K (CEST = GMT+2)]

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=

Comment 65

[Jorg K (CEST = GMT+2)]

The reviewer didn't ask the question, so I'm asking ;-)

Comment 66

[Geoff Lankow (:darktrojan)]

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

Comment 67

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-mail-components-1.diff

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.

Comment 68

[Jorg K (CEST = GMT+2)]

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 69

[:aceman]

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 ?

Comment 70

[:aceman]

(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.

Comment 71

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-mail-components-2.diff landed in comment #74]

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.

Comment 72

[Jorg K (CEST = GMT+2)]

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 73

[:aceman]

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.

Comment 74

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/09e76a937c343d2762a740fc5ba6a0529fabb008
Turn on ESLint by default in mail/components with some exceptions. r=aceman DONTBUILD

Comment 75

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-activity-1.diff

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. :-)

Comment 76

[Jorg K (CEST = GMT+2)]

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?

Comment 77

[:aceman]

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' :)

Comment 78

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-activity-2.diff [landed in comment #80]

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?! :-)

Comment 79

[:aceman]

Comment on attachment 9010469 [details] [diff] [review]
1478572-eslint-activity-2.diff [landed in comment #80]

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

Thanks

Comment 80

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9567ebfd9783
Turn on ESLint in mail/components/activity. r=aceman

Comment 81

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-compose-1.diff

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.

Comment 82

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-compose-2.diff

Okay, who was the smart person who wrote this?!

> var nsIPlaintextEditorMail = Ci.nsIPlaintextEditor;

Comment 83

[Jorg K (CEST = GMT+2)]

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?

Comment 84

[:aceman]

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.

Comment 85

[Geoff Lankow (:darktrojan)]

(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.

Comment 86

[Geoff Lankow (:darktrojan)]

(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.

Comment 87

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-compose-3.diff [landed in comment #88]

Comment 88

[Pulsebot]

Pushed by [e-mail]:
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

Comment 89

[Jorg K (CEST = GMT+2)]

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

Comment 90

[Magnus Melin [:mkmelin]]

What difference does that make? I don't see anything different in the blame

Comment 91

[Jorg K (CEST = GMT+2)]

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

Comment 92

[Magnus Melin [:mkmelin]]

Ah, there's a "Ignore whitespace changes" box you have to check

Comment 93

[Jorg K (CEST = GMT+2)]

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?

Comment 94

[Magnus Melin [:mkmelin]]

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.

Comment 95

[Jorg K (CEST = GMT+2)]

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.

Comment 96

[Jorg K (CEST = GMT+2)]

Compare Geoff's changes
https://hg.mozilla.org/comm-central/annotate/9a567595d18a/mail/components/compose/content/MsgComposeCommands.js#l550
https://hg.mozilla.org/comm-central/annotate/9a567595d18a/mail/components/compose/content/MsgComposeCommands.js?ignorews=1#l550
White-space changeset ignored despite no [skip-blame].

Comment 97

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-im-1.diff

* 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.

Comment 98

[:aceman]

(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 99

[:aceman]

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?

Comment 100

[Geoff Lankow (:darktrojan)]

(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 101

[Geoff Lankow (:darktrojan)]

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 102

[Florian Quèze [:florian]]

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.

Comment 103

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-im-2.diff [landed in comment #104]

Comment 104

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/826d12190290
Turn on ESLint in mail/components/im; r=florian

Comment 105

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-cloudfile-1.diff

This was an easy one. I did it in less time than it takes to run mach build. :)

Comment 106

[Magnus Melin [:mkmelin]]

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 107

[:aceman]

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().

Comment 108

[Jorg K (CEST = GMT+2)]

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

Comment 109

[Geoff Lankow (:darktrojan)]

The file was renamed and needs a build to pick up the changes. I expect you've done a build though, so… hmmm.

Comment 110

[Jorg K (CEST = GMT+2)]

I do builds all the time, last bustage was 6 hours ago :-(

Comment 111

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dd917dc324b6
Turn on ESLint in mail/components/cloudfile; r=aceman

Comment 112

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-cloudfile-2.diff [landed in comment #111]

Only two directories remaining!

Comment 113

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-newmailaccount-1.diff

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.

Comment 114

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-search-1.diff

Here it is. I've made some packaging changes so that this folder is more sensibly organised. Hopefully not broken anything.

Comment 115

[Geoff Lankow (:darktrojan)]

(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 116

[:aceman]

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.

Comment 117

[:aceman]

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?

Comment 118

[Jorg K (CEST = GMT+2)]

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.

Comment 119

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-search-2.diff

Try this not-broken version.

Comment 120

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-newmailaccount-2.diff

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.

Comment 121

[Geoff Lankow (:darktrojan)]

Attachment: 1478572-eslint-newmailaccount-3.diff

NOW it has some brackets. How did I not notice that I hadn't saved the file?

Comment 122

[Jorg K (CEST = GMT+2)]

Comment on attachment 9015151 [details] [diff] [review]
1478572-eslint-search-2.diff

Yes, that works again.

Comment 123

[:aceman]

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?

Comment 124

[Geoff Lankow (:darktrojan)]

You're right, and the old code is wrong.

Comment 125

[Jorg K (CEST = GMT+2)]

Is it? I was just going to land it. Mac has two "special" keys. So don't we need to detect both?

Comment 126

[Pulsebot]

Pushed by [e-mail]:
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

Comment 127

[Jorg K (CEST = GMT+2)]

Landed with
  return AppConstants.platform == "macosx" ? event.metaKey : event.ctrlKey;
as discussed on IRC.