1344723 - Use the prefs defined in TelemetryUtils.jsm in TelemetryController.jsm

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Alessio Placitelli [:Dexter]]

Once bug 1344295 lands, we should change Telemetry code to use the preferences defined in the TelemetryUtils.jsm file rather then redefining them in every Telemetry JSM file.

Comment 1

[Alessio Placitelli [:Dexter]]

Let's restrict this bug to changes to TelemetryController.jsm. After this lands, we can file more bugs for the other files (so we don't risk bitrotting here with a huge patch).

Comment 2

[Alejandro Rodriguez Salamanca [:alejandro]]

I take this one.

Comment 3

[Alejandro Rodriguez Salamanca [:alejandro]]

There are several preferences such as PREF_BRANCH_LOG (and all the preferences that extends this one, PREF_LOG_LEVEL, PREF_LOG_DUMP...), or PREF_NEWPROFILE_PING_ENABLED that are not declared in TelemetryUtils.jsm. Do I need to create this preferences in TelemetryUtils.jsm?

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to Alejandro Rodriguez Salamanca from comment #3)
> There are several preferences such as PREF_BRANCH_LOG (and all the
> preferences that extends this one, PREF_LOG_LEVEL, PREF_LOG_DUMP...), or
> PREF_NEWPROFILE_PING_ENABLED that are not declared in TelemetryUtils.jsm. Do
> I need to create this preferences in TelemetryUtils.jsm?

Yes please! If you see any preference that's missing, make sure to add it to the Utils file.

Comment 5

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Add missing preferences to TelemetryUtils and replace local preferences in TelemetryController

Comment 6

[Alessio Placitelli [:Dexter]]

Comment on attachment 8887476 [details] [diff] [review]
Add missing preferences to TelemetryUtils and replace local preferences in TelemetryController

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

That's a good start, thanks Alejandro!

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +119,5 @@
>  
>  this.EXPORTED_SYMBOLS = ["TelemetryController"];
>  
>  this.TelemetryController = Object.freeze({
>    Constants: Object.freeze({

We now have the TelemetryUtils.Preferences object for all our preferences needs. Let's get rid of this "Constants" object and properly use TelemetryUtils where this was referenced. I think it isn't being used, otherwise you should have seen an error due to ":" being used ;)

@@ +880,5 @@
>     * Called whenever the FHR Upload preference changes (e.g. when user disables FHR from
>     * the preferences panel), this triggers sending the deletion ping.
>     */
>    _onUploadPrefChange() {
> +    const uploadEnabled = Preferences.get(TelemetryUtils.Preferences.HealthReportUploadEnabled, false);

Please use the pref that was already available here.

@@ +909,5 @@
>  
>    _attachObservers() {
>      if (IS_UNIFIED_TELEMETRY) {
>        // Watch the FHR upload setting to trigger deletion pings.
> +      Preferences.observe(TelemetryUtils.Preferences.HealthReportUploadEnabled, this._onUploadPrefChange, this);

Please use the pref that was already available here.

@@ +918,5 @@
>     * Remove the preference observer to avoid leaks.
>     */
>    _detachObservers() {
>      if (IS_UNIFIED_TELEMETRY) {
> +      Preferences.ignore(TelemetryUtils.Preferences.HealthReportUploadEnabled, this._onUploadPrefChange, this);

Please use the pref that was already available here.

::: toolkit/components/telemetry/TelemetryUtils.jsm
@@ +37,5 @@
> +    NewProfilePingEnabled: "toolkit.telemetry.newProfilePing.enabled",
> +    NewProfilePingDelay: "toolkit.telemetry.newProfilePing.delay",
> +
> +    // Log Preferences
> +    Log: "toolkit.telemetry.log.",

This log entry is only used once in the TelemetryController file, let's just leave it as a const there. Just move LogLevel and LogDump here.

@@ +50,5 @@
>      DataSubmissionEnabled: "datareporting.policy.dataSubmissionEnabled",
>      FhrUploadEnabled: "datareporting.healthreport.uploadEnabled",
>      MinimumPolicyVersion: "datareporting.policy.minimumPolicyVersion",
> +    HealthReportUploadEnabled: "datareporting.healthreport.uploadEnabled",
> +

This preference is already available a couple of lines above, as FhrUploadEnabled.

Comment 7

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Remove redundant and useless code

Comment 8

[Alessio Placitelli [:Dexter]]

Comment on attachment 8887526 [details] [diff] [review]
Remove redundant and useless code

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

This looks good with the nit below addressed! You won't need to request a new review after you attach the updated patch.
I will push it to try for you once you upload the updated patch!

::: toolkit/components/telemetry/TelemetryUtils.jsm
@@ +48,5 @@
>      CurrentPolicyVersion: "datareporting.policy.currentPolicyVersion",
>      DataSubmissionEnabled: "datareporting.policy.dataSubmissionEnabled",
>      FhrUploadEnabled: "datareporting.healthreport.uploadEnabled",
>      MinimumPolicyVersion: "datareporting.policy.minimumPolicyVersion",
> +

nit: please remove this empty blank line

Comment 9

[Alessio Placitelli [:Dexter]]

Comment on attachment 8887526 [details] [diff] [review]
Remove redundant and useless code

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

Also please the commit message to something like "Bug 1344723 - Use the prefs defined in TelemetryUtils.jsm in TelemetryController.jsm. r?dexter"

Comment 10

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Use the prefs defined in TelemetryUtils.jsm in TelemetryController.jsm

Comment 11

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6684003deecea7053cdc98ea186b56cc2b3c5648

Comment 12

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9599eab713ca6e7660e18307cf418378896f0363
Bug 1344723 - Use the prefs defined in TelemetryUtils.jsm in TelemetryController.jsm. r=dexter

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9599eab713ca

Comment 15

[Gerry Chang [:gchang]]

We are about to go to 55 RC. Mark 55 won't fix.