Closed Bug 1230373 Opened 4 years ago Closed 2 years ago

Add an eslint rule to detect usage of getService where Services.jsm could have been used instead.

Categories

(Firefox Build System :: Lint and Formatting, defect, P3)

defect

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: MattN, Assigned: standard8)

References

Details

Attachments

(3 files)

For example
> Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);
could be replaced by
> Services.wm
Component: General → ESLint
Severity: enhancement → normal
Priority: -- → P3
Assignee: nobody → standard8
Comment on attachment 8917399 [details]
Bug 1230373 - Enable mozilla/use-services for browser/components/

https://reviewboard.mozilla.org/r/188402/#review193644

A few drive-by comment (I haven't looked at all files). Nice cleanup! :-)

::: browser/components/distribution.js:267
(Diff revision 1)
>  
>      // nsPrefService loads very early.  Reload prefs so we can set
>      // distribution defaults during the prefservice:after-app-defaults
>      // notification (see applyPrefDefaults below)
> -    this._prefSvc.QueryInterface(Ci.nsIObserver);
> -    this._prefSvc.observe(null, "reload-default-prefs", null);
> +    Services.prefs.QueryInterface(Ci.nsIObserver);
> +    Services.prefs.observe(null, "reload-default-prefs", null);

nit: I would skip the second "Services.prefs." here

::: browser/components/migration/content/migration.js:421
(Diff revision 1)
>            Services.telemetry.getKeyedHistogramById("FX_MIGRATION_IMPORTED_HOMEPAGE")
>                              .add(this._source, hasImportedHomepage);
>            if (this._newHomePage) {
>              try {
>                // set homepage properly
> -              var prefSvc = Components.classes["@mozilla.org/preferences-service;1"]
> +              var prefBranch = Services.prefs.getBranch(null);

There's already a .QueryInterface(Ci.nsIPrefBranch) in Services.jsm, I think that gives the same result as .getBranch(null).

::: browser/components/preferences/in-content/main.js:157
(Diff revision 1)
>    get _filter() {
>      delete this._filter;
>      return this._filter = document.getElementById("filter")
>    },
>  
> -  _prefSvc: Cc["@mozilla.org/preferences-service;1"].
> +  _prefSvc: Services.prefs,

this._prefSvc is only one character shorter than Services.prefs, should we get rid of it?

::: browser/components/preferences/in-content/tests/browser_cookies_exceptions.js:256
(Diff revision 1)
>              btnApplyChanges: doc.getElementById("btnApplyChanges"),
>              btnRemove: doc.getElementById("removePermission"),
> -            pm: Cc["@mozilla.org/permissionmanager;1"]
> +            pm: Services.perms,
> -                       .getService(Ci.nsIPermissionManager),
>              ioService: Cc["@mozilla.org/network/io-service;1"]
>                                .getService(Ci.nsIIOService),

This is Services.io
Just to note, that in this bug I'm only going to do browser/components as a proof-of-usage for the rule.

We'll deal with enabling this rule throughout the rest of the tree in other bugs - I did consider an automated fix, but as can be seen from the browser/components patch, there's quite a bit of other tidy up that is worth doing which negates the usefulness of automation.
The latest version also includes a version bump for eslint-plugin-mozilla, so that we can get this goodness around to our outside of mozilla-central users.
Comment on attachment 8917398 [details]
Bug 1230373 - Add an ESLint rule to prefer using Services.jsm rather than getService.

https://reviewboard.mozilla.org/r/188400/#review194252

The process for creating global variables then requiring Services.jsm is kind of nasty. Could we instead parse Services.jsm to an AST and parse through it to find what we need? It's maybe a little more complicated code-wise but doesn't require polluting the global namespace.

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-services.js:14
(Diff revision 3)
> +"use strict";
> +var helpers = require("../helpers");
> +var path = require("path");
> +
> +function getInterfacesFromServicesFile() {
> +  // These interefaces are difficult to get via processing.

*interfaces
Attachment #8917398 - Flags: review?(dtownsend)
Comment on attachment 8917399 [details]
Bug 1230373 - Enable mozilla/use-services for browser/components/

https://reviewboard.mozilla.org/r/188402/#review194608
Attachment #8917399 - Flags: review?(dtownsend) → review+
Comment on attachment 8918871 [details]
Bug 1230373 - Fix an issue with eslint-plugin-mozilla not detecting the global scope properly when arrow functions are used.

https://reviewboard.mozilla.org/r/189738/#review195132
Attachment #8918871 - Flags: review?(dtownsend) → review+
Dave, I think you might have missed the third patch that I slipped in on the last update.
Flags: needinfo?(dtownsend)
Flags: needinfo?(dtownsend)
Comment on attachment 8917398 [details]
Bug 1230373 - Add an ESLint rule to prefer using Services.jsm rather than getService.

https://reviewboard.mozilla.org/r/188400/#review195978

Impressive, thanks!
Attachment #8917398 - Flags: review?(dtownsend) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dcd89150bf7d
Fix an issue with eslint-plugin-mozilla not detecting the global scope properly when arrow functions are used. r=mossop
https://hg.mozilla.org/integration/autoland/rev/55c5290c0147
Add an ESLint rule to prefer using Services.jsm rather than getService. r=mossop
https://hg.mozilla.org/integration/autoland/rev/c072884b1b90
Enable mozilla/use-services for browser/components/ r=mossop
Depends on: 1412439
Product: Testing → Firefox Build System
Duplicate of this bug: 731180
You need to log in before you can comment on or make changes to this bug.