Turn on ESLint in mail/components

RESOLVED FIXED in Thunderbird 64.0

Status

enhancement
RESOLVED FIXED
11 months ago
2 months ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

Trunk
Thunderbird 64.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(16 attachments, 25 obsolete attachments)

17.16 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
147.39 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
46.97 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
7.02 KB, patch
aceman
: review+
Details | Diff | Splinter Review
2.91 KB, patch
aceman
: review+
Details | Diff | Splinter Review
125.84 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
2.55 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
180.18 KB, patch
aceman
: review+
Details | Diff | Splinter Review
67.29 KB, patch
aceman
: review+
Details | Diff | Splinter Review
47.69 KB, patch
aceman
: review+
Details | Diff | Splinter Review
112.39 KB, patch
aceman
: review+
Details | Diff | Splinter Review
289.74 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
166.36 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
73.30 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
62.39 KB, patch
darktrojan
: review+
jorgk
: feedback+
Details | Diff | Splinter Review
31.85 KB, patch
darktrojan
: review+
aceman
: feedback+
Details | Diff | Splinter Review
Assignee

Description

11 months ago
I'm going to work on this when I have a small amount of spare time that isn't big enough to do other things in. I'll do one patch per directory, or more I guess if it's a big one.
Assignee

Comment 1

11 months ago
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Assignee

Comment 2

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

Comment 3

11 months ago
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.
Assignee

Comment 4

11 months ago
Attachment #8995087 - Attachment is obsolete: true
Assignee

Updated

11 months ago
Attachment #8995088 - Flags: review?(jorgk)
Assignee

Comment 6

11 months ago
Again, only the things changed by humans.
Attachment #8995111 - Flags: review?(jorgk)

Comment 7

11 months ago
Comment on attachment 8995088 [details] [diff] [review]
eslint-mail-components-aboutsupport-diff.diff

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

Looks OK.

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

What happened here?

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

Nit: Since you're touching these comments, you could add the missing full stop at the end.
Attachment #8995088 - Flags: review?(jorgk) → review+

Comment 8

11 months ago
Comment on attachment 8995111 [details] [diff] [review]
eslint-mail-components-accountcreation-diff.diff

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

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

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

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

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

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

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

Why did this move to the catch block?

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

Why true?
Attachment #8995111 - Flags: review?(jorgk) → review?(acelists)
Assignee

Comment 9

