1515877 - Turn on ESLint in mailnews

Status: RESOLVED FIXED

(MailNews Core :: General, task)

Description

[Geoff Lankow (:darktrojan)]

Aceman, you are going to hate me for this one. :)

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-move-config-1.diff [landed, comment 6]

Since newsblog is the only directory in mailnews currently linted, I'm shifting these more-specific rules there, because for now linting mailnews is already a big enough job.

Comment 2

[:aceman]

No, I will love you, within some limits :)

Comment 3

[:aceman]

Comment on attachment 9032883 [details] [diff] [review]
1515877-eslint-move-config-1.diff [landed, comment 6]

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

Why can't the settings be global? When linting isn't enabled for whole of mailnews/ aren't the settings safely dormant but will take effect for folders you enable gradually?

Comment 4

[Geoff Lankow (:darktrojan)]

Well yes, but they make the task much bigger and then mailnews code is inconsistent with all the other code. I'm aiming for "good" here, not "perfect". I especially don't want to have to fix all the dodgy jsdoc comments.

Comment 5

[:aceman]

Comment on attachment 9032883 [details] [diff] [review]
1515877-eslint-move-config-1.diff [landed, comment 6]

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

Ok, let's see how the plan continues.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6ac307a0fdf9
Turn on ESLint in mailnews - move more-specific rules into mailnews/extensions/newsblog. r=aceman

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-news-1.diff

Comment 10

[:aceman]

Comment on attachment 9033149 [details] [diff] [review]
1515877-eslint-news-1.diff

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

Thanks.

::: mailnews/news/test/unit/head_server_setup.js
@@ +2,5 @@
>  ChromeUtils.import("resource:///modules/MailServices.jsm");
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  ChromeUtils.import("resource://testing-common/mailnews/localAccountUtils.js");
>  
>  var CC = Components.Constructor;

'CC' seems unused, including any NNTP tests.

::: mailnews/news/test/unit/test_nntpGroupPassword.js
@@ +61,4 @@
>      try {
>        var trans = server.playTransaction();
>       if (trans)
> +        dump("Commands called: " + uneval(trans) + "\n");

Bad indent.

@@ -76,5 @@
>  });
> -
> -function run_test() {
> -  run_next_test();
> -}

Are the tests in the file still running after removing this?

::: mailnews/news/test/unit/test_nntpPassword.js
@@ +37,4 @@
>      try {
>        var trans = server.playTransaction();
>       if (trans)
> +        dump("Commands called: " + trans.them + "\n");

Bad indent.

::: mailnews/news/test/unit/test_nntpPassword2.js
@@ +83,4 @@
>      try {
>        var trans = server.playTransaction();
>       if (trans)
> +        dump("Commands called: " + trans.them + "\n");

Bad indent.

Comment 11

[Geoff Lankow (:darktrojan)]

> Are the tests in the file still running after removing this?

Yes. Apparently run_test functions that only call run_next_test are unnecessary.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-news-2.diff

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-news-3.diff [landed, comment 15]

Actually, I think these tests are self-contained and we can check for unused vars  in the global scope. It doesn't really make much difference here but r? anyway.

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 9033169 [details] [diff] [review]
1515877-eslint-news-3.diff [landed, comment 15]

Looks OK to me. Aceman isn't here, and I need a patch.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9bbe95da846a
Turn on ESLint in mailnews/news; r=aceman

Comment 16

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-local-1.diff [landed, comment 19]

Comment 17

[:aceman]

Comment on attachment 9033258 [details] [diff] [review]
1515877-eslint-local-1.diff [landed, comment 19]

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

Thanks.

::: mailnews/local/test/unit/head_maillocal.js
@@ +34,5 @@
> +  pop3Daemon,
> +  POP3_RFC1939_handler,
> +  POP3_RFC2449_handler,
> +  POP3_RFC5034_handler,
> +} = ChromeUtils.import("resource://testing-common/mailnews/pop3d.js", null);

Why can't we use the /* import-globals-from ...*/ for these?

::: mailnews/local/test/unit/test_fileName.js
@@ -2,5 @@
>  /**
>   * Test handling of special chars in folder names
>   */
>  
> -ChromeUtils.import("resource:///modules/MailServices.jsm");

MailServices is used in this file, how does eslint see it being defined?

::: mailnews/local/test/unit/test_movemailDownload.js
@@ -2,5 @@
>   * The intent of this file is to test that movemail download code
>   * works correctly.
>   */
> -ChromeUtils.import("resource:///modules/MailServices.jsm");
> -ChromeUtils.import("resource://gre/modules/Services.jsm");

Both are used in the test.

::: mailnews/local/test/unit/test_over2GBMailboxes.js
@@ +68,5 @@
>      }
>    }
>  
>    do_throw("Over 2 GiB msgkey was not found!");
> +  return null;

Will this ever be hit?

::: mailnews/local/test/unit/test_over4GBMailboxes.js
@@ +45,5 @@
>  var gInboxSize = 0;            // The size of the Inbox folder
>  var gInbox;                    // The nsIMsgFolder object of the Inbox folder in Local Folders
>  var gExpectedNewMessages = 0;  // The number of messages pushed manually into the mbox file
>  
> +/* exported alert */

Exported? What does it mean?

::: mailnews/local/test/unit/test_preview.js
@@ +25,2 @@
>                   bugmail11_preview);
> +  } catch (ex) { dump(ex); do_throw(ex); }

I'm not sure why there are both dump and do_throw (I'd guess both arrive in the test log), but at least wrap the lines properly.

::: mailnews/local/test/unit/test_streamHeaders.js
@@ +82,5 @@
>    let haveError = false;
>    try {
>      messageService.streamHeaders(uri, createStreamListener(
> +      function theString(k) {}), null, true);
> +  } catch (e) { haveError = true; }

Wrap the code.

::: mailnews/local/test/unit/test_verifyLogon.js
@@ +82,5 @@
>    localAccountUtils.loadLocalMailAccount();
>  
>  
>    incomingServer = MailServices.accounts
> +                    .createIncomingServer(kUserName, "localhost", "pop3");

Strange indent.

Comment 18

[Geoff Lankow (:darktrojan)]

(In reply to :aceman from comment #17)
> ::: mailnews/local/test/unit/head_maillocal.js
> @@ +34,5 @@
> > +  pop3Daemon,
> > +  POP3_RFC1939_handler,
> > +  POP3_RFC2449_handler,
> > +  POP3_RFC5034_handler,
> > +} = ChromeUtils.import("resource://testing-common/mailnews/pop3d.js", null);
> 
> Why can't we use the /* import-globals-from ...*/ for these?

Yeah, that's a better idea, since this is a head file.

> ::: mailnews/local/test/unit/test_fileName.js
> @@ -2,5 @@
> >  /**
> >   * Test handling of special chars in folder names
> >   */
> >  
> > -ChromeUtils.import("resource:///modules/MailServices.jsm");
> 
> MailServices is used in this file, how does eslint see it being defined?

It's in head_maillocal.js (actually twice, oops) which is imported automatically with the rule mozilla/import-headjs-globals.

