Remove unused imports from Telemetry

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Telemetry
P3
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: gfritzsche, Assigned: Matthew, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [measurement:client] [lang=js] [good first bug])

Attachments

(1 attachment)

(Reporter)

Description

3 months ago
PromiseUtils.jsm is unused in:
- TelemetryController.jsm
- TelmetryEnvironment.jsm

We should remove that import.

To make sure changes went fine:
- build: mach build
- run eslint: mach eslint
- run tests: mach test toolkit/components/telemetry/unit
(Assignee)

Comment 1

3 months ago
Hello, I'm very new to Bugzilla and would like to get involved. This looked like a good bug to get started with if no one's currently working on it. I've removed the import from both TelemetryController.jsm and TelemetryEnvironment.jsm, built, and am running tests now.
(Assignee)

Comment 2

3 months ago
eslint complained about 2 warnings: (I don't know if these have been reported elsewhere, I haven't checked yet)

/mozilla-central/browser/components/migration/MSMigrationUtils.jsm
540:40  warning  'aFile' is already declared in the upper scope.  no-shadow (eslint)

/mozilla-central/browser/extensions/formautofill/content/FormAutofillFrameScript.js
52:18  warning  Parameter 'aMessage' uses Hungarian Notation, consider using 'message' instead.  mozilla/no-aArgs (eslint)

All 28 unit tests passed:

 1:18.26 LOG: MainThread INFO INFO | Result summary:
 1:18.26 LOG: MainThread INFO INFO | Passed: 28
 1:18.26 LOG: MainThread INFO INFO | Failed: 0
 1:18.26 LOG: MainThread INFO INFO | Todo: 0
 1:18.26 LOG: MainThread INFO INFO | Retried: 0
 1:18.26 SUITE_END: MainThread 

Summary
=======

Ran 28 tests
Expected results: 28
Unexpected results: 0

Is it typical to attach full output from tests to the bug report?
(Reporter)

Comment 3

3 months ago
(In reply to Matthew from comment #2)
> eslint complained about 2 warnings: (I don't know if these have been
> reported elsewhere, I haven't checked yet)
> 
> /mozilla-central/browser/components/migration/MSMigrationUtils.jsm
> 540:40  warning  'aFile' is already declared in the upper scope.  no-shadow
> (eslint)
> 
> /mozilla-central/browser/extensions/formautofill/content/
> FormAutofillFrameScript.js
> 52:18  warning  Parameter 'aMessage' uses Hungarian Notation, consider using
> 'message' instead.  mozilla/no-aArgs (eslint)

Hi Matthew!
These are outside the changed code, so that is fine.
I think a better eslint command is: mach eslint toolkit/components/telemetry
That should pass without errors.
(Reporter)

Updated

3 months ago
Assignee: nobody → purfling
(Assignee)

Comment 4

3 months ago
Created attachment 8855686 [details] [diff] [review]
Patch for bug 1354039
(Assignee)

Comment 5

3 months ago
Please let me know if I should have split this patch into two separate patches (one for each of the modified files).
(Reporter)

Comment 6

3 months ago
Comment on attachment 8855686 [details] [diff] [review]
Patch for bug 1354039

Review of attachment 8855686 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, thanks!
Attachment #8855686 - Flags: review+
(Reporter)

Comment 7

3 months ago
I already tested this locally, we don't need a try run for this.
Keywords: checkin-needed

Comment 8

3 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39021f2167a7
Remove unused PromiseUtils.jsm import from TelemetryController.jsm and TelemetryEnvironment.jsm. r=gfritzsche
Keywords: checkin-needed

Comment 9

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/39021f2167a7
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.