11 months ago
(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
Assignee

Comment 10

11 months ago
(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.
Assignee

Comment 11

11 months ago
Changed to remove the blanked disabling of the complexity rule.
Attachment #8995110 - Attachment is obsolete: true
Attachment #8995111 - Attachment is obsolete: true
Attachment #8995111 - Flags: review?(acelists)
Attachment #8995142 - Flags: review?(acelists)
Assignee

Comment 12

11 months ago
Comments fixed, ready to land.
Attachment #8995092 - Attachment is obsolete: true
Assignee

Updated

11 months ago
Keywords: checkin-needed
Assignee

Updated

11 months ago
Attachment #8995144 - Flags: review+
Assignee

Updated

11 months ago
Attachment #8995088 - Attachment is obsolete: true

Comment 13

11 months ago
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

11 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fe8684d0f25a
Turn on ESLint in mail/components/about-support. r=jorgk
Keywords: checkin-needed

Comment 15

11 months ago
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

11 months ago
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/4b5191f432bc
Fix eslint bustage by re-adding missing exceptions; a=bustage DONTBUILD

Comment 17

11 months ago
Comment on attachment 8995142 [details] [diff] [review]
eslint-mail-components-accountcreation-diff.diff - r+

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

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

Would you be able to finish bug 1426882 ?

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

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

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

I don't know what this does.

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

This would be a candidate for {} to bind it to the if() visually, or at least a blank line after this.
Attachment #8995142 - Flags: review?(acelists) → review+
Assignee

Comment 18

11 months ago
(In reply to :aceman from comment #17)
> ::: b/mail/components/accountcreation/content/guessConfig.js
> @@ -985,4 @@
> >                           .getService(Ci.nsISocketTransportService);
> >  
> >    // @see NS_NETWORK_SOCKET_CONTRACTID_PREFIX
> > -  var socketTypeName = ssl == SSL ? "ssl" : (ssl == TLS ? "starttls" : null);
> 
> Is this flagged by ESLint? But yes, it is quite unreadable without brackets.

Yes it is. Nested ternary are forbidden.

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

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

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

Fixed.
Attachment #8995142 - Attachment is obsolete: true
Attachment #8995845 - Flags: review+
Assignee

Updated

11 months ago
Keywords: checkin-needed

Comment 19

11 months ago
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?
Assignee

Comment 20

11 months ago
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.
Assignee

Comment 21

11 months ago
… and here's the manual bits again. I've removed the ugly comment decorations and used two blank lines above and one below to make them stand out. Also removed other instances of two or more blank lines in those files.
Attachment #8995845 - Attachment is obsolete: true
Assignee

Updated

11 months ago
Attachment #8995919 - Attachment is patch: true

Updated

11 months ago
Attachment #8995142 - Attachment description: eslint-mail-components-accountcreation-diff.diff → eslint-mail-components-accountcreation-diff.diff - r+

Updated

11 months ago
Attachment #8995144 - Attachment description: eslint-mail-components-aboutsupport.diff → eslint-mail-components-aboutsupport.diff [landed in comment #14]

Comment 22

11 months ago
Comment on attachment 8995911 [details] [diff] [review]
eslint-accountcreation-auto-1478572 [landed in comment #24]

I've looked through those. I decent commit message would be appreciated, also a file extension for Windows users.
Attachment #8995911 - Flags: review+

Comment 23

11 months ago
Tweaked the comments a bit. I hope
  // --------
  // Do stuff
is acceptable.
Attachment #8995919 - Attachment is obsolete: true
Attachment #8996052 - Flags: review+

Comment 24

11 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/dbabbfef78c1
Turn on ESLint in mail/components/accountcreation (automatic changes). r=jorgk
https://hg.mozilla.org/comm-central/rev/f536b94ef285
Turn on ESLint in mail/components/accountcreation (manual changes). r=aceman
Keywords: checkin-needed

Comment 25

11 months ago
Before I forget to ask again: How do you run linting locally?
Assignee

Comment 26

11 months ago
(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/)
Assignee

Updated

11 months ago
Attachment #8995911 - Attachment description: eslint-accountcreation-auto-1478572 → eslint-accountcreation-auto-1478572 [landed in comment #24]
Assignee

Updated

11 months ago
Attachment #8996052 - Attachment description: eslint-accountcreation-1478572.patch → eslint-accountcreation-1478572.patch [landed in comment #24]
Assignee

Comment 27

11 months ago
This one should go a little easier. :)
Attachment #8996607 - Flags: review?(acelists)
Assignee

Comment 28

11 months ago
So should this. Just some semicolons and a comment.
Attachment #8996611 - Flags: review?(acelists)

Comment 29

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

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

Thanks :)
Attachment #8996607 - Flags: review?(acelists) → review+

Comment 30

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

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

Nice
Attachment #8996611 - Flags: review?(acelists) → review+

Comment 31

11 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/970e484cd962
Turn on ESLint in mail/components/devtools. r=aceman
https://hg.mozilla.org/comm-central/rev/98f869c1bcd5
Turn on ESLint in mail/components/downloads. r=aceman DONTBUILD

Updated

11 months ago
Attachment #8996607 - Attachment description: 1478572-eslint-devtools-1.diff → 1478572-eslint-devtools-1.diff [landed in comment #31]

Updated

11 months ago
Attachment #8996611 - Attachment description: 1478572-eslint-downloads-1.diff → 1478572-eslint-downloads-1.diff [landed in comment #31]
Assignee

Comment 34

10 months ago
Comment on attachment 9003725 [details] [diff] [review]
1478572-eslint-addrbook-auto-1.diff

Bit of a monster, I'm afraid, but it's mostly whitespace. This patch was created by mach eslint --fix.
Attachment #9003725 - Flags: review?(acelists)
Assignee

Comment 35

10 months ago
Attachment #9003726 - Attachment is obsolete: true
Attachment #9004125 - Flags: review?(acelists)

Comment 36

10 months ago
Comment on attachment 9003725 [details] [diff] [review]
1478572-eslint-addrbook-auto-1.diff

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

Thanks, nice.

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

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

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

Why this change?

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

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

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

{} around the branch.

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

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

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

The spaces :)

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

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

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

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

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

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

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

{} for the branch

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

The indentation is now wrong.

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

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

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

Use XXX while there :)

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

