Closed Bug 1264206 Opened 8 years ago Closed 7 years ago

Fix eslint errors in browser/components/downloads

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox48 --- affected
firefox55 --- fixed

People

(Reporter: gasolin, Assigned: yiren.wang, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 2 obsolete files)

fix errors while running `./mach eslint --no-ignore browser/components/downloads`
Depends on: 1309842
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).
Mentor: standard8
Whiteboard: [lang=js]
Blocks: 1311338
Hi Mark,

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

Thanks,
Sam
Flags: needinfo?(standard8)
Sure, I've assigned it to you.
Assignee: nobody → sclusker
Flags: needinfo?(standard8)
Sam, are you still working on this?
Flags: needinfo?(sclusker)
No response from Sam, so I'm going to assume Sam is not able to work on it, and open it up to others.
Assignee: sclusker → nobody
Flags: needinfo?(sclusker)
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
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.
Assignee: nobody → 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 :)
(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);
    }
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
(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.
Attached patch changes to patch eslint errors (obsolete) — Splinter Review
Attachment #8849067 - Flags: review?(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");
Attachment #8849067 - Flags: review?(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.
Hello, 

Thank you for your corrections. I'll apply them this weekend. What exactly is the procedure to update the patch ? (hg pull --rebase ?)
(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.
Attached patch updated patch (obsolete) — Splinter Review
Attachment #8851287 - Flags: review?(standard8)
Attachment #8851287 - Attachment is patch: true
Attachment #8849067 - Attachment is obsolete: true
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.
Attachment #8851287 - Flags: review?(standard8)
Attached patch patch_diff.txtSplinter Review
Attachment #8851287 - Attachment is obsolete: true
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.
Attachment #8853772 - Flags: review?(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 <yiren.wang@student.ecp.fr>
# Date 1491211505 -3600
# Node ID ba5d312fa622ccf1faddf70c33c550caa4354fa9
# Parent  38894655c89e68bcd8f45d31a0d3005f2c2b53db
Bug 1264206 - Enable ESLint for browser/components/downloads and fix the existing issues. r=Standard8
Attachment #8853772 - Flags: review?(standard8) → review+
Try servers looked nice and green, so I'll land this in a moment. Thank you for all your work.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba78fb79365f3bcff0575568846fde62f0e0b8a0
Bug 1264206 - Enable ESLint for browser/components/downloads and fix the existing issues. r=Standard8
https://hg.mozilla.org/mozilla-central/rev/ba78fb79365f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Thank you for all the help! I'll take note for the next time.
Depends on: 1353374
(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.
Depends on: 1357915
You need to log in before you can comment on or make changes to this bug.