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

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

Attachments

(6 attachments)

Assignee

Description

2 years ago
I'm working on rolling out mozilla/use-services across the tree, toolkit/components is the next piece of the puzzle.

We're not just doing simple replacements - we're tidying up existing variables/usages at the same time.
Comment hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8924882 [details]
Bug 1412893 - Change instances of using getService to Services.jsm where possible in toolkit/components/places

https://reviewboard.mozilla.org/r/196154/#review201312
Attachment #8924882 - Flags: review?(mak77) → review+

Comment 9

2 years ago
mozreview-review
Comment on attachment 8924881 [details]
Bug 1412893 - Change instances of using getService to Services.jsm where possible in toolkit/components - Part 2.

https://reviewboard.mozilla.org/r/196152/#review201382

Looks good, thanks!

::: toolkit/components/remotebrowserutils/tests/browser/browser_RemoteWebNavigation.js
(Diff revision 1)
>  
>  function waitForPageShow(browser = gBrowser.selectedBrowser) {
>    return BrowserTestUtils.waitForContentEvent(browser, "pageshow", true);
>  }
>  
> -function makeURI(url) {

Yay for another makeURI function disappearing :-).

::: toolkit/components/telemetry/TelemetryStopwatch.jsm
(Diff revision 1)
> +Cu.import("resource://gre/modules/Services.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "Log",
>    "resource://gre/modules/Log.jsm");
>  
> -var Telemetry = Cc["@mozilla.org/base/telemetry;1"]
> -                  .getService(Ci.nsITelemetry);

Note: removing this changes the behavior, as here the service would have been initialized when first importing the module, whereas now we rely on it being  initialized by something else (which is fine in this case, as the telemetry service is loaded during early startup anyway).
Attachment #8924881 - Flags: review?(florian) → review+

Comment 10

2 years ago
mozreview-review
Comment on attachment 8924880 [details]
Bug 1412893 - Change instances of using getService to Services.jsm where possible in toolkit/components - Part 1.

https://reviewboard.mozilla.org/r/196150/#review201386

drive-by comment on one file, but I only skimmed the patch, only looked quickly at about half the touched files

::: toolkit/components/nsDefaultCLH.js:84
(Diff revision 1)
>  
>      if (cmdLine.preventDefault)
>        return;
>  
>      var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>                            .getService(nsIPrefBranch);

This one looks like it wants to be replaced too ;-).

I think it's not being caught by eslint due to the 'const nsIPrefBranch' at the top of the file. Could you please have another look at the whole file to see if we should remove these const and use Ci instead? Or should we make the eslint rule catch this case too?
Assignee

Comment 11

2 years ago
mozreview-review-reply
Comment on attachment 8924880 [details]
Bug 1412893 - Change instances of using getService to Services.jsm where possible in toolkit/components - Part 1.

https://reviewboard.mozilla.org/r/196150/#review201386

> This one looks like it wants to be replaced too ;-).
> 
> I think it's not being caught by eslint due to the 'const nsIPrefBranch' at the top of the file. Could you please have another look at the whole file to see if we should remove these const and use Ci instead? Or should we make the eslint rule catch this case too?

You're right, we're not catching those kind of instances. I think it is reasonable for ESLint to catch the cases where the variable name is the same as the interface name (as that's been a traditional way of naming).

I've filed bug 1414240 to handle fixing the ESLint rule. I think we should work on these instances when we fix the rule, I'd prefer not to make these already fairly large patches even larger.

Comment 12

2 years ago
mozreview-review
Comment on attachment 8924880 [details]
Bug 1412893 - Change instances of using getService to Services.jsm where possible in toolkit/components - Part 1.

https://reviewboard.mozilla.org/r/196150/#review201558

::: toolkit/components/apppicker/content/appPicker.js:100
(Diff revision 1)
>  
>      /**
>      * Retrieve the moz-icon for the app
>      */
>      getFileIconURL: function getFileIconURL(file) {
> -      var ios = Components.classes["@mozilla.org/network/io-service;1"].
> +      if (!Services.io) return "";

I don't think this can ever happen can it?
Attachment #8924880 - Flags: review?(dtownsend) → review+

Comment 13

2 years ago
mozreview-review
Comment on attachment 8924883 [details]
Bug 1412893 - Change instances of using getService to Services.jsm where possible in toolkit/components/printing.

https://reviewboard.mozilla.org/r/196156/#review201560
Attachment #8924883 - Flags: review?(dtownsend) → review+

Comment 14

2 years ago
mozreview-review
Comment on attachment 8924885 [details]
Bug 1412893 - Enable ESLint rule mozilla/use-services for toolkit/components/

https://reviewboard.mozilla.org/r/196160/#review201562

What this does doesn't seem to match the commit message.
Attachment #8924885 - Flags: review?(dtownsend) → review-

Comment 15

2 years ago
mozreview-review
Comment on attachment 8924884 [details]
Bug 1412893 - Change instances of using getService to Services.jsm where possible in toolkit/components/url-classifier

https://reviewboard.mozilla.org/r/196158/#review201628
Attachment #8924884 - Flags: review?(francois) → review+
Assignee

Comment 16

2 years ago
mozreview-review-reply
Comment on attachment 8924880 [details]
Bug 1412893 - Change instances of using getService to Services.jsm where possible in toolkit/components - Part 1.

https://reviewboard.mozilla.org/r/196150/#review201558

> I don't think this can ever happen can it?

Yep, shouldn't do, looks like I left that in there by mistake.
Assignee

Comment 17

2 years ago
mozreview-review-reply
Comment on attachment 8924885 [details]
Bug 1412893 - Enable ESLint rule mozilla/use-services for toolkit/components/

https://reviewboard.mozilla.org/r/196160/#review201562

I was going to do all of toolkit/ first, then I found it a bit more complicated. Unfortunately I forgot to update the message. Update incoming...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

2 years ago
mozreview-review
Comment on attachment 8924885 [details]
Bug 1412893 - Enable ESLint rule mozilla/use-services for toolkit/components/

https://reviewboard.mozilla.org/r/196160/#review202040
Attachment #8924885 - Flags: review?(dtownsend) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 31

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/80fbacdf4ca9
Change instances of using getService to Services.jsm where possible in toolkit/components - Part 1. r=mossop
https://hg.mozilla.org/integration/autoland/rev/ff14bf576133
Change instances of using getService to Services.jsm where possible in toolkit/components - Part 2. r=florian
https://hg.mozilla.org/integration/autoland/rev/c964c1b562c4
Change instances of using getService to Services.jsm where possible in toolkit/components/places r=mak
https://hg.mozilla.org/integration/autoland/rev/bd1481d9e7cd
Change instances of using getService to Services.jsm where possible in toolkit/components/printing. r=mossop
https://hg.mozilla.org/integration/autoland/rev/73f8a7bbae04
Change instances of using getService to Services.jsm where possible in toolkit/components/url-classifier r=francois
https://hg.mozilla.org/integration/autoland/rev/46f5bb50f656
Enable ESLint rule mozilla/use-services for toolkit/components/ r=mossop
You need to log in before you can comment on or make changes to this bug.