Use XXX while there :)

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

'let' while touching it.

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

no spaces

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

Now the elses do not align vertically. Please add {} for other branches if at least one has them.
Attachment #9003725 - Flags: review?(acelists) → review+

Comment 37

10 months ago
Comment on attachment 9004125 [details] [diff] [review]
1478572-eslint-addrbook-manual-2.diff

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

Thanks :)

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

Why is this better?

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

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

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

Why?

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

Aha, you already fixed this here;)

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

What's the problem here?

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

Here the {} could go.

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

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

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

Default of 1?
Attachment #9004125 - Flags: review?(acelists) → review+
Assignee

Comment 38

10 months ago
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.
Assignee

Comment 39

10 months ago
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
Assignee

Comment 40

10 months ago
Just the changes in response to the review. I didn't realise that replying to review comments using the review tool didn't copy the original message, but they should be readable by going back to the review tool. :-/
Attachment #9004167 - Flags: review?(acelists)

Comment 41

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

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

Thanks.
Attachment #9004167 - Flags: review?(acelists) → review+
Assignee

Comment 42

10 months ago
Gear down, flaps set, prepare for landing. :)
Attachment #9003725 - Attachment is obsolete: true
Attachment #9004125 - Attachment is obsolete: true
Attachment #9004167 - Attachment is obsolete: true
Attachment #9004169 - Flags: review+
Assignee

Updated

10 months ago
Attachment #9004169 - Attachment description: 1478572-eslint-addrbook-4.diff [landed in comment #41] → 1478572-eslint-addrbook-4.diff [landed in comment #42]

Comment 43

10 months ago
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/dc872eb2bea0
Turn on ESLint in mail/components/addrbook; r=aceman

Comment 44

10 months ago
Why did you merge the two patches? Now it's hard to see who made the mistakes :-(

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

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

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

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

Also, this appears to be a bug since min wasn't defined. So perhaps we should backport that fix, no?
Flags: needinfo?(geoff)

Comment 45

10 months ago
I've done it for you so you have more time for bug 1486051 ;-)

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

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

So I just fixed the logic error and the indentation.
Flags: needinfo?(geoff)
Attachment #9004334 - Flags: review?(geoff)
Assignee

Comment 46

10 months ago
Comment on attachment 9004334 [details] [diff] [review]
1478572-addressbook-follow-up.patch [landed in comment 49]

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

If you start nit-picking indentation matters you'll be here a long time. :-)
Attachment #9004334 - Flags: review?(geoff) → review+

Comment 47

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

Comment 48

10 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a03add67bee9
Turn on ESLint in mail/components/addrbook, follow-up: fix logic error. r=darktrojan

Comment 49

10 months ago
(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
Assignee

Comment 50

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

Comment 51

10 months ago
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.
Assignee

Updated

10 months ago
Attachment #9004334 - Attachment description: 1478572-addressbook-follow-up.patch → 1478572-addressbook-follow-up.patch [landed in comment 49]
Assignee

Comment 52

10 months ago
I thought I'd dealt with the new comma-dangle rule, but apparently I had not.
Attachment #9005913 - Attachment is obsolete: true
Assignee

Comment 53

10 months ago
Attachment #9005915 - Attachment is obsolete: true
Assignee

Comment 55

10 months ago
Updated because of bug 1488445.
Attachment #9005955 - Attachment is obsolete: true
Assignee

Comment 56

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

See comment 51.
Attachment #9005954 - Flags: review?(acelists)
Assignee

Updated

10 months ago
Attachment #9006432 - Flags: review?(acelists)

Comment 57

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

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

Thanks.

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

Why this? Does eslint not like these aligned initializations?

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

Does it want the comma after last member?

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

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

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

These have now lost the vertical alignment.

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

Let.

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

{} around the branch.

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

let

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

This needs {} and reindenting now.

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

{}

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

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

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

{}

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

This looks weird now.

@@ +467,1 @@
>          break;

{}

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

{} now

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

Does this change (fix) behaviour) ?

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

