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
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.
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.
can I take this bug?
Sure, assigned it to you :)
Assignee: nobody → robertthyberg
should i remove it from other files as well? or just from TelemetryEnvironmret
Per the code search in comment 0 this isn't used anywhere else, so removing from TelemetryEnvironment should be all.
Created attachment 8650145 [details] [diff] [review] removed references to RECORD_PREF_NOTIFY and adjusted the logic relating to it
Attachment #8650145 - Flags: review?(gavin.sharp)
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.
Attachment #8650145 - Flags: review?(gfritzsche) → review+
Created attachment 8650690 [details] [diff] [review] rm_pref_notify.patch undid the line removal
Pushed to try together with bug 1192811: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f073abd2da6
Attachment #8650145 - Attachment is obsolete: true
The try run from comment 10 is all green, so setting checkin-needed.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.