Closed Bug 888030 Opened 8 years ago Closed 5 years ago

Make health report xpcom service runnable on b2g app

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: gps, Unassigned)

References

Details

Attachments

(1 file)

The current FHR code base doesn't come close to running under the b2g app. Stefan has been working on adding #ifdefs etc to make everything happy.
+ enabled FHR in /b2g/confvars.sh
+ included DataReporting in /b2g/installer/package-manifest.in
+ HACK: because B2G does not have a "session-windows-restored" event, for testing purposes I had to make FHR load at "profile-after-change" in /services/datareportings/DataReportingService.js
+ added #ifdef wrappers for unexistent providers in B2G (currently plaes & search) in /services/healthreport/HealthReportComponents.manifest and as a result enabled prepocessing on this file in /services/healthreport/moz.build
+ added a global constant for the data collection inverval for easier development in /services/healthreport/healthreporter.jsm
+ HACKS: disabled 3 xpcshell tests in /services/healthreport/tests/xpcshell/xpcshell.ini . Atempting to do OS or toolkit detection and having them being skipped if platforms is B2G did not function on my configuration so these had to be hard disabled. They make no sense on B2G as their compoenents are not included in this application but they should be run on other environments
Attachment #768610 - Flags: review?(gps)
forgot to note: this patch makes FHR run on B2G (data collection and successful data submission work)
Depends on: 887518
Comment on attachment 768610 [details] [diff] [review]
Migrating FHR to Boot 2 Gecko;

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

Great patch! Not ready for checkin, but a terrific start. I'm kinda surprised how little code we needed to change to make FHR work on B2G!

::: b2g/confvars.sh
@@ +18,5 @@
>  # MOZ_APP_DISPLAYNAME is set by branding/configure.sh
>  
>  MOZ_SAFE_BROWSING=
>  MOZ_SERVICES_COMMON=1
> +MOZ_SERVICES_HEALTHREPORT=1

We can't check this in until we're ready to ship.

You can add this line to your mozconfig file for the same effect.

::: b2g/installer/package-manifest.in
@@ +497,5 @@
> +
> +#ifdef MOZ_DATA_REPORTING
> +@BINPATH@/components/DataReporting.manifest
> +@BINPATH@/components/DataReportingService.js
> +#endif

Before we add this, I'd like to verify that MOZ_DATA_REPORTING isn't enabled in B2G builds today. The reason is I don't want to add a new XPCOM service to B2G until it's ready to ship.

Remove traces of MOZ_SERVICES_HEALTHREPORT from this patch and your mozconfig and run |mach configure|. Is this variable in your config.status file?

::: services/datareporting/DataReportingService.js
@@ -107,5 @@
>          break;
>  
>        case "profile-after-change":
>          this._os.removeObserver(this, "profile-after-change");
> -        this._os.addObserver(this, "sessionstore-windows-restored", true);

As you mentioned, we need a better story here. You should ask some b2g folks if there is an equivalent to sessionstore-windows-restored. If not, we may need some #ifdefs.

::: services/healthreport/HealthReportComponents.manifest
@@ +1,2 @@
>  # Register Firefox Health Report providers.
>  category healthreport-js-provider-default AddonsProvider resource://gre/modules/HealthReport.jsm

Shouldn't AddonsProvider also be behind an #ifdef?

Also, I wonder if we should instead create per-app categories. e.g.

category healthreport-js-provider-default-browser ...
category healthreport-js-provider-default-b2g ...

Or even:

category healthreport-js-provider-default-browser-or-b2g ...

Although we'd have to consider polluting the category manager. Just something to think about.

::: services/healthreport/tests/xpcshell/xpcshell.ini
@@ +9,4 @@
>  [test_provider_addons.js]
> +# TODO: This should be `skip-if = os == "b2g"` but that (or similar attemtps)
> +# don't seem to work so for the duration of Bug 888025 these tests are hard disabled
> +skip-if = true

This will obviously need changed before commit (as you mentioned).
Attachment #768610 - Flags: review?(gps) → feedback+
Status: NEW → ASSIGNED
Assignee: steven.mirea → nobody
Status: ASSIGNED → NEW
FHR is going away per bug 1209088, closing.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.