831688 - MOZ_SERVICES_HEALTHREPORT should be enabled in xulrunner

Status: RESOLVED FIXED

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

Description

[Mike Hommey [:glandium]]

... and prefs enabling it (datareporting.healthreport.service.enabled, datareporting.healthreport.uploadEnabled) should both default to false, and have an override on the application side (i.e. in browser/app/profile/firefox.js for desktop Firefox).

Comment 1

[Gregory Szorc [:gps]]

I have no clue what we should do about this. Who uses xulrunner? Is FHR that important for it? I also don't know if xulrunner has all the dependencies necessary to run FHR. If it doesn't just work when enabled in the build system, then this might be non-trivial. In the grand scheme of things, I don't think this is too important at the moment.

Comment 2

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #1)
> I have no clue what we should do about this. Who uses xulrunner? Is FHR that
> important for it? I also don't know if xulrunner has all the dependencies
> necessary to run FHR. If it doesn't just work when enabled in the build
> system, then this might be non-trivial. In the grand scheme of things, I
> don't think this is too important at the moment.

Fedora and Debian build Firefox as a xulrunner application.

Comment 3

[Mike Hommey [:glandium]]

It's also not completely about FHR. IIRC, since the FHR landing, Telemetry depends on healthreport.

Comment 4

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #2)
> Fedora and Debian build Firefox as a xulrunner application.

This is important. But, I'm not sure the market share warrants us prioritizing this over perf improvements that affect the overwhelming majority of our users. But I'm not a PM.

(In reply to Mike Hommey [:glandium] from comment #3)
> It's also not completely about FHR. IIRC, since the FHR landing, Telemetry
> depends on healthreport.

Telemetry does not depend on FHR. The notification bar and the XPCOM service that drives is independent of FHR and Telemetry. Those are enabled if one of {crash reports, Telemetry, FHR} is enabled. See MOZ_DATA_REPORTING.

Comment 5

[Mike Hommey [:glandium]]

I was mislead by my check script. The problem i was seeing is that telemetry now enables data reporting, and the data reporting module imports healthreport modules. Reading the code, it handles the lack of healthreport properly, but my check script is not that smart.

That being said, i wouldn't completely oppose to FHR not being enabled on Firefox-on-xulrunner builds, but the browser/ part enables MOZ_SERVICES_HEALTHREPORT, and /that/ adds code to the browser parts that probably don't have their gre counterparts when MOZ_SERVICES_HEALTHREPORT is not set in xulrunner.

Now, I understand that we're not necessarily interested in receiving data from such builds, but i would certainly understand users wanting access to the collected data locally. That is, that about:healthreport and about:telemetry work and display data, whether the data is being sent to a mozilla server or not. As a user, I do want this, and I find it sad that i can't see anything significant in about:healthreport without pinging a mozilla server and it's not even clear the frame displays a page from a distant server.

Comment 6

[Gregory Szorc [:gps]]

If /browser/confvars.sh is not the proper place to enable MOZ_SERVICES_HEALTHREPORT for dekstop but not xulrunner, where should we? 

You are correct that it is advantageous for FHR to be enabled and collecting even if it is not submitting because about:healthreport could be useful.

Because xulrunner is being used to power some Firefox deployments, I think it makes sense to enable FHR there. If downstream builders need to make modifications to whether it submits data or where it submits data, we have prefs for that.

Again, this bug is a low priority for me unless someone tells me otherwise.

Comment 7

[Mike Hommey [:glandium]]

I can take the bug, it's not the problem. The problem is agreeing on what to do :)

Comment 8

[Richard Newman [:rnewman]]

Let's do it.

Comment 9

[Richard Newman [:rnewman]]

Flagging as P4, because this is less important than addressing FHR on Android.

Comment 11

[Jan Horak [:jhorak]]

Attachment: Fix for build with libxul v1

Okay, reattaching patch according to bug 831688 comment 4.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 760469 [details] [diff] [review]
Fix for build with libxul v1

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

This is correct from a technical sense. But, I'm not sure if all the other pieces will be present in xulrunner, notably the data policy notification bar and about:healthreport. I also don't know if all the tests will pass because I've never attempted to run them in xulrunner.

If there is no notification bar, FHR will just collect a bunch of data but will never upload it. And, if there is no about:healthreport, the user won't be able to see said data.

So, I'm reluctantly r+'ing this. We definitely need verification of some kind, however.

Comment 13

[Jan Horak [:jhorak]]

Attachment: Fix for build with libxul v2

Attaching patch with commit message. Copying review from previous version.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f172342f18c2

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f172342f18c2