> ::: mailnews/local/test/unit/test_over2GBMailboxes.js
> @@ +68,5 @@
> >      }
> >    }
> >  
> >    do_throw("Over 2 GiB msgkey was not found!");
> > +  return null;
> 
> Will this ever be hit?

No, but ESLint doesn't know do_throw throws. I'll add a comment.

> ::: mailnews/local/test/unit/test_over4GBMailboxes.js
> @@ +45,5 @@
> >  var gInboxSize = 0;            // The size of the Inbox folder
> >  var gInbox;                    // The nsIMsgFolder object of the Inbox folder in Local Folders
> >  var gExpectedNewMessages = 0;  // The number of messages pushed manually into the mbox file
> >  
> > +/* exported alert */
> 
> Exported? What does it mean?

That means it's not used here or anywhere the linter might expect to find it, but we still need it. In this case the test runs code that calls window.alert (which doesn't actually make sense as this is an XPCShell test).

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5753ad5a95c9
Turn on ESLint in mailnews/local; r=aceman

Comment 20

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-addrbook-1.diff [landed, comment 22]

Comment 21

[:aceman]

Comment on attachment 9035250 [details] [diff] [review]
1515877-eslint-addrbook-1.diff [landed, comment 22]

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

Thanks, this one seems to work, just needs to fix some bitrot.

::: mailnews/addrbook/content/abDragDrop.js
@@ +6,5 @@
> +/* import-globals-from ../../../mail/base/content/nsDragAndDrop.js */
> +/* import-globals-from ../../../mail/components/addrbook/content/abCommon.js */
> +/* import-globals-from ../../../mail/components/compose/content/addressingWidgetOverlay.js */
> +/* import-globals-from abResultsPane.js */
> +/* globals MailServices */

Please import MailServices.jsm.

::: mailnews/addrbook/content/abMailListDialog.js
@@ +4,5 @@
>  
> +/* import-globals-from ../../../mail/components/addrbook/content/abCommon.js */
> +/* import-globals-from ../../../mail/components/compose/content/addressingWidgetOverlay.js */
> +/* globals MailServices */
> +/* globals Services */

Import the jsms.

::: mailnews/addrbook/content/abResultsPane.js
@@ +4,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/. */
>  
> +/* import-globals-from ../../../mail/components/addrbook/content/abCommon.js */
> +/* globals GetAbViewListener */

Where does this come from? Why isn't it collected by some import-globals-from ?

@@ +23,5 @@
>   *
>   * goSetMenuValue()
>   *   Core function in globalOverlay.js
>   */
> +/* globals getSelectedDirectoryURI, AbResultsPaneDoubleClick, AbEditCard, goSetMenuValue */

The same.

::: mailnews/addrbook/content/addrbookWidgets.xml
@@ +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/. -->
>  
> +<!-- globals Services -->

Import

::: mailnews/addrbook/test/unit/test_db_enumerator.js
@@ +55,5 @@
>          let item = enm_cards.getNext();
>          let abCard = item.QueryInterface(Ci.nsIAbCard);
>  
> +        for (let key of Object.keys(card_properties)) {
> +          abCard.getProperty(key, null);

Why is this change (in -> of) needed?

::: mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch1.js
@@ +215,5 @@
>    });
>  
>    let childCards = ab.childCards;
>    while (childCards.hasMoreElements()) {
> +    childCards.getNext().QueryInterface(Ci.nsIAbCard);

I wonder if this does anything.

::: mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch2.js
@@ -133,5 @@
> -                { search: "te", expected: [7, 8, 9] },
> -                { search: "tes", expected: [8, 9] },
> -                { search: "test", expected: [9] } ];
> -
> -var inputs = [ firstNames, lastNames];//, displayNames, nickNames, emails, lists ];

Are these tests disabled for some reason? We may want to resurrect them, please keep them commented out.

Comment 22

[Pulsebot]

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

Comment 23

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-test-resources-1.diff [landed, comment 27]

This code (mailnews/test) is in truly appalling condition considering how much our tests rely on it. There are variables assumed to exist that could only have come from somewhere else, and things that can only be done in certain circumstances or stuff breaks. I hope to clean it up, but first let's at least get it linted.

Comment 24

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-test-fakeserver-1.diff [landed, comment 30]

And mailnews/test/fakeserver while I'm at it.

Comment 25

[:aceman]

Comment on attachment 9047233 [details] [diff] [review]
1515877-eslint-test-resources-1.diff [landed, comment 27]

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

Thanks.

