Closed Bug 1234526 Opened 4 years ago Closed 4 years ago

Remove services/healthreport

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect, P1)

defect

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

(Whiteboard: [measurement:client])

Attachments

(3 files, 1 obsolete file)

This bug is about removing services/metrics & services/healthreport from Firefox codebase.


We should also take care of removing:

- Orphaned manifest entries: https://dxr.mozilla.org/mozilla-central/search?q=%22HealthReport.jsm%22+-path%3Aobj&redirect=false&case=false
- Uses of Metrics.jsm throughout the codebase
No longer depends on: 1234522
Points: --- → 3
Changed this bug to only remove services/healthreport to ease reviewing.
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Summary: Remove services/metrics & services/healthreport → Remove services/healthreport
Blocks: 1235345
Depends on: 1234522
This bug should also investigate and eventually remove all the uses of the FHR prefs (listed in [0]), except the following:

- datareporting.healthreport.infoURL - Used to build the preferences panel
- datareporting.healthreport.uploadEnabled - Used to control ping upload in Telemetry
- datareporting.healthreport.about.reportUrl - Used for the about:healthreport content URL / former v2 URL
- datareporting.healthreport.about.reportUrlUnified - Used for the about:healthreport content URL / UT URL

[0] - https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/services/healthreport/healthreport-prefs.js
Priority: P2 → P1
(In reply to Alessio Placitelli [:Dexter] from comment #2)
> This bug should also investigate and eventually remove all the uses of the
> FHR prefs (listed in [0]), except the following:
> 
> - datareporting.healthreport.infoURL - Used to build the preferences panel
> - datareporting.healthreport.uploadEnabled - Used to control ping upload in
> Telemetry
> - datareporting.healthreport.about.reportUrl - Used for the
> about:healthreport content URL / former v2 URL
> - datareporting.healthreport.about.reportUrlUnified - Used for the
> about:healthreport content URL / UT URL

This was planned to be used, but we decided to go a different route (see bug 1227579).
We should remove that and any uses in code (potentially in a separate bug).
Attachment #8704160 - Flags: review?(gfritzsche)
Attachment #8704162 - Flags: review?(gfritzsche)
Comment on attachment 8704160 [details] [diff] [review]
part 1 - Remove services/healthreport

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

This is not removing test_healthreporter.js.

::: mobile/android/installer/package-manifest.in
@@ -440,5 @@
>  
> -#ifdef MOZ_SERVICES_HEALTHREPORT
> -@BINPATH@/components/HealthReportComponents.manifest
> -@BINPATH@/components/HealthReportService.js
> -#endif

Already removed: https://hg.mozilla.org/mozilla-central/rev/de71199f4e00

::: modules/libpref/greprefs.js
@@ +6,5 @@
>  #ifdef MOZ_SERVICES_HEALTHREPORT
>  #if MOZ_WIDGET_TOOLKIT == android
>  #include ../../mobile/android/chrome/content/healthreport-prefs.js
>  #else
> +#include ../../toolkit/components/telemetry/healthreport-prefs.js

Lets file something about consolidating those prefs into one file, telemetry-prefs.js

::: services/healthreport/docs/dataformat.rst
@@ -1,1 @@
> -.. _healthreport_dataformat:

People might still have to look at this for a bit.
Lets move this to the Telemetry doc dir and index (so we can kill the FHR dirs) and mark it with a clear disclaimer on FHR being obsolete.
Attachment #8704160 - Flags: review?(gfritzsche) → review+
Comment on attachment 8704162 [details] [diff] [review]
part 2 - Remove unused prefs

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

::: toolkit/components/telemetry/healthreport-prefs.js
@@ +11,2 @@
>  pref("datareporting.healthreport.about.reportUrl", "https://fhr.cdn.mozilla.net/%LOCALE%/v4/");
>  pref("datareporting.healthreport.about.reportUrlUnified", "https://fhr.cdn.mozilla.net/%LOCALE%/v4/");

Didn't we want to kill .reportUrlUnified somewhere?
I'd be happy to have it in a separate bug as long as it happens.
Attachment #8704162 - Flags: review?(gfritzsche) → review+
Attachment #8704160 - Attachment is obsolete: true
Attachment #8704693 - Flags: review+
(In reply to Georg Fritzsche [:gfritzsche] [away until Jan 3] from comment #6)
> Comment on attachment 8704160 [details] [diff] [review]
> part 1 - Remove services/healthreport
> 
> Review of attachment 8704160 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is not removing test_healthreporter.js.

This is being removed by bug 1234522.

> ::: mobile/android/installer/package-manifest.in
> @@ -440,5 @@
> >  
> > -#ifdef MOZ_SERVICES_HEALTHREPORT
> > -@BINPATH@/components/HealthReportComponents.manifest
> > -@BINPATH@/components/HealthReportService.js
> > -#endif
> 
> Already removed: https://hg.mozilla.org/mozilla-central/rev/de71199f4e00

Ah, good!

> ::: modules/libpref/greprefs.js
> @@ +6,5 @@
> >  #ifdef MOZ_SERVICES_HEALTHREPORT
> >  #if MOZ_WIDGET_TOOLKIT == android
> >  #include ../../mobile/android/chrome/content/healthreport-prefs.js
> >  #else
> > +#include ../../toolkit/components/telemetry/healthreport-prefs.js
> 
> Lets file something about consolidating those prefs into one file,
> telemetry-prefs.js

Filed bug 1237276.

> ::: services/healthreport/docs/dataformat.rst
> @@ -1,1 @@
> > -.. _healthreport_dataformat:
> 
> People might still have to look at this for a bit.
> Lets move this to the Telemetry doc dir and index (so we can kill the FHR
> dirs) and mark it with a clear disclaimer on FHR being obsolete.

Good point.
(In reply to Georg Fritzsche [:gfritzsche] [away until Jan 3] from comment #7)
> Comment on attachment 8704162 [details] [diff] [review]
> part 2 - Remove unused prefs
> 
> Review of attachment 8704162 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/healthreport-prefs.js
> @@ +11,2 @@
> >  pref("datareporting.healthreport.about.reportUrl", "https://fhr.cdn.mozilla.net/%LOCALE%/v4/");
> >  pref("datareporting.healthreport.about.reportUrlUnified", "https://fhr.cdn.mozilla.net/%LOCALE%/v4/");
> 
> Didn't we want to kill .reportUrlUnified somewhere?
> I'd be happy to have it in a separate bug as long as it happens.

That's being done as part of bug 1236580.
Attachment #8705077 - Flags: review?(gfritzsche) → review+
Duplicate of this bug: 1217448
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.