1264206 - Fix eslint errors in browser/components/downloads

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[[e-mail]]

fix errors while running `./mach eslint --no-ignore browser/components/downloads`

Comment 1

[Mark Banner (:standard8)]

Setting as mentored bug.

To enable checking of files, remove the `browser/components/downloads/**` entry from the top-level `.eslintignore`.

Then you can run eslint on the directory with

  ./mach eslint browser/components/downloads

(note: eslint needs a one-time setup of `./mach eslint --setup`).

The errors listed in the output are the ones to be fixed.

Information about the eslint rules can be found here:

http://eslint.org/docs/rules/

If you are uncertain of how to fix a rule, please feel free to ask here or on irc (https://wiki.mozilla.org/Irc) in #fx-team (I'm on as Standard8).

Comment 2

[Sam Clusker]

Hi Mark,

I will give this a go over the course of today and tomorrow if that's okay.

Thanks,
Sam

Comment 3

[Mark Banner (:standard8)]

Sure, I've assigned it to you.

Comment 4

[Mark Banner (:standard8)]

Sam, are you still working on this?

Comment 5

[Mark Banner (:standard8)]

No response from Sam, so I'm going to assume Sam is not able to work on it, and open it up to others.

Comment 6

[yiren wang]

Hello, I would love to try and work on this, is it possible to know where I can find a guide for me to set up all the necessary tools to lint the code ? 

Thanks a lot :) 
Yiren

Comment 7

[Mark Banner (:standard8)]

Yiren,

If you have the Firefox code base already set up, then it should be quite easy. If not, then you'll need to set it up, here's some instructions:

https://developer.mozilla.org/docs/Mozilla/Developer_guide/Introduction#Step_1_-_Build_Firefox_for_Desktop_or_Android.

You'll also need to setup ESLint and have that running:

https://developer.mozilla.org/docs/ESLint

Once you're there you should be able to follow the steps in comment 1 to fix the bug.

If you need help, feel free to comment here, email or ask in #eslint on irc.

Comment 8

[yiren wang]

Hello, 

I'm corrected the most basic eslint errors but there are still many due to undefined variables. As I have no idea how the code works I don't really understand what I can do to correct it. I can always add the line to disable no-undef rule but that doesn't seems to be a good idea. 

There's an error I don't quite understand as well : "947:13 error Parsing error: Unexpected token for  (eslint)"

the code is : 


  get selectedNodes() {
    return [for (element of this._richlistbox.selectedItems)
        if (element._placesNode)
        element._placesNode];
  },


I understood this as: 
  
  get selectedNodes() {
    const arr = [];
    this._richlistbox.selectedItems.forEach(element => {
        if (element._placesNode) { array.push(element._placesNode) }
    });
    return arr;
  },

Is this correct ? 

Thank you :)

Comment 9

[Mark Banner (:standard8)]

(In reply to yiren wang from comment #8)
> Hello, 
> 
> I'm corrected the most basic eslint errors but there are still many due to
> undefined variables. As I have no idea how the code works I don't really
> understand what I can do to correct it. I can always add the line to disable
> no-undef rule but that doesn't seems to be a good idea. 

There are some extra hints here for no-undef: https://developer.mozilla.org/en-US/docs/ESLint#no-undef

However, if you can't work them out, feel free to post a partial patch, and I'll help you through the remaining issues.

> There's an error I don't quite understand as well : "947:13 error Parsing
> error: Unexpected token for  (eslint)"
...
> I understood this as: 
>   
>   get selectedNodes() {
>     const arr = [];
>     this._richlistbox.selectedItems.forEach(element => {
>         if (element._placesNode) { array.push(element._placesNode) }
>     });
>     return arr;
>   },
> 
> Is this correct ? 

Yes, I think that is correct, however I think you could simplify it further and make it easier to read:

    get selectedNodes() {
      return this._richlistbox.selectedItems.filter(element => element._placesNode);
    }

Comment 10

[yiren wang]

Hello, 

I've finished patching all the lint errors and I wonder if there's a specific documentation for submitting my changes and testing them (should I use xpcshell-test ?). 

Thank you,
Yiren

Comment 11

[Mark Banner (:standard8)]

(In reply to yiren wang from comment #10)
> I've finished patching all the lint errors

Great!

> I wonder if there's a
> specific documentation for submitting my changes and testing them (should I
> use xpcshell-test ?). 

You can run all the tests for downloads via

  ./mach test browser/components/downloads

(just watch as it goes through, sometimes it doesn't make clear that errors have occurred when it gets to the end).

Alternately you can push the patch to mozreview / attach it here, and then I can request all the relevant tests to run on test (try) servers.

Comment 12

[yiren wang]

Attachment: changes to patch eslint errors

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8849067 [details] [diff] [review]
changes to patch eslint errors

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

Hi Yiren,

Thank you for the patch. It is starting to look good.

There's a few comments here, but I think they should be quite easy to fix. Mostly about how we're handling returns and also slightly better ways of dealing with the no-undef values.

Could you also update the patch to include removing the downloads line from the .eslintignore at the top of the file tree please?

::: browser/components/downloads/DownloadsCommon.jsm
@@ +107,4 @@
>      } catch (ex) { }
>      return this.prefs[name];
>    },
> +  observe(aSubject, aTopic, aData) { // eslint-disable-line consistent-return

This function doesn't actually need to return anything, so I think we can just drop the 'return ' from the start of the line three lines below.

::: browser/components/downloads/DownloadsTaskbar.jsm
@@ +109,4 @@
>            return;
>          }
>          this._summary = summary;
> +        return this._summary.addView(this); // eslint-disable-line consistent-return

It looks like addView doesn't return anything, so we can drop the return part here - it wouldn't be used anyway.

::: browser/components/downloads/DownloadsViewUI.jsm
@@ +242,4 @@
>        }
>  
>        let referrer = this.download.source.referrer || this.download.source.url;
> +      let [displayHost, fullHost] = DownloadUtils.getURIHost(referrer); // eslint-disable-line no-unused-vars

