1196219 - Remove RECORD_PREF_NOTIFY_ONLY from TelemetryEnvironment

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

We don't actually use the RECORD_PREF_NOTIFY_ONLY behavior for any pref, so we should remove it and any logic that handles it:
https://dxr.mozilla.org/mozilla-central/search?q=path%3Atoolkit%2Fcomponents%2Ftelemetry%2F+RECORD_PREF_NOTIFY_ONLY&redirect=true&case=true&limit=67&offset=0

Comment 1

[Benjamin Smedberg]

Are you sure we don't need it? There are a bunch of things in the settings block which are based on prefs and can change mid-run.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Yes, we have 3 different behaviors:
  RECORD_PREF_STATE: 1, // Don't record the preference value
  RECORD_PREF_VALUE: 2, // We only record user-set prefs.
  RECORD_PREF_NOTIFY_ONLY: 3, // Record nothing, just notify of changes.

Everything we do is covered by the first two, the third is unused and of dubious value.

Comment 3

[rthyberg]

can I take this bug?

Comment 4

[Georg Fritzsche [:gfritzsche]]

Sure, assigned it to you :)

Comment 5

[rthyberg]

should i remove it from other files as well? or just from TelemetryEnvironmret

Comment 6

[Georg Fritzsche [:gfritzsche]]

Per the code search in comment 0 this isn't used anywhere else, so removing from TelemetryEnvironment should be all.

Comment 7

[rthyberg]

Attachment: removed references to RECORD_PREF_NOTIFY and adjusted the logic relating to it

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8650145 [details] [diff] [review]
removed references to RECORD_PREF_NOTIFY and adjusted the logic relating to it

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

This looks good to me with the below fixed.

Note that we have to finish bug 1192811 first before we can land this bug.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ -768,3 @@
>          continue;
>        }
> -

Don't remove the empty line here, there is no need to.

Comment 9

[rthyberg]

Attachment: rm_pref_notify.patch

undid the line removal

Comment 10

[Georg Fritzsche [:gfritzsche]]

Pushed to try together with bug 1192811:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f073abd2da6

Comment 12

[Georg Fritzsche [:gfritzsche]]

The try run from comment 10 is all green, so setting checkin-needed.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/42a5571f51c9

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42a5571f51c9