1256772 - [ESLint] Fix ESLint issues in devtools/client/webconsole/jsterm.js

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Lin Clark [:linclark]]

If you haven’t contributed to Firefox before, follow the steps here to set up your environment: https://developer.mozilla.org/en-US/docs/Tools/Contributing#Getting_set_up

Then, automatically configure ESLint to work with the Firefox specific rules by following the instructions here: https://wiki.mozilla.org/DevTools/CodingStandards

Then you can see the issues that need to be fixed by running

> eslint --no-ignore devtools/client/webconsole/jsterm.js

Comment 1

[[e-mail]]

took it

Comment 2

[[e-mail]]

Attachment: MozReview Request: Bug 1256772 - Fix ESLint issues in devtools/client/webconsole/jsterm.js; r?linclark

Review commit: https://reviewboard.mozilla.org/r/44747/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44747/

Comment 3

[[e-mail]]

Comment on attachment 8738890 [details]
MozReview Request: Bug 1256772 - Fix ESLint issues in devtools/client/webconsole/jsterm.js; r?linclark

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44747/diff/1-2/

Comment 4

[Lin Clark [:linclark]]

Comment on attachment 8738890 [details]
MozReview Request: Bug 1256772 - Fix ESLint issues in devtools/client/webconsole/jsterm.js; r?linclark

https://reviewboard.mozilla.org/r/44747/#review41631

Nice! Just a little nit and it should be ready.

I see that you can run try jobs. It looks like the try job ran the bc suite but not the devtools suite. If you use mochitest-dt and mochitest-e10s-devtools-chrome, it should run our test suite.

::: devtools/client/webconsole/jsterm.js:411
(Diff revision 2)
>      };
>  
>      // attempt to execute the content of the inputNode
>      executeString = executeString || this.getInputValue();
>      if (!executeString) {
> -      return;
> +      deferred.resolve();

I'd say return null here instead of resolving the promise. That will keep it more in line with the current code, but should still get rid of the ESLint error.

Comment 5

[[e-mail]]

Comment on attachment 8738890 [details]
MozReview Request: Bug 1256772 - Fix ESLint issues in devtools/client/webconsole/jsterm.js; r?linclark

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44747/diff/2-3/

Comment 6

[Lin Clark [:linclark]]

Comment on attachment 8738890 [details]
MozReview Request: Bug 1256772 - Fix ESLint issues in devtools/client/webconsole/jsterm.js; r?linclark

https://reviewboard.mozilla.org/r/44747/#review42047

It looks like there were two unexpected failures in the devtools tests on Linux. It would be worth pulling down changes and updating the branch and then running the tests again... it could be that these test failures are unrelated to your changes. Other than that, this looks good!

Comment 7

[[e-mail]]

Comment on attachment 8738890 [details]
MozReview Request: Bug 1256772 - Fix ESLint issues in devtools/client/webconsole/jsterm.js; r?linclark

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44747/diff/3-4/

Comment 8

[[e-mail]]

Thanks for double check. I cherry-pick the commit to latest gecko and push a new try to see how it progress.

Comment 9

[Lin Clark [:linclark]]

Comment on attachment 8738890 [details]
MozReview Request: Bug 1256772 - Fix ESLint issues in devtools/client/webconsole/jsterm.js; r?linclark

https://reviewboard.mozilla.org/r/44747/#review42703

It's odd that try is showing that the ESLint job is failing for this, but not showing any errors in this file. 

I tried to download and apply the patch, but it doesn't apply. Sorry to ask, but could you try updating your code and pushing to try again? For the try push, you only need to include these two groups of tests: mochitest-dt,mochitest-e10s-devtools-chrome

Comment 10

[[e-mail]]

I found 2 issues here, 

1st is the conflict with .eslintignore file (It happens because other devtool eslint fix landed to inbound). 

2nd comes with some new eslint rules enabled during review. So I need do `./mach eslint --setup` to trigger those new rules. 

Will send new PR once the patch is ready.

Comment 11

[[e-mail]]

Comment on attachment 8738890 [details]
MozReview Request: Bug 1256772 - Fix ESLint issues in devtools/client/webconsole/jsterm.js; r?linclark

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44747/diff/4-5/

Comment 12

[[e-mail]]

https://reviewboard.mozilla.org/r/44747/#review42813

::: devtools/client/webconsole/jsterm.js:318
(Diff revision 5)
>      let errorDocLink;
>      if (errorDocURL) {
>        errorMessage += " ";
>        errorDocLink = this.hud.document.createElementNS(XHTML_NS, "a");
>        errorDocLink.className = "learn-more-link webconsole-learn-more-link";
> -      errorDocLink.textContent = "[" + l10n.getStr("webConsoleMoreInfoLabel") + "]";
> +      errorDocLink.textContent = `[${l10n.getStr("webConsoleMoreInfoLabel")}]`;

use template liternal to shorten the line length

::: devtools/client/webconsole/jsterm.js:1031
(Diff revision 5)
> -   * The inputNode "keypress" event handler.
> +   * The inputNode "keypress" charCode handler.
> -   *
>     * @private
>     * @param nsIDOMEvent event
>     */
> -  _keyPress: function(event) {
> +  _keyPressCharCodeHandler(event) {

I initially separate _keyPress functions with keyPressCharCodeHandler, keyPressKeyCodeHandler to reduce eslint complexity to 37, then add _keyPressKeyCodeDomVKRightHandler to reduce complexity under 20 to pass eslint check

::: devtools/client/webconsole/jsterm.js:1242
(Diff revision 5)
>          if (this.autocompletePopup.isOpen || this.lastCompletion.value) {
>            this.clearCompletion();
>          }
>          break;
>  
> -      case Ci.nsIDOMKeyEvent.DOM_VK_RIGHT: {
> +      case Ci.nsIDOMKeyEvent.DOM_VK_RIGHT:

remove redundant curly brace

Comment 13

[Lin Clark [:linclark]]

Comment on attachment 8738890 [details]
MozReview Request: Bug 1256772 - Fix ESLint issues in devtools/client/webconsole/jsterm.js; r?linclark

https://reviewboard.mozilla.org/r/44747/#review43091

::: devtools/client/webconsole/jsterm.js
(Diff revision 5)
> -  _keyPress: function(event) {
> +  _keyPressCharCodeHandler(event) {
>      let inputNode = this.inputNode;
>      let inputValue = this.getInputValue();
> -    let inputUpdated = false;
>  
> -    if (event.ctrlKey) {

Removing this wrapping if changes the functionality. For example, it would mean that the code under case 101 would fire if you pressed "e" (instead of "Ctrl-e").

The fact that this passed tests makes me worry that we probably aren't testing these at all. If we aren't, I'd rather not touch this code too much until we add test coverage.

How about we fix all of the issues besides the complexity. I've never tried it, but you should be able to add an ignore block around the function like this:

> @TODO Fix complexity once this code is properly tested.
> /*eslint-disable */
> _keyPressCharCodeHandler(event) {
> .... rest of function
> /*eslint-enable */

Comment 14

[[e-mail]]

Comment on attachment 8738890 [details]
MozReview Request: Bug 1256772 - Fix ESLint issues in devtools/client/webconsole/jsterm.js; r?linclark

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44747/diff/5-6/

Comment 15

[[e-mail]]

https://reviewboard.mozilla.org/r/44747/#review43091

> Removing this wrapping if changes the functionality. For example, it would mean that the code under case 101 would fire if you pressed "e" (instead of "Ctrl-e").
> 
> The fact that this passed tests makes me worry that we probably aren't testing these at all. If we aren't, I'd rather not touch this code too much until we add test coverage.
> 
> How about we fix all of the issues besides the complexity. I've never tried it, but you should be able to add an ignore block around the function like this:
> 
> > @TODO Fix complexity once this code is properly tested.
> > /*eslint-disable */
> > _keyPressCharCodeHandler(event) {
> > .... rest of function
> > /*eslint-enable */

Thanks for suggestion. I keep _keyPress function unchanged and wrap it with eslint-disable/eslint-enable comments

Comment 16

[[e-mail]]

Comment on attachment 8738890 [details]
MozReview Request: Bug 1256772 - Fix ESLint issues in devtools/client/webconsole/jsterm.js; r?linclark

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44747/diff/6-7/

Comment 17

[[e-mail]]

Comment on attachment 8738890 [details]
MozReview Request: Bug 1256772 - Fix ESLint issues in devtools/client/webconsole/jsterm.js; r?linclark

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44747/diff/7-8/

Comment 18

[Lin Clark [:linclark]]

LGTM, thanks!

Comment 19

[Lin Clark [:linclark]]

Comment on attachment 8738890 [details]
MozReview Request: Bug 1256772 - Fix ESLint issues in devtools/client/webconsole/jsterm.js; r?linclark

https://reviewboard.mozilla.org/r/44747/#review45681

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d58135ead59c

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d58135ead59c