Closed Bug 1196219 Opened 8 years ago Closed 8 years ago

Remove RECORD_PREF_NOTIFY_ONLY from TelemetryEnvironment


(Toolkit :: Telemetry, defect)

Not set



Tracking Status
firefox42 --- wontfix
firefox43 --- fixed


(Reporter: gfritzsche, Assigned: robertthyberg)



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


(1 file, 1 obsolete file)

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:
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.
Attachment #8650145 - Flags: review?(
Attachment #8650145 - Attachment is patch: true
Attachment #8650145 - Flags: review?( → review?(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.
Attachment #8650145 - Flags: review?(gfritzsche) → review+
undid the line removal
Flags: needinfo?(gfritzsche)
Pushed to try together with bug 1192811:
Flags: needinfo?(gfritzsche)
Attachment #8650145 - Attachment is obsolete: true
Attachment #8650690 - Flags: review+
The try run from comment 10 is all green, so setting checkin-needed.
Keywords: checkin-needed
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.