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

RESOLVED FIXED in Firefox 49

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: linclark, Assigned: gasolin, Mentored)

Tracking

unspecified
Firefox 49
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [btpp-backlog])

Attachments

(1 attachment)

Reporter

Description

3 years ago
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
Reporter

Updated

3 years ago
Blocks: 1256948
Priority: -- → P3
Reporter

Updated

3 years ago
Whiteboard: [btpp-backlog]
Assignee

Comment 1

3 years ago
took it
Assignee: nobody → gasolin
Assignee

Comment 3

3 years ago
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/
Reporter

Comment 4

3 years ago
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.
Attachment #8738890 - Flags: review?(lclark)
Assignee

Comment 5

3 years ago
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/
Attachment #8738890 - Flags: review?(lclark)
Reporter

Comment 6

3 years ago
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!
Attachment #8738890 - Flags: review?(lclark)
Assignee

Comment 7

3 years ago
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/
Attachment #8738890 - Flags: review?(lclark)
Assignee

Comment 8

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

Updated

3 years ago
Status: NEW → ASSIGNED
Reporter

Comment 9

3 years ago
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
Attachment #8738890 - Flags: review?(lclark)
Assignee

Comment 10

3 years ago
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.
Assignee

Comment 11

3 years ago
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/
Attachment #8738890 - Flags: review?(lclark)
Assignee

Comment 12

3 years ago
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
Reporter

Updated

3 years ago
Attachment #8738890 - Flags: review?(lclark)
Reporter

Comment 13

3 years ago
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 */
Assignee

Comment 14

3 years ago
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/
Attachment #8738890 - Flags: review?(lclark)
Assignee

Comment 15

3 years ago
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
Assignee

Comment 16

3 years ago
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/
Assignee

Comment 17

3 years ago
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/
Reporter

Comment 18

3 years ago
LGTM, thanks!
Keywords: checkin-needed
Reporter

Comment 19

3 years ago
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
Attachment #8738890 - Flags: review?(lclark) → review+

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d58135ead59c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.