::: mailnews/test/resources/alertTestUtils.js
@@ +72,2 @@
>      if (typeof confirmCheck == "function") {
> +      // eslint-disable-next-line no-undef

What is this? Isn't confirmCheck the current function? It does not look like an argument to this function. What is this meant to do?

::: mailnews/test/resources/logHelper.js
@@ +14,5 @@
>  
>  var {Log4Moz} = ChromeUtils.import("resource:///modules/gloda/log4moz.js");
>  var {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
>  var {IOUtils} = ChromeUtils.import("resource:///modules/IOUtils.js");
> +// eslint-disable-next-line mozilla/reject-importGlobalProperties

What does this do?

@@ -689,5 @@
> -  do_throw = _wrapped_do_throw;
> -  _orig_do_check_neq = do_check_neq;
> -  do_check_neq = _wrapped_do_check_neq;
> -  _orig_do_check_eq = do_check_eq;
> -  do_check_eq = _wrapped_do_check_eq;

These also seem to exist in mozmill/shared-modules/test-nntp-helpers.js and mozmill/shared-modules/test-folder-display-helpers.js, also unused.

::: mailnews/test/resources/messageGenerator.js
@@ +98,5 @@
>  
>    get contentTypeHeaderValue() {
>      let s = this._contentType;
>      if (this._charset)
> +      s += "; charset=" + this._charset;

Heh, really nice, I thought these were 2 commands :)

@@ +892,5 @@
>        msg.from = srcMsg.to[0];
>  
>        // we want the <>'s.
>        msg.headers["In-Reply-To"] = srcMsg.headers["Message-Id"];
> +      msg.headers.References = (srcMsg.headers.References || [])

How consistent... Just because the other property names contain a - they stay as []...

@@ +1019,5 @@
>      let args = {};
>      // zero out all the age_incr fields in age (if present)
>      if (aSetDef.age_incr) {
>        args.age = {};
> +      for (let [unit] of Object.entries(aSetDef.age_incr))

Object.keys() ?

::: mailnews/test/resources/messageInjection.js
@@ +665,5 @@
>      // Note: in order to cut down on excessive fsync()s, we do a two-pass
>      //  approach.  In the first pass we just allocate messages to the folder
>      //  we are going to insert them into.  In the second pass we insert the
>      //  messages into folders in batches and perform any mutations.
> +    let folderBatches = aMsgFolders.map(folder => { return {folder, messages: []}; });

Does it need the "{ return" ?

::: mailnews/test/resources/messageModifier.js
@@ +32,5 @@
>    this.synMessages = aSynMessages;
>  
>    if (aMsgFolders == null)
>      this.msgFolders = [];
> +  else if (!("length" in aMsgFolders))

Maybe Array.isArray(aMsgFolders) would be clearer.

::: mailnews/test/resources/msgFolderListenerSetup.js
@@ +191,5 @@
>  
>      // Check: array sizes should be equal.
>      Assert.equal(count, array.length);
>  
> +    for (var i = 0; i < count; i++) {

'let' if touching it.

::: mailnews/test/resources/searchTestUtils.js
@@ +4,2 @@
>  
> +// Contains various functions commonly used in testing mailnews search

Dot at end of sentences while you are touching it.

Comment 26

[Geoff Lankow (:darktrojan)]

Comment on attachment 9047233 [details] [diff] [review]
1515877-eslint-test-resources-1.diff [landed, comment 27]

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

::: mailnews/test/resources/alertTestUtils.js
@@ +72,2 @@
>      if (typeof confirmCheck == "function") {
> +      // eslint-disable-next-line no-undef

alertUtilsPrompts.confirmCheck is the current function. confirmCheck is (theoretically) a test-defined function.

::: mailnews/test/resources/logHelper.js
@@ +14,5 @@
>  
>  var {Log4Moz} = ChromeUtils.import("resource:///modules/gloda/log4moz.js");
>  var {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
>  var {IOUtils} = ChromeUtils.import("resource:///modules/IOUtils.js");
> +// eslint-disable-next-line mozilla/reject-importGlobalProperties

importGlobalProperties isn't required now in most contexts. This isn't one of those contexts, so I have to disable the warning about it.

@@ -689,5 @@
> -  do_throw = _wrapped_do_throw;
> -  _orig_do_check_neq = do_check_neq;
> -  do_check_neq = _wrapped_do_check_neq;
> -  _orig_do_check_eq = do_check_eq;
> -  do_check_eq = _wrapped_do_check_eq;

Might as well get it now that we've noticed, I suppose.

::: mailnews/test/resources/messageInjection.js
@@ +665,5 @@
>      // Note: in order to cut down on excessive fsync()s, we do a two-pass
>      //  approach.  In the first pass we just allocate messages to the folder
>      //  we are going to insert them into.  In the second pass we insert the
>      //  messages into folders in batches and perform any mutations.
> +    let folderBatches = aMsgFolders.map(folder => { return {folder, messages: []}; });

Yes, the syntax gets confused by the {}.

::: mailnews/test/resources/messageModifier.js
@@ +32,5 @@
>    this.synMessages = aSynMessages;
>  
>    if (aMsgFolders == null)
>      this.msgFolders = [];
> +  else if (!("length" in aMsgFolders))

Even better, I'll flip the order:

>  if (Array.isArray(aMsgFolders))
>    this.msgFolders = aMsgFolders;
>  else if (aMsgFolders)
>    this.msgFolders = [aMsgFolders];
>  else
>    this.msgFolders = [];

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6fb9189f3a62
Turn on ESLint in mailnews/test/resources; r=aceman DONTBUILD

Comment 28

[:aceman]

Comment on attachment 9047307 [details] [diff] [review]
1515877-eslint-test-fakeserver-1.diff [landed, comment 30]

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

Thanks.

::: mailnews/test/fakeserver/imapd.js
@@ +842,5 @@
>          case "EXISTS":
>            line = "* " + this._selectedMailbox._messages.length + " EXISTS";
>            break;
>          }
> +        response = line + "\0" + response;

I wonder if this is correct if 'line' is undefined. But not your fault.

@@ +924,5 @@
>      // simulate annoying server that drops connection on STARTTLS
>      if (this.dropOnStartTLS) {
>        this.closing = true;
>        return "";
> +    } return "BAD maild doesn't support TLS ATM";

Should there be new line before the return?

@@ +1382,2 @@
>    },
> +  _parseSequenceSet(set, uid, ids /* optional*/) {

Why is there no space before the closing */ in all these comments? Does eslint automatically do it like this?

@@ +1395,2 @@
>        }
> +      if (!(set - 1 in this._selectedMailbox._messages))

Is this actually
(!((set - 1) in this._selectedMailbox._messages))
or as _messages is an array, then 
(!this._selectedMailbox._messages.includes(set - 1))
?

I only knew the syntax ("property" in object) so I wonder what this (int in array) means.

@@ +1517,5 @@
>      if (parts[3])
>        response += "<" + parts[3][0] + ">";
>      response += " ";
>  
> +    data = "";

It's strange that this clears data but then later we append with to it with += .
At if it was not supposed to be empty.

@@ -1711,5 @@
> -           ((aBox._children.length > 0) ?
> -            (((aBox.flags.length > 0) ? ' ' : '') + '\\HasChildren') :
> -            ((aBox.flags.indexOf('\\NoInferiors') == -1) ?
> -             (((aBox.flags.length > 0) ? ' ' : '') + '\\HasNoChildren') :
> -             '')) +

The horror :)

@@ +1901,1 @@
>          return "BAD can't fetch X-CUSTOM-VALUE";

Fix the indent.

@@ +1907,1 @@
>          return "BAD can't fetch X-CUSTOM-LIST";

Fix the indent.

@@ +1939,1 @@
>        namespaces[namespace.type].push(namespace);

{} around for() body.

@@ -2057,5 @@
> -           ((aBox._children.length > 0) ?
> -            (((aBox.flags.length > 0) ? ' ' : '') + '\\HasChildren') :
> -            ((aBox.flags.indexOf('\\NoInferiors') == -1) ?
> -             (((aBox.flags.length > 0) ? ' ' : '') + '\\HasNoChildren') :
> -             '')) + ') "' + aBox.delimiter + '" "' + aBox.displayName + '"\0';

All clear...

::: mailnews/test/fakeserver/maild.js
@@ +186,5 @@
>  
>      for (let reader of this._readers)
>        reader._realCloseSocket();
>  
> +    if (this._readers.some(function(e) { return e.observer.forced; }))

Could be (e => e.observer.forced) while touching it. More occurrences below.

@@ +401,5 @@
>  
>            // Find the command and splice it out...
>            var splitter = line.indexOf(" ");
> +          var command = splitter == -1 ? line : line.substring(0, splitter);
> +          var args = splitter == -1 ? "" : line.substring(splitter + 1);

Let

::: mailnews/test/fakeserver/nntpd.js
@@ +407,2 @@
>      command = command.toUpperCase();
> +    if ("LIST_" + command in this)

I'd make it (("LIST_" + command) in this) to make it readable.

::: mailnews/test/fakeserver/pop3d.js
@@ +274,5 @@
>  
>    this._kAuthSchemeStartFunction = {};
>    this._kAuthSchemeStartFunction["CRAM-MD5"] = this.authCRAMStart;
> +  this._kAuthSchemeStartFunction.PLAIN = this.authPLAINStart;
> +  this._kAuthSchemeStartFunction.LOGIN = this.authLOGINStart;

The same comment as in smtpd.js later.

@@ +359,3 @@
>        if (this.dropOnAuthFailure)
>          this.closing = true;
>        return "-ERR Wrong username or password, crook!";

Reindent?

::: mailnews/test/fakeserver/smtpd.js
@@ +38,5 @@
>  
>    this._kAuthSchemeStartFunction = {};
>    this._kAuthSchemeStartFunction["CRAM-MD5"] = this.authCRAMStart;
> +  this._kAuthSchemeStartFunction.PLAIN = this.authPLAINStart;
> +  this._kAuthSchemeStartFunction.LOGIN = this.authLOGINStart;

I don't like this inconsistency.
Would it complain about this?
this._kAuthSchemeStartFunction = {
 "CRAM-MD5": this.authCRAMStart,
 "PLAIN": this.authPLAINStart,
 "LOGIN": this.authLOGINStart,
}

It's hard when we push half-legal property names on the object.
It looks to me _kAuthSchemeStartFunction should be a Map() anyway and then the ugliness would go.

@@ +146,3 @@
>        if (this.dropOnAuthFailure)
>          this.closing = true;
>        return "535 5.7.8 Wrong username or password, crook!";

Did you intentionally not re-indent this? As you did re-indent a similar block later on.

Comment 29

[Geoff Lankow (:darktrojan)]

(In reply to :aceman from comment #28)

@@ +1382,2 @@

},

• _parseSequenceSet(set, uid, ids /* optional*/) {

Why is there no space before the closing */ in all these comments? Does
eslint automatically do it like this?

The linter doesn't care about space before the closing characters, so it only adds them at the start. Fixed four of them.

@@ +1395,2 @@

   }

•  if (!(set - 1 in this._selectedMailbox._messages))
  

Is this actually
(!((set - 1) in this._selectedMailbox._messages))
or as _messages is an array, then
(!this._selectedMailbox._messages.includes(set - 1))
?

I only knew the syntax ("property" in object) so I wonder what this (int in
array) means.

Looks like it's supposed to be !((set - 1) in …. An array is basically an object with numeric keys, so in works the same way.

@@ +1517,5 @@

 if (parts[3])
   response += "<" + parts[3][0] + ">";
 response += " ";

• data = "";

It's strange that this clears data but then later we append with to it with
+= .
At if it was not supposed to be empty.

I think they've just reused a variable name. Nothing else about the function implied that the earlier value was still needed.

@@ +146,3 @@

   if (this.dropOnAuthFailure)
     this.closing = true;
   return "535 5.7.8 Wrong username or password, crook!";

Did you intentionally not re-indent this? As you did re-indent a similar
block later on.

Nope, I'm just blind.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9f651d2726f2
Turn on ESLint in mailnews/test/fakeserver; r=aceman

Comment 31

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-base-test-1.diff [landed, comment 34]

Here's the next bit.

Comment 32

[Jorg K (CEST = GMT+2)]

Comment on attachment 9051167 [details] [diff] [review]
1515877-eslint-base-test-1.diff [landed, comment 34]

Before this rots :-(
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=13dbf9dc1071b423c7d07ecb2efebfed8abbc892

Comment 33

[Jorg K (CEST = GMT+2)]

Comment on attachment 9051167 [details] [diff] [review]
1515877-eslint-base-test-1.diff [landed, comment 34]

OK, I looked through it and didn't find anything exciting, in fact, lots of repetition. I noticed that `recursiveDeleteMailboxes()` is also unused in test_listSubscribed.js and test_lsub.js, but they are IMAP tests.

Strangely enough linting doesn't pass with the patch applied ;-)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mailnews/base/test/unit/test_postPluginFilters.js:55:4 | Missing trailing comma. (comma-dangle)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mailnews/base/test/unit/test_searchAddressInAb.js:149:4 | Missing trailing comma. (comma-dangle)

I'll fix it.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/564a2fae15fb
Turn on ESLint in mailnews/base/test. r=jorgk DONTBUILD

Comment 35

[Jorg K (CEST = GMT+2)]

I landed that with DONTBUILD since I wanted to use it as a test case to start a Daily, see bug 1538462.

Comment 36

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-base-1.diff [landed, comment 39]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0c051ca6d6763d0ee492f567c83d7c625ab95442

I found a bunch of dead code in the filter editor.

Comment 37

[:aceman]

Comment on attachment 9054786 [details] [diff] [review]
1515877-eslint-base-1.diff [landed, comment 39]

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

Argh, all my pending patches for folderWidgets.xml are busted now... :(

Thanks.

::: mailnews/base/content/junkCommands.js
@@ +55,5 @@
>  
>    // move only when the corresponding setting is activated
>    // and the currently viewed folder is not the junk folder.
>    if (spamSettings.moveOnSpam &&
> +      !(aFolder.flags & Ci.nsMsgFolderFlags.Junk)) {

I prefer aFolder.getFlag(Ci.nsMsgFolderFlags.Junk) when touching it.

@@ +94,5 @@
>        aFolder.storeCustomKeywords(null, "Junk", "NonJunk", junkMsgKeys, junkMsgKeys.length);
>      }
>  
> +    if (aGoodMsgHdrs.length) {
> +      var goodMsgKeys = [];

let?

@@ +343,5 @@
>    MsgJunkMailInfo(true);
>  
>    // use direct folder commands if possible so we don't mess with the selection
>    let selectedFolder = gFolderDisplay.displayedFolder;
> +  if (!(selectedFolder.flags & Ci.nsMsgFolderFlags.Virtual)) {

.getFlag()

::: mailnews/base/content/msgSynchronize.js
@@ +105,1 @@
>      return item;

Fix the indent please while here.

::: mailnews/base/prefs/content/AccountManager.js
@@ +98,2 @@
>      case "identity":
> +      element.identitykey = account.defaultIdentity.key;

How I hate this.

@@ +1482,2 @@
>      if (!this._dataStore.hasValue(document.documentURI, aAccountKey, "open")) {
>      // If there was no value stored, use opened state.

Looks like bad indent.

::: mailnews/base/prefs/content/AccountManager.xul
@@ +27,2 @@
>  <script type="application/javascript" src="chrome://messenger/content/am-help.js"/>
> +#endif

Does it cause errors loading the file?

::: mailnews/base/prefs/content/AccountWizard.js
@@ +441,3 @@
>        if (accountData.identity && accountData.identity.email) {
>            // fixup the email address if we have a default domain
> +          var emailArray = accountData.identity.email.split("@");

Let?

::: mailnews/base/prefs/content/accountUtils.js
@@ +144,5 @@
>          }
>  
>          // This will do nothing on platforms without a shell service
> +        const NS_SHELLSERVICE_CID = "@mozilla.org/suite/shell-service;1";
> +        if (NS_SHELLSERVICE_CID in Cc) {

I wonder why this needs the constant.

::: mailnews/base/prefs/content/am-identity-edit.js
@@ +199,2 @@
>  
> +    var attachSignaturePath = document.getElementById("identity.signature").value;

let

::: mailnews/base/prefs/content/am-offline.js
@@ +280,2 @@
>  
> +    for (var i = 0; i < prefstrArray.length; i++) {

let

::: mailnews/base/prefs/content/am-server-advanced.js
@@ +71,5 @@
>    }
>  
>    var controls = getControls();
>  
> +  for (var i = 0; i < controls.length; i++) {

let

@@ +117,5 @@
>    }
>  
>    // Save the controls back to the "gServerSettings" array.
>    var controls = getControls();
> +  for (var i = 0; i < controls.length; i++) {

let

::: mailnews/base/prefs/content/am-server.js
@@ +302,5 @@
>                    document.getElementById("fixedUserName"),
>                    document.getElementById("fixedServerPort")];
>  
>    var len = controls.length;
> +  for (var i = 0; i < len; i++) {

let

::: mailnews/base/prefs/content/aw-done.js
@@ +23,5 @@
>          email = pageData.identity.email.value;
>      }
>      setDivTextFromForm("identity.email", email);
>  
> +    var userName = "";

'let' here and below where you touch the vars.

::: mailnews/base/prefs/content/aw-incoming.js
@@ +67,1 @@
>        newsServer.value = pageData.newsserver.hostname.value;

The block is kinda strange, whole newsServer isn't used anywhere.

::: mailnews/base/prefs/content/aw-outgoing.js
@@ +101,1 @@
>          smtpNameInput.value = smtpNameInput.value || loginNameInput.value;

heh

::: mailnews/base/prefs/content/converterDialog.js
@@ +105,5 @@
>        deferredToAccountName = deferredToAccount.incomingServer.username;
>      }
>  
>      if (aServer.rootFolder.filePath.path != deferredToRootFolder) {
> +      document.getElementById("warningSpan").textContent =

Are you sure about these? Please test it.
I thought .textContent does not wrap long lines, but innerHTML does, which we need here.

::: mailnews/base/search/content/FilterEditor.js
@@ -217,5 @@
>    return true;
>  }
>  
> -// the folderListener object
> -var gFolderListener = {

Looks like remnants from the possibility to create a folder directly in the dialog. I think there is a bug to reinstate it. But we probably want to implement it in the folder picker, not the filter editor specifically.

::: mailnews/base/search/content/searchTerm.js
@@ -358,5 @@
> -    var editFilter = null;
> -    try { editFilter = gFilter; } catch(e) { }
> -
> -    var editMailView = null;
> -    try { editMailView = gMailView; } catch(e) { }

Strange.

::: mailnews/base/search/content/searchWidgets.xml
@@ +162,3 @@
>                var filterActionRow = listBox.getItemAtIndex(index);
> +              if (filterActionRow != currentFilterActionRow) {
> +                var actionValue = filterActionRow.getAttribute("value");

let

::: mailnews/base/util/errUtils.js
@@ +292,5 @@
> +      "originalTargetXPath",
> +    ];
> +    for (let name of names) {
> +      if (name in event)
> +        this._append(name + ": " + event[name] + "\n");

Nice, thanks.

::: mailnews/base/util/folderUtils.jsm
@@ +140,5 @@
>  
>    // This is a HACK to work around bug 41133. If we have one of the
>    // dummy "news" accounts there, that account won't have an
>    // incomingServer attached to it, and everything will blow up.
> +  accountList = accountList.filter((a) => a.incomingServer);

Pre-existing, but maybe !!a.incomingServer would be more correct here (to return a boolean) ?

@@ +191,5 @@
>    for (let folder of aFolderList) {
>      addIfRecent(folder);
>    }
>  
> +  return recentFolders.map(function(f) { return f.folder; });

You didn't convert this to (f) => f.folder ?

::: mailnews/base/util/mailnewsMigrator.js
@@ +147,4 @@
>          let card = childCards.getNext()
>                               .QueryInterface(Ci.nsIAbCard);
>  
> +        if (card.getProperty("AllowRemoteContent", "0") == "0")

How did you find out?

Comment 38

[Geoff Lankow (:darktrojan)]

(In reply to :aceman from comment #37)

::: mailnews/base/prefs/content/AccountManager.xul
@@ +27,2 @@

<script type="application/javascript" src="chrome://messenger/content/am-help.js"/>
+#endif

Does it cause errors loading the file?

No, but nothing in that file is used by Thunderbird. I've stopped us shipping it.

::: mailnews/base/prefs/content/converterDialog.js
@@ +105,5 @@

   deferredToAccountName = deferredToAccount.incomingServer.username;
 }

 if (aServer.rootFolder.filePath.path != deferredToRootFolder) {

•  document.getElementById("warningSpan").textContent =
  

Are you sure about these? Please test it.
I thought .textContent does not wrap long lines, but innerHTML does, which
we need here.

That doesn't make sense. Line wrapping is controlled by the CSS, not the DOM. But yes, I tested it anyway and it's fine.

::: mailnews/base/util/folderUtils.jsm
@@ +140,5 @@

// This is a HACK to work around bug 41133. If we have one of the
// dummy "news" accounts there, that account won't have an
// incomingServer attached to it, and everything will blow up.

• accountList = accountList.filter((a) => a.incomingServer);

Pre-existing, but maybe !!a.incomingServer would be more correct here (to
return a boolean) ?

That's implicitly already happening in the filter function.

::: mailnews/base/util/mailnewsMigrator.js
@@ +147,4 @@

     let card = childCards.getNext()
                          .QueryInterface(Ci.nsIAbCard);

•    if (card.getProperty("AllowRemoteContent", "0") == "0")
  

How did you find out?

IIRC, I fought with this for ages and ended up running the code by hand. I don't think getProperty ever returns an integer even if one's recorded.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d34239c5e7a0
Turn on ESLint in mailnews/base; r=aceman
https://hg.mozilla.org/comm-central/rev/cbd032bfb562
Mass indentation corrections in mailnews/base; rs=white-space-only

Comment 40

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-compose-1.diff [landed, comment 42]

This one should be fairly quick, plus it's mostly test code, so don't be too picky.

Comment 41

[:aceman]

Comment on attachment 9059422 [details] [diff] [review]
1515877-eslint-compose-1.diff [landed, comment 42]

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

Thanks.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d99eabee0b1f
Turn on ESLint in mailnews/compose. r=aceman

Comment 43

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-db-1.diff

Comment 44

[:aceman]

Comment on attachment 9059753 [details] [diff] [review]
1515877-eslint-db-1.diff

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

::: mailnews/db/gloda/modules/collection.js
@@ +40,2 @@
>        // purge dead weak references while we're at it
> +      collections = this._collectionsByNoun[nounID].filter((aRef) => aRef.get());

Are the brackets around aRef needed?

@@ -290,5 @@
>        cache.add(needToCache.concat(Object.keys(unresolvedIndexToItem).
>                                     map(key => unresolvedIndexToItem[key])));
>      }
> -
> -    return aItems;

Why is this removed?

::: mailnews/db/gloda/modules/datamodel.js
@@ -638,5 @@
>     * Trash this message's in-memory representation because it should no longer
>     *  be reachable by any code.  The database record is gone, it's not coming
>     *  back.
>     */
> -  _objectPurgedMakeYourselfUnpleasant: function gloda_message_nuke() {

I didn't know we have funny function names ;)

::: mailnews/db/gloda/modules/gloda.js
@@ -1278,4 @@
>          if (aConversation instanceof GlodaConversation)
>            return [null, aConversation.id];
> -        else // assume they're just passing the id directly
> -          return [null, aConversation];

Why is the comment lost?

Comment 45

[Geoff Lankow (:darktrojan)]

(In reply to :aceman from comment #44)

•  collections = this._collectionsByNoun[nounID].filter((aRef) => aRef.get());
  

Are the brackets around aRef needed?

No, but some reviewers around here get annoyed if I write arrow functions without them.

• return aItems;

Why is this removed?

In another branch of the function nothing is returned. I checked that neither caller uses the return value, and besides, it's an argument, so they already have reference to it.

•    else // assume they're just passing the id directly
  

•      return [null, aConversation];
  

Why is the comment lost?

Oops.

Comment 46

[:aceman]

Comment on attachment 9059753 [details] [diff] [review]
1515877-eslint-db-1.diff

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

Rest of the review :)

::: mailnews/db/gloda/test/unit/base_index_messages.js
@@ +269,5 @@
>    // Configure the gloda indexer to hang while streaming the message.
>    configure_gloda_indexing({hangWhile: "streaming"});
>  
>    // create a folder with a message inside.
> +  let [folder] = make_folder_with_sets([{count: 1}]);

Will this work? If make_folder_with_sets returns 2 values, don't we rather need 'let [folder, ] = make_folder_with_sets([{count: 1}]);' here? I want to prevent that folder gets automatically assigned an array or something, when it only should be a single object.

::: mailnews/db/gloda/test/unit/base_query_messages.js
@@ +546,5 @@
>   * @tests gloda.noun.message.attr.recipientsMatch
>   * @tests gloda.datastore.sqlgen.kConstraintFulltext
>   */
>  function test_query_messages_by_recipients_name() {
> +  let [name] = world.peoples[0];

Again, world.peoples[0] seem to be an array of 2 elements. Or you can try to make it 'let name = world.peoples[0][0];' as done elsewhere.

::: mailnews/db/gloda/test/unit/resources/glodaTestHelper.js
@@ +49,4 @@
>  // Create a message generator
>  var msgGen = gMessageGenerator = new MessageGenerator();
>  // Create a message scenario generator using that message generator
> +var scenarios = new MessageScenarioFactory(msgGen);

This could use some cleanup. Many tests set 'scenarios' (but don't use it) and also 'gMessageScenarioFactory' which is defined in messageInjection.js (which does not use it). Or we can do it in a followup.

Is 'scenarios' used in this particular file (or the ones it imports)?

@@ +606,5 @@
>      for (let item of aItems) {
>        if (item.headerMessageID in this._glodaMessagesByMessageId)
>          mark_failure(
>            ["Gloda message", item.folderMessage,
> +            "already indexed once since the last wait_for_gloda_indexer call!",

Indent.

@@ +787,5 @@
>   *     the original array's sequence being maintained.
>   */
>  function permute(aArray, aPermutationId) {
>    let out = [];
> +  for (let l = aArray.length; l > 0; l--) {

Eh, 'l' named variable that looks like 1? :)

::: mailnews/db/gloda/test/unit/test_index_compaction.js
@@ +264,5 @@
>    // turn off indexing...
>    configure_gloda_indexing({event: false});
>  
>    // create a folder with a message inside.
> +  let [folder] = make_folder_with_sets([{count: 1}]);

[folder,] ?

::: mailnews/db/gloda/test/unit/test_mime_attachments_size.js
@@ +23,5 @@
>  load("../../../../resources/messageGenerator.js");
>  load("../../../../resources/messageModifier.js");
>  load("../../../../resources/messageInjection.js");
>  
> +/* exported scenarios */

Where is it exported to? Single user of 'scenarios' seems to be mailnews/db/gloda/test/unit/base_index_messages.js, which is a standalone test, not loading this one. Or what am I missing?

::: mailnews/db/gloda/test/unit/test_mime_emitter.js
@@ +28,5 @@
>  load("../../../../resources/messageGenerator.js");
>  load("../../../../resources/messageModifier.js");
>  load("../../../../resources/messageInjection.js");
>  
> +/* exported scenarios */

Again.

::: mailnews/db/gloda/test/unit/test_nuke_migration.js
@@ +55,5 @@
>    make_out_of_date_database();
>  
>    // - tickle gloda
>    // public.js loads gloda.js which self-initializes and initializes the datastore
> +  ChromeUtils.import("resource:///modules/gloda/public.js");

Why this change?

::: mailnews/db/msgdb/test/unit/test_maildb.js
@@ +34,5 @@
>      doTest(++gCurTestNum);
>    },
>    function test_async_open() {
>      let messageGenerator = new MessageGenerator();
> +    gTestFolder.QueryInterface(Ci.nsIMsgLocalMailFolder);

Ok, so localFolder was unused. Do we need a gTestFolder QId to nsIMsgLocalMailFolder anywhere? Is the gTestFolder.addMessage() call below using gTestFolder as nsIMsgLocalMailFolder?
Of so, would it be better to do 'gTestFolder = gTestFolder.QueryInterface(Ci.nsIMsgLocalMailFolder);' here to make it clear? But then gTestFolder is a nsIMsgLocalMailFolder for the rest of the test.

Or if not, then the whole line is unneeded?

::: mailnews/db/msgdb/test/unit/test_propertyEnumerator.js
@@ +30,4 @@
>    // test some of the default properties
>    var enumerator = gHdr.propertyEnumerator;
>    var properties = [];
> +  while (enumerator.hasMore()) {

Note for me: looks like all this could be done as 'var properties = toArray(gHdr.propertyEnumerator);' from fixIterator, if not with some new builtin JS iterator unrolling.

Comment 47

[Geoff Lankow (:darktrojan)]

(In reply to :aceman from comment #46)

• let [folder] = make_folder_with_sets([{count: 1}]);

Will this work? If make_folder_with_sets returns 2 values, don't we rather
need 'let [folder, ] = make_folder_with_sets([{count: 1}]);' here? I want to
prevent that folder gets automatically assigned an array or something, when
it only should be a single object.

Yes, that works: let [a] = [1, 2, 3]; a == 1; In fact, the linter prevents adding the trailing comma and space.

::: mailnews/db/gloda/test/unit/resources/glodaTestHelper.js

+var scenarios = new MessageScenarioFactory(msgGen);

This could use some cleanup. Many tests set 'scenarios' (but don't use it)
and also 'gMessageScenarioFactory' which is defined in messageInjection.js
(which does not use it). Or we can do it in a followup.

Is 'scenarios' used in this particular file (or the ones it imports)?

This is one of those spaghetti-code things we seem to have far too much of. I'd rather not try to figure out exactly what's going on in this bug.

@@ +787,5 @@

• the original array's sequence being maintained.
  

*/
function permute(aArray, aPermutationId) {
let out = [];

• for (let l = aArray.length; l > 0; l--) {

Eh, 'l' named variable that looks like 1? :)

Well that's odd. I'm going to change it to i, like everybody else uses. Maybe this was once buried in loops and they'd already used i, j, and k.

::: mailnews/db/gloda/test/unit/test_index_compaction.js
@@ +264,5 @@

• let [folder] = make_folder_with_sets([{count: 1}]);

[folder,] ?

As before.

::: mailnews/db/gloda/test/unit/test_mime_attachments_size.js
@@ +23,5 @@

+/* exported scenarios */

Where is it exported to? Single user of 'scenarios' seems to be
mailnews/db/gloda/test/unit/base_index_messages.js, which is a standalone
test, not loading this one. Or what am I missing?

As before. base_index_messages.js isn't a test, it's included by some of the tests.

::: mailnews/db/gloda/test/unit/test_nuke_migration.js
@@ +55,5 @@

make_out_of_date_database();

// - tickle gloda
// public.js loads gloda.js which self-initializes and initializes the datastore

• ChromeUtils.import("resource:///modules/gloda/public.js");

Why this change?

We don't use the Gloda object, but the file still needs to be imported here so that it runs.

::: mailnews/db/msgdb/test/unit/test_maildb.js
@@ +34,5 @@

 doTest(++gCurTestNum);

},
function test_async_open() {
let messageGenerator = new MessageGenerator();

• gTestFolder.QueryInterface(Ci.nsIMsgLocalMailFolder);

Ok, so localFolder was unused. Do we need a gTestFolder QId to
nsIMsgLocalMailFolder anywhere? Is the gTestFolder.addMessage() call below
using gTestFolder as nsIMsgLocalMailFolder?
Of so, would it be better to do 'gTestFolder =
gTestFolder.QueryInterface(Ci.nsIMsgLocalMailFolder);' here to make it
clear? But then gTestFolder is a nsIMsgLocalMailFolder for the rest of the
test.

Or if not, then the whole line is unneeded?

I'll add the gTestFolder =. The effect is the same, but this does make it clearer.

::: mailnews/db/msgdb/test/unit/test_propertyEnumerator.js
@@ +30,4 @@

// test some of the default properties
var enumerator = gHdr.propertyEnumerator;
var properties = [];

• while (enumerator.hasMore()) {

Note for me: looks like all this could be done as 'var properties =
toArray(gHdr.propertyEnumerator);' from fixIterator, if not with some new
builtin JS iterator unrolling.

Probably true, but it is called test_propertyEnumerator, I guess it should test the property enumerator.

Comment 48

[:aceman]

Ok, thanks.

Comment 49

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-db-2.diff [landed, comment 51, partly backed out, comment 54]

Comment 50

[Geoff Lankow (:darktrojan)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=93c7fffe77daee36f1fc6fdbf8bf752b36019d4d

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/635dddf89443
Turn on ESLint in mailnews/db. r=aceman

Comment 52

[alta88]

the latest change has broken gloda faceted search; backing out the cs fixes it.

Comment 53

[Jorg K (CEST = GMT+2)]

Indeed. I'll take care of it.

Comment 54

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/d26bb5a3349ffceaffc8409f32f2f3023b873385
Backed out non-test part of changeset 635dddf89443 (bug 1515877) for breaking Gloda facet search. a=backout

I backed half of it out. Test still pass and facet search works again.

Comment 55

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-db-3.diff [landed, comment 56]

Yeah, I screwed up.

Comment 56

[Pulsebot]

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

Comment 57

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-extensions-1.diff [landed, comment 66]

Something for the weekend :)

Comment 58

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-imap-1.diff [landed, comment 66]

This is 100% test code, so I'd rather other stuff was reviewed first.

Comment 59

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-import-1.diff [landed, comment 66]

Comment 60

[:aceman]

Comment on attachment 9062778 [details] [diff] [review]
1515877-eslint-import-1.diff [landed, comment 66]

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

Thanks.

::: mailnews/import/content/fieldMapImport.js
@@ +65,5 @@
>    var count = top.fieldMap.mapSize;
>    var index;
>    var i;
>    for (i = 0; i < count; i++) {
> +    index = top.fieldMap.GetFieldMap(i);

Seems the 'var' can be moved into the loop, as 'let'.

@@ +175,5 @@
>    top.fieldMap.SetFieldMapSize(max);
>  
>    for (var i = 0; i < max; i++) {
> +    fIndex = gListbox.getItemAtIndex(i).getAttribute("field-index");
> +    on = gListbox.getItemAtIndex(i).firstChild.getAttribute("checked");

'var' for 'on' and 'fIndex' can be moved inside loop.

::: mailnews/import/content/import-test.html
@@ +28,1 @@
>  <form> 

Kill the trailing space while here.

::: mailnews/import/content/importDialog.js
@@ +254,2 @@
>               gImportMsgsBundle.getFormattedString("ImportSettingsSuccess", [ name ]),
>               null, true);

There is some strange indent here.

::: mailnews/import/test/unit/test_winmail.js
@@ -143,5 @@
>      };
>    },
>  };
>  
> -function _test(registry, expectedAccounts) {

expectedAccounts is used on line 150, why removing it?

Comment 61

[:aceman]

Comment on attachment 9062392 [details] [diff] [review]
1515877-eslint-extensions-1.diff [landed, comment 66]

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

Thanks.

::: mailnews/extensions/bayesian-spam-filter/test/unit/test_msgCorpus.js
@@ +130,2 @@
>    do_test_pending();
> +  while (1) {

'while (true)' could be cleaner

::: mailnews/extensions/mdn/content/am-mdn.js
@@ +104,5 @@
>  }
>  
>  // Disables xul elements that have associated preferences locked.
> +function onLockPreference(initPrefString, keyString) {
> +  var finalPrefString;

Can this be folded into the line with assignment to the variable?

Comment 62

[Geoff Lankow (:darktrojan)]

Comment on attachment 9062778 [details] [diff] [review]
1515877-eslint-import-1.diff [landed, comment 66]

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

::: mailnews/import/test/unit/test_winmail.js
@@ -143,5 @@
>      };
>    },
>  };
>  
> -function _test(registry, expectedAccounts) {

It's already defined in this scope (line 124) why pass it around? I actually missed removing it from the call to _test, so I'll fix that.

Comment 63

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-intl-1.diff [landed, comment 75]

This is all tests, too.

Comment 64

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-jsaccount-1.diff [landed, comment 75]

Comment 65

[Geoff Lankow (:darktrojan)]

Attachment: 1515877-eslint-mime-1.diff

I think that's the last one for this bug!

Comment 66

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fb00935549d3
Turn on ESLint in mailnews/extensions; r=aceman
https://hg.mozilla.org/comm-central/rev/5e693bbfa3fc
Turn on ESLint in mailnews/imap; rs=NPOTB
https://hg.mozilla.org/comm-central/rev/7ca0abedb917
Turn on ESLint in mailnews/import; r=aceman

Comment 67

[Jorg K (CEST = GMT+2)]

Comment on attachment 9062776 [details] [diff] [review]
1515877-eslint-imap-1.diff [landed, comment 66]

Why exactly was IMAP not reviewed? Review was requested and then cancelled. "Not part of the build" is used in M-C for code that isn't compiled, like stuff with `#ifdef MOZ_THUNDERBIRD` or so. Although some IMAP tests may be switched off, most of them actually run, so "NPOTB" doesn't apply, IMO.

Comment 68

[Geoff Lankow (:darktrojan)]

I suggested doing it this way (so that we can get this job finished before the beta changeover) to Magnus, and he agreed.

NPOTB refers to anything which isn't shipped to users, at least as I've always understood it.

Comment 69

[Jorg K (CEST = GMT+2)]

Comment on attachment 9063400 [details] [diff] [review]
1515877-eslint-intl-1.diff [landed, comment 75]

r=jorgk

Comment 70

[Jorg K (CEST = GMT+2)]

(In reply to Geoff Lankow (:darktrojan) from comment #68)

I suggested doing it this way (so that we can get this job finished before the beta changeover) to Magnus, and he agreed.

I'm a big friend of PLR, but not of "no review", unless its automatic changes, like the C++ reformatting (and even those, I review).

NPOTB refers to anything which isn't shipped to users, at least as I've always understood it.

With this argument you could make any test changes you want, including removing all tests :-( - You could also make any changes to the build configuration. NPOTB refers to dead code, something that doesn't change any aspect of anything.

Comment 71

[:aceman]

Comment on attachment 9062776 [details] [diff] [review]
1515877-eslint-imap-1.diff [landed, comment 66]

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

Thanks.

::: mailnews/imap/test/unit/test_imapAutoSync.js
@@ +179,4 @@
>        dump("folder added into Q " + this.qName(queue) + " " + folder.URI + "\n");
>        if (folder instanceof Ci.nsIMsgFolder &&
>            queue == nsIAutoSyncMgrListener.PriorityQueue) {
> +        return;

Why these returns?

::: mailnews/imap/test/unit/test_imapFilterActionsPostplugin.js
@@ +396,2 @@
>        var msgHdr = aMsgHdrs.queryElementAt(i, Ci.nsIMsgDBHdr);
> +      let isOffline = !!((msgHdr.flags & Ci.nsMsgMessageFlags.Offline));

Too many brackets?

::: mailnews/imap/test/unit/test_offlineStoreLocking.js
@@ +175,4 @@
>  };
>  
>  function teardown() {
> +  Assert.ok(gGotAlert);

How did you find out? :)

::: mailnews/imap/test/unit/test_syncChanges.js
@@ +39,5 @@
>      IMAPPump.inbox.updateFolderWithListener(null, asyncUrlListener);
>      yield false;
>    },
>    function checkMailboxEmpty() {
> +    IMAPPump.inbox.msgDatabase.getMsgHdrForMessageID(gSynthMessage.messageId);

Does this do anything when not returning the header?

Comment 72

[Jorg K (CEST = GMT+2)]

We can land the intl stuff.

Comment 73

[Jorg K (CEST = GMT+2)]

Comment on attachment 9062776 [details] [diff] [review]
1515877-eslint-imap-1.diff [landed, comment 66]

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

::: mailnews/imap/test/unit/test_imapAutoSync.js
@@ +179,4 @@
>        dump("folder added into Q " + this.qName(queue) + " " + folder.URI + "\n");
>        if (folder instanceof Ci.nsIMsgFolder &&
>            queue == nsIAutoSyncMgrListener.PriorityQueue) {
> +        return;

Hmm, otherwise the block would be empty, so why have it in the first place?

Comment 74

[:aceman]

Comment on attachment 9063402 [details] [diff] [review]
1515877-eslint-jsaccount-1.diff [landed, comment 75]

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

Thanks.

Comment 75

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/46772686920b
Turn on ESLint in mailnews/intl. r=jorgk
https://hg.mozilla.org/comm-central/rev/71907b0d0630
Turn on ESLint in mailnews/jsaccount. r=aceman

Comment 76

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cb2423b0246d
follow-up - Fix reviewer comments; r=aceman DONTBUILD

Comment 77

[:aceman]

Comment on attachment 9063405 [details] [diff] [review]
1515877-eslint-mime-1.diff

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

::: mailnews/mime/jsmime/test/head_xpcshell_glue.js
@@ +42,5 @@
>             .then(contents => callback(undefined, contents), callback);
>    },
>  };
>  requireCache.set("fs", fs);
> +var {jsmime} = ChromeUtils.import("resource:///modules/jsmime.jsm");

Does Components.utils.import() still work?

::: mailnews/mime/test/unit/head_mime.js
@@ +9,5 @@
>  var {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
>  var {MailServices} = ChromeUtils.import("resource:///modules/MailServices.jsm");
>  var {XPCOMUtils} = ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  var {mailTestUtils} = ChromeUtils.import("resource://testing-common/mailnews/mailTestUtils.js");
> +var {PromiseTestUtils} = ChromeUtils.import("resource://testing-common/mailnews/PromiseTestUtils.jsm");

How did this work with the typo? Isn't it unused? Or maybe imported via other file.

::: mailnews/mime/test/unit/test_attachment_size.js
@@ +27,5 @@
>  
>  // Create a message generator
>  var msgGen = gMessageGenerator = new MessageGenerator();
>  // Create a message scenario generator using that message generator
> +new MessageScenarioFactory(msgGen);

This is strange. does creating a dead object (without reference to it) do anything?
If none of the variables are needed, can this line be removed?
I also think some of these are unused, I have bug 1547208 for it.

::: mailnews/mime/test/unit/test_badContentType.js
@@ +24,5 @@
>  
>  // Create a message generator
>  var msgGen = gMessageGenerator = new MessageGenerator();
>  // Create a message scenario generator using that message generator
> +new MessageScenarioFactory(msgGen);

Ditto.

::: mailnews/mime/test/unit/test_hidden_attachments.js
@@ +23,5 @@
>  
>  // Create a message generator
>  var msgGen = gMessageGenerator = new MessageGenerator();
>  // Create a message scenario generator using that message generator
> +new MessageScenarioFactory(msgGen);

Ditto.

Comment 78

[Geoff Lankow (:darktrojan)]

(In reply to :aceman from comment #77)

+var {jsmime} = ChromeUtils.import("resource:///modules/jsmime.jsm");

Does Components.utils.import() still work?

Yes it does, I don't know why this one was still hanging around though.

+var {PromiseTestUtils} = ChromeUtils.import("resource://testing-common/mailnews/PromiseTestUtils.jsm");

How did this work with the typo? Isn't it unused? Or maybe imported via
other file.

Pure luck, the right variable is also imported in the only file that needs it.

+new MessageScenarioFactory(msgGen);

This is strange. does creating a dead object (without reference to it) do
anything?
If none of the variables are needed, can this line be removed?
I also think some of these are unused, I have bug 1547208 for it.

I keep seeing this pattern and can never remember what I do with it. Removed.

Comment 79

[Pulsebot]

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