Remove RECORD_PREF_NOTIFY_ONLY from TelemetryEnvironment

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gfritzsche, Assigned: robertthyberg)

Tracking

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 wontfix, firefox43 fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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

3 years ago
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.
(Reporter)

Comment 2

3 years ago
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.
(Assignee)

Comment 3

3 years ago
can I take this bug?
(Reporter)

Comment 4

3 years ago
Sure, assigned it to you :)
Assignee: nobody → robertthyberg
(Assignee)

Comment 5

3 years ago
should i remove it from other files as well? or just from TelemetryEnvironmret
(Reporter)

Comment 6

3 years ago
Per the code search in comment 0 this isn't used anywhere else, so removing from TelemetryEnvironment should be all.
(Assignee)

Comment 7

3 years ago
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)
(Reporter)

Updated

3 years ago
Attachment #8650145 - Attachment is patch: true
Attachment #8650145 - Flags: review?(gavin.sharp) → review?(gfritzsche)
(Reporter)

Comment 8

3 years ago
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+
(Assignee)

Comment 9

3 years ago
Created attachment 8650690 [details] [diff] [review]
rm_pref_notify.patch

undid the line removal
Flags: needinfo?(gfritzsche)
(Reporter)

Comment 10

3 years ago
Pushed to try together with bug 1192811:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f073abd2da6
Flags: needinfo?(gfritzsche)
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1137178
(Reporter)

Updated

3 years ago
Attachment #8650145 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Attachment #8650690 - Flags: review+
(Reporter)

Comment 12

3 years ago
The try run from comment 10 is all green, so setting checkin-needed.
(Reporter)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/42a5571f51c9
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.