Closed Bug 1412890 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Password Manager, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file)

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.
I've seen the test failures in test_autofill_password-only.html on try server - I'll look at those in the morning.
Priority: -- → P3
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: