1542666 - Turn on ESLint in editor

Status: RESOLVED FIXED

(MailNews Core :: Composition, task)

Description

[Geoff Lankow (:darktrojan)]

I've done the linting in editor, ignoring most of the parts Thunderbird doesn't ship.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1542666-eslint-editor-auto-1.diff

These changes were done by eslint --fix. Not asking for review, as nobody who values their sanity wants to review a computer-generated 22000-line patch.

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1542666-eslint-editor-manual-1.diff

Good luck, and remember, most of this code never sees the light of day, so don't be too picky.

Comment 3

[Geoff Lankow (:darktrojan)]

Here's a Try run I prepared earlier.

Comment 4

[:aceman]

Comment on attachment 9056449 [details] [diff] [review]
1542666-eslint-editor-manual-1.diff

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

Thanks.

::: editor/ui/composer/content/ComposerCommands.js
@@ +1443,5 @@
>    }
>    return false;
>  }
>  
> +/* eslint-disable */

Disable whole linting? Why?

@@ +1729,5 @@
>      goDoCommand(command);
>    }
>  }
>  
> +/* eslint-disable */

Again?

::: editor/ui/composer/content/editor.js
@@ -556,5 @@
>        return false;
>      }
>  
>      // Save to local disk
> -    return await SaveDocument(false, false, editor.contentsMIMEType);

Was this 'await' wrong? Does eslint find this stuff?

@@ +2788,5 @@
>    var editor = GetCurrentEditor();
>    if (!editor) return null;
>  
>    try {
>      var selection = editor.selection;

'selection' is declared int he block, but used outside.

::: editor/ui/composer/content/editorUtilities.js
@@ -279,5 @@
>  /** *********** General editing command utilities ***************/
>  
>  function GetDocumentTitle() {
>    try {
> -    return new XPCNativeWrapper(GetCurrentEditor().document, "title").title;

Found by eslint?

::: editor/ui/dialogs/content/EdConvertToTable.js
@@ +286,5 @@
>              editor.selection.collapse(node2, 0);
>            } catch (e) {}
>            break;
>          }
> +        node2 = node2.nextSibling;

Heh, nice.

::: editor/ui/dialogs/content/EdDialogTemplate.js
@@ +22,5 @@
>    // gDialog is declared in EdDialogCommon.js
>    // Set commonly-used widgets like this:
>    gDialog.fooButton = document.getElementById("fooButton");
>  
> +  InitDialog();

Nice ;)

::: editor/ui/dialogs/content/EdReplace.js
@@ +70,5 @@
>    // get the find service, which stores global find state
>      gFindService = Cc["@mozilla.org/find/find_service;1"]
>                           .getService(Ci.nsIFindService);
> +  } catch (e) {
> +    dump("No find service!\n"); gFindService = 0;

2 commands

@@ +334,5 @@
>          selecRange = selection.getRangeAt(0);
>        }
>      }
> +  } catch (e) {
> +  }

At other places you left this as 'catch (e) {}'.

::: editor/ui/dialogs/content/EdTableProps.js
@@ -657,5 @@
>          gActiveEditor.selectTableColumn();
>          break;
>      }
> -    // Get number of cells selected
> -    var tableOrCellElement = gActiveEditor.getSelectedOrParentTableElement(tagNameObj, countObj);

The countObj is used below. Maybe you only wanted to kill tableOrCellElement ?

Comment 5

[Geoff Lankow (:darktrojan)]

(In reply to :aceman from comment #4)

Disable whole linting? Why?
Again?

I said why, in the very first chunk of the diff. We don't use this code (I wish it would be chopped out and put in another file, but that's not going to happen anytime soon) and it's not worth my time hunting down the errors in it.

::: editor/ui/composer/content/editor.js
@@ -556,5 @@

• return await SaveDocument(false, false, editor.contentsMIMEType);

Was this 'await' wrong? Does eslint find this stuff?

Not wrong, just pointless. The async function returns a Promise, and SaveDocument returns a Promise. There's probably no difference at all at runtime.

::: editor/ui/composer/content/editorUtilities.js
@@ -279,5 @@

/** *********** General editing command utilities ***************/

function GetDocumentTitle() {
try {

• return new XPCNativeWrapper(GetCurrentEditor().document, "title").title;

Found by eslint?

Yeah, I don't think XPCNativeWrapper is really a thing any more. Anyway, I just accessed the document the same way SetDocumentTitle does, so I think this okay.

::: editor/ui/dialogs/content/EdTableProps.js
@@ -657,5 @@

     gActiveEditor.selectTableColumn();
     break;
 }

• // Get number of cells selected
• var tableOrCellElement = gActiveEditor.getSelectedOrParentTableElement(tagNameObj, countObj);

The countObj is used below. Maybe you only wanted to kill tableOrCellElement
?

Well spotted, I didn't realise that was an out-arg.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2f1b233c4f65
Turn on ESLint in editor (automatic changes by eslint --fix); rs=me
https://hg.mozilla.org/comm-central/rev/09d757485ede
Turn on ESLint in editor (manual changes); r=aceman