Reindent now?

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

Maybe this would deserve {} now.

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

Probably break this line at the = ?

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

let

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

This if() could use {} around the block.
Attachment #9005954 - Flags: review?(acelists) → review+

Comment 58

10 months ago
Comment on attachment 9006432 [details] [diff] [review]
1478572-eslint-preferences-manual-3.diff

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

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

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

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

Nice.

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

Is this function actually used?

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

'let' when you are touching this.

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

Interesting. Did this even work?

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

'let' when you are touching this.

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

Add {} for the loop even when oneliner.

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

Are the new {} needed?

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

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

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

'let' when touching it.

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

Why is all dns.* removed?

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

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

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

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

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

What does split() return if the string is empty? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/split says it is "an array containing one empty string". Is this check then correct? Also it could use {}.
Attachment #9006432 - Flags: feedback+
Assignee

Comment 59

10 months ago
Attachment #9006432 - Attachment is obsolete: true
Attachment #9006432 - Flags: review?(acelists)
Attachment #9006727 - Flags: review?(acelists)
Assignee

Comment 60

10 months ago
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

10 months ago
(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

10 months ago
Comment on attachment 9006727 [details] [diff] [review]
1478572-eslint-preferences-manual-4.diff [landed in comment #63]

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

Thanks
Attachment #9006727 - Flags: review?(acelists) → review+
Assignee

Updated

10 months ago
Keywords: checkin-needed

Comment 63

10 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0cac668a566b
Turn on ESLint in mail/components/preferences (automatic changes). r=aceman
https://hg.mozilla.org/comm-central/rev/2a29ee0adb31
Turn on ESLint in mail/components/preferences (manual changes). r=aceman
Keywords: checkin-needed

Comment 64

10 months ago
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

10 months ago
The reviewer didn't ask the question, so I'm asking ;-)
Flags: needinfo?(geoff)
Assignee

Comment 66

10 months ago
That case can never be hit. It'd only be handleInternally if we'd set it that way earlier, using a menuitem we don't have. Besides, Thunderbird doesn't do the action in question. It looks like I'm not the first person to come across this: https://searchfox.org/comm-central/source/mail/components/preferences/applications.js#1173
Flags: needinfo?(geoff)
Assignee

Updated

9 months ago
Attachment #9005954 - Attachment description: 1478572-eslint-preferences-auto-2.diff → 1478572-eslint-preferences-auto-2.diff [landed in comment #63]
Assignee

Updated

9 months ago
Attachment #9006727 - Attachment description: 1478572-eslint-preferences-manual-4.diff → 1478572-eslint-preferences-manual-4.diff [landed in comment #63]
Assignee

Comment 67

9 months ago
The number of folders we don't lint is now smaller than the ones we do lint, so I've flipped the exception list and linted all the stray files from around the place.
Attachment #9009076 - Flags: review?(acelists)

Comment 68

9 months ago
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

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

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

Great, thanks for doing this cleanup.

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

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

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

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

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

Is this line too long ?

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

If somebody uses fixIterator in this file, will he need to add it to this line?
Why wasn't this change needed in other files using iteratorUtils.jsm ?
Attachment #9009076 - Flags: review?(acelists) → review+

Comment 70

9 months ago
(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.
Assignee

Comment 71

9 months ago
Round 2.

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

That's correct. There's only one other use of iteratorUtils still in files I've done so far, … and I did it the lazy way. Fixed that here too.
Attachment #9009076 - Attachment is obsolete: true
Attachment #9009289 - Flags: review?(acelists)

Comment 72

9 months ago
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

9 months ago
Comment on attachment 9009289 [details] [diff] [review]
1478572-eslint-mail-components-2.diff landed in comment #74]

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

Thanks.
Attachment #9009289 - Flags: review?(acelists) → review+
Assignee

Updated

9 months ago
Keywords: checkin-needed

Comment 74

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

Updated

9 months ago
Attachment #9009289 - Attachment description: 1478572-eslint-mail-components-2.diff → 1478572-eslint-mail-components-2.diff landed in comment #74]

Updated

9 months ago
Keywords: checkin-needed
Assignee

Comment 75

9 months ago
I haven't been well this week so I thought I'd do something light… apparently this was not it.

Go on, complain that I've removed a bunch of state-the-obvious comments. :-)
Attachment #9010191 - Flags: review?(acelists)

Comment 76

9 months ago
Comment on attachment 9010191 [details] [diff] [review]
1478572-eslint-activity-1.diff

I've looked through the entire thing, no complaints. So why wasn't it light?
Attachment #9010191 - Flags: feedback+

Comment 77

9 months ago
Comment on attachment 9010191 [details] [diff] [review]
1478572-eslint-activity-1.diff

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

Looks fine, thank you.

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

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

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

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

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

Constants and they are not even 'const' :)
Attachment #9010191 - Flags: review?(acelists) → review+
Assignee

Comment 78

9 months ago
I made a few changes, you might want to check them.

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

Because I just did the same thing to all the lines here? Why is this one named differently though? Who knows?! :-)
Attachment #9010191 - Attachment is obsolete: true
Attachment #9010469 - Flags: review?(acelists)

Comment 79

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

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

Thanks
Attachment #9010469 - Flags: review?(acelists) → review+
Assignee

Updated

9 months ago
Keywords: checkin-needed

Comment 80

9 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9567ebfd9783
Turn on ESLint in mail/components/activity. r=aceman
Keywords: checkin-needed
Assignee

Comment 81

9 months ago
Posted patch 1478572-eslint-compose-1.diff (obsolete) — Splinter Review
Warning: monster patch!

A few things of note here:

* There's a few big blocks where the indentation needs fixing. That can be done before landing, as it makes the diff even bigger and hides actual changes.
* I've broken up some nested ternary operations and some crazy if conditions. Please check I've done it right.
Attachment #9011330 - Flags: review?(jorgk)
Assignee

Updated

9 months ago
Attachment #9010469 - Attachment description: 1478572-eslint-activity-2.diff → 1478572-eslint-activity-2.diff [landed in comment #80]
Assignee

Comment 82

9 months ago
Posted patch 1478572-eslint-compose-2.diff (obsolete) — Splinter Review
Okay, who was the smart person who wrote this?!

> var nsIPlaintextEditorMail = Ci.nsIPlaintextEditor;
Attachment #9011330 - Attachment is obsolete: true
Attachment #9011330 - Flags: review?(jorgk)
Attachment #9011339 - Flags: review?(jorgk)

Comment 83

9 months ago
Comment on attachment 9011339 [details] [diff] [review]
1478572-eslint-compose-2.diff

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

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

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

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

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

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

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

Nit: bad indent (I can fix).

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

Where did that start? 3082?

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

Nit: I'd revert that indentation change.

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

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

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

use 'bundle'.

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

Nit: bad indent.

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

Not needed?

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

What was 'sPrefs' meant to be?
Attachment #9011339 - Flags: review?(jorgk) → review+

Updated

9 months ago
Attachment #9011339 - Flags: review?(acelists)

Comment 84

9 months ago
Comment on attachment 9011339 [details] [diff] [review]
1478572-eslint-compose-2.diff

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

Thank you.

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

How ugly ;)

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

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

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

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

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

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

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

Can be moved to previous line.

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

Probably just inline rowCount here.
Attachment #9011339 - Flags: review?(acelists) → review+
Assignee

Comment 85

