1256771 - [ESLint] Fix ESLint issues in devtools/client/webconsole/hudservice.js

Status: RESOLVED DUPLICATE of bug 1436076

(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/hudservice.js

Comment 1

[simplyblue24]

Attachment: 1256771.patch

Hi, Can you please review the patch.Thanks.

Comment 2

[Lin Clark [:linclark]]

This looks good. I'm just going to run it through our test server to make sure. If the tests pass, this will be good to go.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af16cd22a0c4

Comment 3

[Lin Clark [:linclark]]

Comment on attachment 8739348 [details] [diff] [review]
1256771.patch

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

It looks like there are some test failures which need to be resolved before we land this. You can see what tests are failing by clicking on that link and clicking the orange dt8. A list of tests will show up at the bottom. You can try running the tests locally using ./mach test <path/to/test>

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8739348 [details] [diff] [review]
1256771.patch

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

::: devtools/client/webconsole/hudservice.js
@@ +349,5 @@
>     *
>     * @return object
>     *         A promise for the initialization.
>     */
> +  init: function WC_init() {

You can remove the additional function name here and elsewhere in the file (WC_init in this case). It used to be needed to get good stack traces but it isn't anymore.  And there's cases where it's causing a line wrap to avoid the 80 char limit so I'd rather they all get deleted while you are at it.  Thanks!

Comment 5

[[:Towkir] Ahmed]

Hi, 
Can I take this one ? Looks like it's been inactive for a while.

Comment 6

[Lin Clark [:linclark]]

Yes, that sounds good. Thanks for offering.

Comment 7

[[:Towkir] Ahmed]

Attachment: hudservice_eslint_fix.patch

Hi Brian & Lin,
I have tried to fix some of the errors and now there are some remaining about which I am little confused. Can you please help on some errors ? 

> 28:1   error  Line 28 exceeds the maximum line length of 90            max-len
If I use multiline strings there is another eslint errors, but I think another way should be out there except for using multiline strings
> 121:15  error  'hudId' is defined but never used             no-unused-vars
Shall I remove this ? if so, then how ?
> 174:11  error  'deferred' is defined but never used          no-unused-vars
If I remove this, how ?
> 219:16  error  'BrowserConsole' is already declared in the upper scope  
> no-shadow
what to do with this ?
> 407:24  error  Expected to return a value at the end of this function   
> consistent-return
Shall I add a 'return' at the starting of line (in the patch) 417 ? 
> 417:1   error  Line 417 exceeds the maximum line length of 90        
> max-len
Well, this is not a string. How do I write this in multiple line ? 

Please provide your valuable suggestion on this, and If I have done anything wrong in the patch, correct me please .

(The patch is just a diff file, so it contains no patch header, after your comments, I will update and attach the final one)

Thanks

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to [:Towkir] Ahmed from comment #7)
> Created attachment 8783327 [details] [diff] [review]
> hudservice_eslint_fix.patch
> 
> Hi Brian & Lin,
> I have tried to fix some of the errors and now there are some remaining
> about which I am little confused. Can you please help on some errors ? 
> 
> > 28:1   error  Line 28 exceeds the maximum line length of 90            max-len
> If I use multiline strings there is another eslint errors, but I think
> another way should be out there except for using multiline strings

Not a fan of us having to do this just for eslint, but you could split into two lines:

const BROWSER_CONSOLE_WINDOW_FEATURES =
  "chrome,titlebar,toolbar,centerscreen,resizable,dialog=no";

> > 121:15  error  'hudId' is defined but never used             no-unused-vars
> Shall I remove this ? if so, then how ?

You should be able to remove it by doing [,hud] instead

> > 174:11  error  'deferred' is defined but never used          no-unused-vars
> If I remove this, how ?

If it's unused you should be able to just remove it

> > 219:16  error  'BrowserConsole' is already declared in the upper scope  
> > no-shadow
> what to do with this ?

Rename the argument BrowserConsole passed into the arrow function as something else, like browserConsole.

> > 407:24  error  Expected to return a value at the end of this function   
> > consistent-return
> Shall I add a 'return' at the starting of line (in the patch) 417 ? 

You should be able to reformat that as:

      browserWin.BrowserViewSourceOfDocument({
        URL: SourceURL,
        lineNumber: SourceLine
      });
      return;

> > 417:1   error  Line 417 exceeds the maximum line length of 90        
> > max-len
> Well, this is not a string. How do I write this in multiple line ? 

You can just split it up like:

    this.gViewSourceUtils.viewSource(SourceURL, null, this.iframeWindow.document,
        SourceLine || 0);

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8783327 [details] [diff] [review]
hudservice_eslint_fix.patch

See Comment 8

Comment 10

[[:Towkir] Ahmed]

Attachment: hudservice_eslint_fix.patch

I already had the patch without eslint errors with help of Nicolas Chevobbe.
Brian, please comment some docs link to learn to push to try after reviewing this. I tried to push to try but could not, I have level 1 access.
Thanks

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8784417 [details] [diff] [review]
hudservice_eslint_fix.patch

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

Please rebase this and make sure this file is included in the .eslintignore file with: !devtools/client/webconsole/hudservice.js

Comment 12

[[:Towkir] Ahmed]

Attachment: hudservice_eslint_fix.patch

Hi, sorry for being late. Hope this works :)

Comment 13

[Brian Grinstead [:bgrins]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec93dd3e4c17

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8788191 [details] [diff] [review]
hudservice_eslint_fix.patch

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

There is at least one error coming through with this applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec93dd3e4c17&selectedJob=27540703 

TEST-UNEXPECTED-FAIL | devtools/client/scratchpad/test/browser_scratchpad_open_error_console.js | A promise chain failed to handle a rejection: - at resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/hudservice.js:658 - TypeError: this.target.client is null

Please check on any variable renames that all references to those variables are changed

::: devtools/client/webconsole/hudservice.js
@@ +616,1 @@
>      if (this._bc_init) {

All references of _bc_init should be replaced with bcInit

@@ +654,1 @@
>      if (this._bc_destroyer) {

All references of _bc_destroyer should be replaced with bcDestroyer

Comment 15

[Brian Grinstead [:bgrins]]

Going to handle this in Bug 1436076