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)
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•8 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•8 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•8 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
||
bugherder |
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.
Description
•