9 months ago
(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.
Assignee

Comment 86

9 months ago
(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 88

9 months ago
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/49bf37634232
Turn on ESLint in mail/components/compose; r=jorgk, aceman
https://hg.mozilla.org/comm-central/rev/9a567595d18a
Turn on ESLint in mail/components/compose, mass whitespace changes; rs=whitespace-only

Comment 89

9 months ago
I land white-space changes with [skip-blame] to avoid losing the history, see for example here:
https://hg.mozilla.org/comm-central/rev/1e38647496ae
What difference does that make? I don't see anything different in the blame

Comment 91

9 months ago
Don't you?
https://hg.mozilla.org/comm-central/annotate/2a29ee0adb310b54a6a2df72034953fed8f2b043/mailnews/addrbook/src/nsAddrDatabase.cpp#l134
That's close to nsAddrDatabase::~nsAddrDatabase().

My changeset doesn't show although the whole thing was indented here:
https://hg.mozilla.org/comm-central/rev/1e38647496ae#l1.12
Ah, there's a "Ignore whitespace changes" box you have to check

Comment 93

9 months ago
OK, well, there seems to be quite some ignorance in this field, including mine. When linting was first discussed, one possible drawback was destroying history. Then someone discovered [skip-blame]. And now you're saying, it's not an issue at all, just check the box. So where does that leave us? Forget about [skip-blame] and tell people to check the box?
Never noticed the box before, perhaps it's new?

Anyway, https://hg.mozilla.org/comm-central/annotate/1e38647496ae/mailnews/addrbook/src/nsAddrDatabase.cpp by default do show the change, and when checking the box it's not shown. I don't know if your annotation made a difference or not.

Comment 95

9 months ago
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.
Assignee

Updated

9 months ago
Attachment #9011980 - Attachment description: 1478572-eslint-compose-3.diff → 1478572-eslint-compose-3.diff [landed in comment #88]
Assignee

Comment 97

9 months ago
Posted patch 1478572-eslint-im-1.diff (obsolete) — Splinter Review
* I don't know why we have an "im" component and a "chat" top-level directory. There's probably a good reason, but I don't know what it is.

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

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

* Similar concerns about imXPCOMUtils.jsm.
Attachment #9013191 - Flags: review?(acelists)

Comment 98

9 months ago
(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

9 months ago
Comment on attachment 9013191 [details] [diff] [review]
1478572-eslint-im-1.diff

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

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

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

Interesting, was this wrong?

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

Nice, always good to remove preprocessing a file.
Is AppConstants.jsm imported into this file?
Attachment #9013191 - Flags: review?(florian)
Attachment #9013191 - Flags: review?(clokep)
Attachment #9013191 - Flags: review?(acelists)
Attachment #9013191 - Flags: feedback+
Assignee

Comment 100

9 months ago
(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.
Assignee

Comment 101

9 months ago
Comment on attachment 9013191 [details] [diff] [review]
1478572-eslint-im-1.diff

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

Found some mistakes. Fixing my copy now…

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

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

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

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

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

Looks good to me, thanks for all the cleanups!

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

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

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

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

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

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

It worked through an obscure quirk of XBL. I think XBL handlers have the binding as their global object. Adding "this." is more explicit anyway.
Attachment #9013191 - Flags: review?(florian)
Attachment #9013191 - Flags: review?(clokep)
Attachment #9013191 - Flags: review+
Assignee

Comment 103

9 months ago
Attachment #9013191 - Attachment is obsolete: true
Attachment #9013462 - Flags: review+
Assignee

Updated

9 months ago
Keywords: checkin-needed

Comment 104

9 months ago
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/826d12190290
Turn on ESLint in mail/components/im; r=florian
Keywords: checkin-needed
Assignee

Updated

9 months ago
Attachment #9013462 - Attachment description: 1478572-eslint-im-2.diff → 1478572-eslint-im-2.diff [landed in comment #104]
Assignee

Comment 105

9 months ago
This was an easy one. I did it in less time than it takes to run mach build. :)
Attachment #9013539 - Flags: review?(acelists)
Comment on attachment 9013539 [details] [diff] [review]
1478572-eslint-cloudfile-1.diff

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

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

can you take care of this else after return while you're at it

Comment 107

9 months ago
Comment on attachment 9013539 [details] [diff] [review]
1478572-eslint-cloudfile-1.diff

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

Thanks.

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

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

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

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

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

Nice, I didn't know they made this not throw and take a default value. Bug 1338306.
We should go in a new bug and simplify the needless try{}catch() blocks around get*Pref().
Attachment #9013539 - Flags: review?(acelists) → review+

Comment 108

9 months ago
Any idea why in MozMill I see:
JavaScript error: resource:///modules/chatHandler.jsm, line 22: NS_ERROR_FILE_NOT_FOUND:

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

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

I was running this one:
mozmake SOLO_TEST=folder-pane/test-folder-pane.js mozmill-one
Flags: needinfo?(geoff)
Assignee

Comment 109

9 months ago
The file was renamed and needs a build to pick up the changes. I expect you've done a build though, so… hmmm.
Flags: needinfo?(geoff)

Comment 110

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

Comment 111

9 months ago
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/dd917dc324b6
Turn on ESLint in mail/components/cloudfile; r=aceman
Assignee

Comment 112

9 months ago
Only two directories remaining!
Attachment #9013539 - Attachment is obsolete: true
Attachment #9013938 - Flags: review+
Assignee

Comment 113

9 months ago
I have a patch for search as well, to complete the set, but I'm going to change things around in there because it's a hive of unnecessarily pre-processed files.
Attachment #9015003 - Flags: review?(acelists)
Assignee

Comment 114

9 months ago
Posted patch 1478572-eslint-search-1.diff (obsolete) — Splinter Review
Here it is. I've made some packaging changes so that this folder is more sensibly organised. Hopefully not broken anything.
Attachment #9015011 - Flags: review?(acelists)
Assignee

Comment 115

9 months ago
(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

9 months ago
Comment on attachment 9015003 [details] [diff] [review]
1478572-eslint-newmailaccount-1.diff

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

Thanks.

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

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

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

Nice.
Attachment #9015003 - Flags: review?(acelists) → review+

Comment 117

9 months ago
Comment on attachment 9015011 [details] [diff] [review]
1478572-eslint-search-1.diff

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

Thanks, nice reorganization in the SearchIntegration. However I can't test it on either Win oe Mac.
Jorg, can you try on Windows?
Attachment #9015011 - Flags: review?(acelists)
Attachment #9015011 - Flags: review+
Attachment #9015011 - Flags: feedback?(jorgk)

Comment 118

9 months ago
Comment on attachment 9015011 [details] [diff] [review]
1478572-eslint-search-1.diff

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

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

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

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

I tried with TB 60 and I was asked to authorise WSEnable.exe and those folders/files were created.
Attachment #9015011 - Flags: feedback?(jorgk) → feedback-
Assignee

Comment 119

9 months ago
Try this not-broken version.
Attachment #9015011 - Attachment is obsolete: true
Attachment #9015151 - Flags: review+
Attachment #9015151 - Flags: feedback?(jorgk)
Assignee

Comment 120

9 months ago
Is this better aceman? Tests for OS X the way we should do it, and has some brackets! I know they're not needed but it makes things clearer.
Attachment #9015003 - Attachment is obsolete: true
Attachment #9015152 - Flags: review+
Attachment #9015152 - Flags: feedback?(acelists)
Assignee

Comment 121

9 months ago
NOW it has some brackets. How did I not notice that I hadn't saved the file?
Attachment #9015152 - Attachment is obsolete: true
Attachment #9015152 - Flags: feedback?(acelists)
Attachment #9015154 - Flags: review+
Attachment #9015154 - Flags: feedback?(acelists)

Comment 122

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

Yes, that works again.
Attachment #9015151 - Flags: feedback?(jorgk) → feedback+

Comment 123

9 months ago
Comment on attachment 9015154 [details] [diff] [review]
1478572-eslint-newmailaccount-3.diff

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

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

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

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

However that is not the same. The original version seems to return true when either meta or ctrl is pressed. Do both exist on OS X? And is that what was wanted?
Attachment #9015154 - Flags: feedback?(acelists) → feedback+
Assignee

Comment 124

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

Comment 125

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

Updated

9 months ago
Keywords: leave-open

Comment 126

9 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/da7cae0efc9d
Turn on ESLint in mail/components/newmailaccount. r=aceman
https://hg.mozilla.org/comm-central/rev/3a92dfd67030
Turn on ESLint in mail/components/search. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED

Comment 127

9 months ago
Landed with
  return AppConstants.platform == "macosx" ? event.metaKey : event.ctrlKey;
as discussed on IRC.
Target Milestone: --- → Thunderbird 64.0
Assignee

Updated

2 months ago
Blocks: 1549166
You need to log in before you can comment on or make changes to this bug.