Closed Bug 1412893 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: General, enhancement, P3)

Product:
Toolkit
Component:
General
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(6 files)

Bug 1412893 - Change instances of using getService to Services.jsm where possible in toolkit/components - Part 1.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1412893 - Change instances of using getService to Services.jsm where possible in toolkit/components - Part 2.
59 bytes, text/x-review-board-request
florian
: review+
Details
Bug 1412893 - Change instances of using getService to Services.jsm where possible in toolkit/components/places
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1412893 - Change instances of using getService to Services.jsm where possible in toolkit/components/printing.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1412893 - Change instances of using getService to Services.jsm where possible in toolkit/components/url-classifier
59 bytes, text/x-review-board-request
francois
: review+
Details
Bug 1412893 - Enable ESLint rule mozilla/use-services for toolkit/components/
59 bytes, text/x-review-board-request
mossop
: review+
Details 
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 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 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 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?
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 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 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 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 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+
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.
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 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+
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.

Attachment

General

Creator:
Created:
Updated:
Size: