Closed
Bug 1412890
Opened 7 years ago
Closed 7 years ago
Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr
Categories
(Toolkit :: Password Manager, defect, P3)
Toolkit
Password Manager
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 hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
mozreview-review |
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.
Assignee | ||
Comment 3•7 years ago
|
||
I've seen the test failures in test_autofill_password-only.html on try server - I'll look at those in the morning.
Updated•7 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
I fixed the issue - Services was out of scope in one of the functions that gets run in a parent process.
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
mozreview-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 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+
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 9•7 years ago
|
||
I've also just added the rule to toolkit/components/passwordmgr/.eslintrc.js - totally meant to do that before, somehow forgot.
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
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
Comment 12•7 years ago
|
||
(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
Comment 13•7 years ago
|
||
Backed out changeset cac71ff2481c (bug 1412890) possible ESlint failure. Backout requested by Mark Banner. https://hg.mozilla.org/integration/autoland/rev/d86cc075ca5d2155086cccb5e0086573961447e9 https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=cac71ff2481c20c77090cb508650d2b36175f2e8&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Flags: needinfo?(standard8)
Assignee | ||
Comment 14•7 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
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
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/da3d20251959
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•