1478834 - aboutTelemetry.js imports Preference.jsm both directly and through a getter

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement)

Description

[Marco Castelluccio [:marco]]

https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/toolkit/content/aboutTelemetry.js#14-20

Comment 1

[Jan-Erik Rediger [:janerik]]

To help Mozilla out with this bug, here's the steps:

0) Comment here on the bug that you want to volunteer to help. I (or someone else) will assign it to you.
1) Download and build the Firefox source code: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds (an Artifact build is enough)
- If you have any problems, please ask on IRC (https://wiki.mozilla.org/Irc) in the #introduction channel. They're there to help you get started.
- You can also read the Developer Guide, which has answers to most development questions: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
2) Start working on this bug. 
- Open aboutTelemetry.jsm
- Remove line 19 and 20 (the `defineModuleGetter` for `Preferences.jsm`
- If you have any problems with this bug, please comment on this bug and set the needinfo flag for me. Also, you can find me and my teammates on the #telemetry channel on IRC (https://wiki.mozilla.org/Irc) most hours of most days.
3) Build your change with `mach build`. Start Firefox (`mach run`) and open about:telemetry. Check that basic functionality works (e.g. switching to other sections or selecting another ping)
4) Submit the patch for review. Mark me as a reviewer so I'll get an email to come look at your code.
- Here's the guide: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
5) After a series of reviews and changes to your patch, I'll mark it for checkin or push it to autoland. Your code will soon be shipping to Firefox users worldwide!
6) ...now you get to think about what kind of bug you'd like to work on next. Let me know what you're interested in and I can help you find your next contribution.

Comment 2

[Siddhant [:Siddhant085]]

I would like to work on this bug. I am new to Mozilla.

Comment 3

[Siddhant [:Siddhant085]]

Attachment: Removed lines 19-20 from aboutTelemetry.js

Removed lines 19-20 from aboutTelemetry.js as Preference.jsm is already included in line 14. Tested by opening about:telemetry and making sure things work.

Comment 4

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8995782 [details] [diff] [review]
Removed lines 19-20 from aboutTelemetry.js

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

The patch looks good and simple as expected. Could you do me the favor and reword the commit message into something like "Bug 1478834 - Remove duplicated import of Preferences module" ?

Comment 5

[Marco Castelluccio [:marco]]

It might be better to remove the direct import and keep the delayed one, to have better perf when loading the page (even though it's not a page used often and widely, so its perf is not that important).

Comment 6

[Siddhant [:Siddhant085]]

Attachment: Bug 1478834 - Remove duplicated import of Preferences module in aboutTelemetry.js

Removed duplicate import of module preference in aboutTelemetry.js. Removed the direct import and kept the delayed one. Even though the page is not used often its better to keep the best performance we can.

Comment 7

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8995973 [details] [diff] [review]
Bug 1478834 - Remove duplicated import of Preferences module in aboutTelemetry.js

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

The change itself is fine, but I'm just now realizing that the patch file does not contain the commit message. In order to correctly apply it we need it as a HG patchset. See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Submitting_the_patch for details how to do that.
If you're using git, you should look at https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install-git.html, which uses MozReview, which is also fine to use.

Comment 8

[Siddhant [:Siddhant085]]

Attachment: Remove duplicated import of Preferences module in aboutTelemetry.js

Comment 9

[Jan-Erik Rediger [:janerik]]

Thanks for your contribution! I marked the patch as reviewed and marked it for checkin, so it will land in the next hours.
Do you already have something in mind you want to do next? I can also take a look and see if I find something suitable.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57c753069162
Remove duplicated import of Preferences module in aboutTelemetry.js r=janerik

Comment 11

[Siddhant [:Siddhant085]]

(In reply to Jan-Erik Rediger [:janerik] from comment #9)
> Thanks for your contribution! I marked the patch as reviewed and marked it
> for checkin, so it will land in the next hours.
> Do you already have something in mind you want to do next? I can also take a
> look and see if I find something suitable.

I don't have anything particular in mind. I would like to lean towards core functionalities but for starters anything meaningful should be fine.

Comment 12

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/57c753069162