Closed Bug 1412890 Opened 3 years ago Closed 3 years ago

Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr

Categories

(Toolkit :: Password Manager, defect, P3)

Product:
Toolkit
Component:
Password Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file)

Bug 1412890 - Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr.
59 bytes, text/x-review-board-request
MattN
: review+
Details 
I'm working on rolling out mozilla/use-services across the tree. passwordmgr has a good chunk of changes to do, and I've seen a few issues with tests when changing it.

Hence, separating it out into a separate bug.
Comment on attachment 8923474 [details]
Bug 1412890 - Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr.

https://reviewboard.mozilla.org/r/194616/#review199610

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-services.js:28
(Diff revision 1)
>    // These interfaces are difficult/not possible to get via processing.
>    result: {
>      "nsIPrefBranch": "prefs",
>      "nsIPrefService": "prefs",
> -    "nsIXULRuntime": "appInfo",
> -    "nsIXULAppInfo": "appInfo",
> +    "nsIXULRuntime": "appinfo",
> +    "nsIXULAppInfo": "appinfo",

Note: this is a correctness thing I noticed whilst working on deploying the rule, the only effect is on the ESLint error message will show the correct replacement to use.
Blocks: 1412893
I've seen the test failures in test_autofill_password-only.html on try server - I'll look at those in the morning.
I fixed the issue - Services was out of scope in one of the functions that gets run in a parent process.
Comment on attachment 8923474 [details]
Bug 1412890 - Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr.

https://reviewboard.mozilla.org/r/194616/#review200222

My only concern is about tests which are skipped/disabled either permanently or in e10s. Could you verify that those tests still pass (if they did before your change)?

::: toolkit/components/passwordmgr/test/mochitest/test_prompt_http.html:32
(Diff revision 3)
>  
>  let chromeScript = runInParent(SimpleTest.getTestFileURL("pwmgr_common.js"));
>  
>  runInParent(() => {
>    const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
>    Cu.import("resource://gre/modules/Services.jsm");

Should this also be destructured to make the scope clear to tools

::: toolkit/components/passwordmgr/test/pwmgr_common.js:131
(Diff revision 3)
> -  var pwmgr = SpecialPowers.Cc["@mozilla.org/login-manager;1"].
> -              getService(SpecialPowers.Ci.nsILoginManager);
> -  ok(pwmgr != null, "Access LoginManager");
> +  // var pwmgr = SpecialPowers.Cc["@mozilla.org/login-manager;1"].
> +  //             getService(SpecialPowers.Ci.nsILoginManager);
> +  // ok(pwmgr != null, "Access LoginManager");

You can delete this

::: toolkit/components/passwordmgr/test/test_master_password.html:20
(Diff revision 3)
> +// eslint-disable-next-line mozilla/use-services
>  var pwmgr   = SpecialPowers.Cc["@mozilla.org/login-manager;1"]
>                             .getService(SpecialPowers.Ci.nsILoginManager);

Ideally the rule would be smart enough to handle this
Attachment #8923474 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8923474 [details]
Bug 1412890 - Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr.

https://reviewboard.mozilla.org/r/194616/#review200222

Thanks for the suggestion, looks like there was only one that was skipped for e10s, and I've fixed that up.

> Should this also be destructured to make the scope clear to tools

Yes it should. Missed fixing that one.

> Ideally the rule would be smart enough to handle this

Interestingly we can actually use SpecialPowers.Services. I'll leave it as it is for now, but file a follow-up to improve the suggested information or something and revisit these instances.
I've also just added the rule to toolkit/components/passwordmgr/.eslintrc.js - totally meant to do that before, somehow forgot.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cac71ff2481c
Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr. r=MattN
(In reply to Mark Banner (:standard8) from comment #8)
> Thanks for the suggestion, looks like there was only one that was skipped
> for e10s, and I've fixed that up.

Ok, I didn't just mean tests that you were changing though. Some tests never got ported to work with e10s. Could you double-check the following directory locally with --disable-e10s: https://dxr.mozilla.org/mozilla-central/rev/ee21e5f7f1c1726e0ed2697eb45df54cdceedd36/toolkit/components/passwordmgr/test/mochitest.ini#2
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #12)
> (In reply to Mark Banner (:standard8) from comment #8)
> > Thanks for the suggestion, looks like there was only one that was skipped
> > for e10s, and I've fixed that up.
> 
> Ok, I didn't just mean tests that you were changing though. Some tests never
> got ported to work with e10s. Could you double-check the following directory
> locally with --disable-e10s:
> https://dxr.mozilla.org/mozilla-central/rev/
> ee21e5f7f1c1726e0ed2697eb45df54cdceedd36/toolkit/components/passwordmgr/test/
> mochitest.ini#2

Thanks, I certainly wasn't expecting tests in that higher level directory. Looks like there's a few more places for Services -> SpecialPowers.Services. I'll fix those up and re-land later.
Flags: needinfo?(standard8)
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da3d20251959
Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr. r=MattN
https://hg.mozilla.org/mozilla-central/rev/da3d20251959
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.