Please could you change this to:

 let [displayHost /*, fullHost */ ] = ...

@@ +246,3 @@
>  
>        let date = new Date(this.download.endTime);
> +      let [displayDate, fullDate] = DownloadUtils.getReadableDates(date); // eslint-disable-line no-unused-vars

ditto with this line.

@@ +280,4 @@
>      throw new Error("Unexpected reputationCheckVerdict: " +
>                      this.download.error.reputationCheckVerdict);
>      // return anyway to avoid a JS strict warning.
> +    // return [null, null];

Lets just drop these two comment lines. We shouldn't need them now.

@@ +298,4 @@
>        verdict: this.download.error.reputationCheckVerdict,
>        window,
>        dialogType,
> +    }).then(action => { // eslint-disable-line consistent-return

I'm not sure there's a clean result here, but lets change this so in the default case it returns `Promise.resolve()`

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +1178,4 @@
>    },
>  
>    _copySelectedDownloadsToClipboard() {
> +    let urls = this._richlistbox.selectedItems.forEach(element => element._shell.download.source.url);

I believe this should be map rather than forEach.

@@ +1210,4 @@
>    },
>  
>    _canDownloadClipboardURL() {
> +    let [url, name] = this._getURLFromClipboardData(); // eslint-disable-line no-unused-vars

Please use the /* , name */

::: browser/components/downloads/content/contentAreaDownloadsView.js
@@ +6,4 @@
>  
>  var ContentAreaDownloadsView = {
>    init() {
> +    let view = new DownloadsPlacesView(document.getElementById("downloadsRichListBox")); // eslint-disable-line no-undef

Instead of this, please add this line near the top of the file after the license:

/* import-globals-from allDownloadsViewOverlay.js */


That file gets loaded into the same scope as this one via contentAreaDownloadsView.xul & allDownloadsViewOverlay.xul

::: browser/components/downloads/content/downloads.js
@@ +300,4 @@
>     * Handles the mousemove event for the panel, which disables focusring
>     * visualization.
>     */
> +  handleEvent(aEvent) { // eslint-disable-line consistent-return

handleEvent doesn't actually need to return anything (the value isn't handled), so please change the return statements to normal statements and just add break; after those.

::: browser/components/downloads/test/browser/browser_downloads_panel_block.js
@@ +112,3 @@
>            // Keep backing off before trying again.
>            iBackoff++;
> +      } else if (DownloadsPanel._state != DownloadsPanel.kStateUninitialized) {

This change isn't quite right - it looks like you need to restore this second else section, as the verifyCount changes & show panel (etc), need to be done whenever we hit this else.

At the moment the test hangs due to this.

::: browser/components/downloads/test/browser/browser_downloads_panel_footer.js
@@ +17,4 @@
>    yield task_openDownloadsSubPanel();
>  
>    yield new Promise(resolve => {
> +    sinon.stub(DownloadsCommon, "showDirectory", file => { // eslint-disable-line no-undef

For this, in the head.js in this directory, just above this line:

    Services.scriptloader.loadSubScript("resource://testing-common/sinon-1.16.1.js");

Please add:

    /* global sinon:false */

::: browser/components/downloads/test/browser/head.js
@@ +206,4 @@
>  
>  let gHttpServer = null;
>  function startServer() {
> +  gHttpServer = new HttpServer(); // eslint-disable-line no-undef

Rather than disabling this here, please can you move this line from the start of browser_indicatorDrop.js/browser_libraryDrop.js to near the start of this file (near the similar functions):

XPCOMUtils.defineLazyModuleGetter(this, "HttpServer",
  "resource://testing-common/httpd.js");

Comment 14

[Mark Banner (:standard8)]

Hi Yiren, just so you're aware - when you update the patch, please can you also rebase it on the latest mozilla-central code? There's a few rule changes that have just landed, so we need to make sure we don't fail those as well.

Comment 15

[yiren wang]

Hello, 

Thank you for your corrections. I'll apply them this weekend. What exactly is the procedure to update the patch ? (hg pull --rebase ?)

Comment 16

[Mark Banner (:standard8)]

(In reply to yiren wang from comment #15)
> Hello, 
> 
> Thank you for your corrections. I'll apply them this weekend. What exactly
> is the procedure to update the patch ? (hg pull --rebase ?)

I believe that should work.

Comment 17

[yiren wang]

Attachment: updated patch

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8851287 [details] [diff] [review]
updated patch

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

This is looking much better. There's just two extra items where I think I wasn't clear enough on my original review.

If you could address those, and remove the `browser/components/downloads/**` line in .eslintignore, then I think this will be good to go :-)

::: browser/components/downloads/DownloadsCommon.jsm
@@ -111,4 @@
>    observe(aSubject, aTopic, aData) {
>      if (this.prefs.hasOwnProperty(aData)) {
>        delete this[aData];
> -      return this[aData] = this.getPref(aData);

Sorry, I wasn't clear enough here, just the return part of this line should be dropped, e.g.:

    this[aData] = this.getPref(aData);

::: browser/components/downloads/test/browser/browser_downloads_panel_block.js
@@ +118,5 @@
>            iBackoff = 0;
>            if (DownloadsPanel._state != DownloadsPanel.kStateUninitialized) {
> +              DownloadsPanel._state = DownloadsPanel.kStateHidden;
> +          } else {
> +              DownloadsPanel.showPanel();

Again, I think I wasn't quite clear enough here, this part should be:

```
if (DownloadsPanel._state != DownloadsPanel.kStateUninitialized) {
    DownloadsPanel._state = DownloadsPanel.kStateHidden;	
    DownloadsPanel._state = DownloadsPanel.kStateHidden;
}
DownloadsPanel.showPanel();
```

Hence, showPanel is always called.

Comment 19

[yiren wang]

Attachment: patch_diff.txt

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 8853772 [details] [diff] [review]
patch_diff.txt

I guess you meant to request review for this - I'll take a look at it in a bit.

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 8853772 [details] [diff] [review]
patch_diff.txt

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

Thank you for the update. This looks good.

The tests seem to run fine now, so I've pushed this to our test servers to check for anything I might have missed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fa4722635389e3808c7d96fc5139b07f3cb7d30

Once they've finished, I'll push this to the integration tree for you.

For future reference, it is normally useful to include a patch header on your patch, which includes you as the author, and a nice commit message. I've put an example below. For this landing I've got that already added locally, so I'll just push with that included.


# HG changeset patch
# User Yiren Wang <[e-mail]>
# Date 1491211505 -3600
# Node ID ba5d312fa622ccf1faddf70c33c550caa4354fa9
# Parent  38894655c89e68bcd8f45d31a0d3005f2c2b53db
Bug 1264206 - Enable ESLint for browser/components/downloads and fix the existing issues. r=Standard8

Comment 22

[Mark Banner (:standard8)]

Try servers looked nice and green, so I'll land this in a moment. Thank you for all your work.

Comment 23

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba78fb79365f3bcff0575568846fde62f0e0b8a0
Bug 1264206 - Enable ESLint for browser/components/downloads and fix the existing issues. r=Standard8

Comment 24

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

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

Comment 25

[yiren wang]

Thank you for all the help! I'll take note for the next time.

Comment 26

[:Paolo Amadini]

(In reply to Mark Banner (:standard8) from comment #13)
> > +        return this._summary.addView(this); // eslint-disable-line consistent-return
> 
> It looks like addView doesn't return anything, so we can drop the return
> part here - it wouldn't be used anyway.

Note that it does return a Promise, so the "return" is needed for correct chaining.