Enable eslint of browser/components/privatebrowsing/

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: standard8, Assigned: standard8, Mentored)

Tracking

unspecified
Firefox 54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(3 attachments, 1 obsolete attachment)

To help with preventing errors in coding, we should enable eslint in browser/components/privatebrowsing/

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

Then you can run eslint on the directory with

  ./mach eslint browser/components/privatebrowsing/

(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).
Mark, please assign this bug to me & review the submitted patch. Thanks!
Assignee: nobody → souravgarg833
Comment on attachment 8805234 [details]
Bug 1311345 - Enable eslint of browser/components/privatebrowsing/;

https://reviewboard.mozilla.org/r/89006/#review88396

Thank you for attempting this.

Unfortunately the browser_privatebrowsing_DownloadLastDirWithCPS.js test doesn't run correctly. I've left some notes on how to fix that below.

The rest of the changes look good though, but I'll give them another pass once the test is fixed.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js:58
(Diff revision 1)
>   * ===================
>   * Function with tests
>   * ===================
>   */
>  
> -function runTest() {
> +function* runTest() {

I tried applying this change, but it doesn't actually work - the test just hangs.

I think the problem is, that this test is effectively trying to spin its own generator handling.

I believe what you need to do is:

- Drop the test() function, except keep the requestLongerTimeout.
- Where moveAlong is called, change those functions to return a promise, similar in style to this function: https://dxr.mozilla.org/mozilla-central/rev/944cb0fd05526894fcd90fbe7d1e625ee53cd73d/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_urlbarfocus.js#18
- Change the line `function* runTest()` to `add_task(function* () {` (and add a closing bracket at the end).

I think that will give you enough to get the test working again. If you've built Firefox, then you can run the test with `./mach test browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js`.

Note: you can do artifact builds to speed up build times - note they download binary files - see http://www.ncalexander.net/blog/2015/12/31/firefox-artifact-builds-for-mac/ for more info.

Also, once you've built once, you shouldn't have to rebuild when you're modifying the test, just run it again.
Attachment #8805234 - Flags: review?(standard8) → review-
(In reply to Mark Banner (:standard8, afk until Dec) from comment #3)
> - Drop the test() function, except keep the requestLongerTimeout.

should I completely drop test function, and move requestLongerTimeout outside the function?

> - Where moveAlong is called, change those functions to return a promise,

How to deal with executeSoon(moveAlong)? According to my knowledge, executeSoon() is used to briefly delay a event. I wonder why moveAlong is passed here without any parameter.


I tried everything you told but it is still producing same error. Anyway I'll try again.
(In reply to Sourav Garg [:jordan] from comment #4)
> (In reply to Mark Banner (:standard8, afk until Dec) from comment #3)
> > - Drop the test() function, except keep the requestLongerTimeout.
> 
> should I completely drop test function, and move requestLongerTimeout
> outside the function?

Move it into runTest() please.

> 
> > - Where moveAlong is called, change those functions to return a promise,
> 
> How to deal with executeSoon(moveAlong)? According to my knowledge,
> executeSoon() is used to briefly delay a event. I wonder why moveAlong is
> passed here without any parameter.

I think you can just use `yield;` instead, to let the queue clear.

> I tried everything you told but it is still producing same error. Anyway
> I'll try again.

If you still have problems, upload what you have, and I'll take another look.
Sourav has mentioned to me he's too busy to drive this through at the moment, so I've picked it up and de-rotted the patch, as well as fix the remaining issues that have appeared since this was first created.
Assignee: souravgarg833 → standard8
Attachment #8805234 - Attachment is obsolete: true
Comment on attachment 8841955 [details]
Bug 1311345 - Enable eslint of browser/components/privatebrowsing/ - Initial changes;

https://reviewboard.mozilla.org/r/116004/#review117396
Attachment #8841955 - Flags: review?(standard8) → review+
Comment on attachment 8841956 [details]
Bug 1311345 - Enable eslint of browser/components/privatebrowsing/ - Automatically fixed changes by eslint.

https://reviewboard.mozilla.org/r/116006/#review117452

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js:176
(Diff revision 1)
> -  //is(Services.contentPrefs.hasPref(uri1, "browser.download.lastDir", null),
> +  // is(Services.contentPrefs.hasPref(uri1, "browser.download.lastDir", null),
>    //   false, "LastDir preference should be absent");

I guess we should just delete these two lines?
Attachment #8841956 - Flags: review?(jaws) → review+
Comment on attachment 8841957 [details]
Bug 1311345 - Enable eslint of browser/components/privatebrowsing/ - Final manual fixes.

https://reviewboard.mozilla.org/r/116008/#review117458

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_geoprompt.js:21
(Diff revision 1)
>          yield new Promise(resolve => { executeSoon(resolve); });
> -        let notification = aWindow.PopupNotifications.getNotification("geolocation");
> +        notification = aWindow.PopupNotifications.getNotification("geolocation");

Huh, I'm curious as to how this ever worked.
Attachment #8841957 - Flags: review?(jaws) → review+
Comment on attachment 8841957 [details]
Bug 1311345 - Enable eslint of browser/components/privatebrowsing/ - Final manual fixes.

https://reviewboard.mozilla.org/r/116008/#review117458

> Huh, I'm curious as to how this ever worked.

`notification` is set in a higher scope, so this is a just in case while loop. I'm guessing that it is pretty much always there straight away. I'll leave the code in, as it may get hit occasionally.
Comment on attachment 8841956 [details]
Bug 1311345 - Enable eslint of browser/components/privatebrowsing/ - Automatically fixed changes by eslint.

https://reviewboard.mozilla.org/r/116006/#review117452

> I guess we should just delete these two lines?

Yep, lets do that.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4964c35974f4
Enable eslint of browser/components/privatebrowsing/ - Initial changes; r=standard8
https://hg.mozilla.org/integration/autoland/rev/a98e615cb53e
Enable eslint of browser/components/privatebrowsing/ - Automatically fixed changes by eslint. r=jaws
https://hg.mozilla.org/integration/autoland/rev/90cba103308a
Enable eslint of browser/components/privatebrowsing/ - Final manual fixes. r=jaws
You need to log in before you can comment on or make changes to this bug.