857419 - Implement about:healthreport wrapper on Android

Status: RESOLVED FIXED

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

Description

[Nick Alexander :nalexander [he/him]]

There are a few parts to this ticket:

* copy-and-modify browser/base/content/abouthealthreport/* to mobile/android/chrome/content
* ensure Gecko prefs code is round-tripped through Java preferences so that background service and front-end display are kept in sync
* implement postMessage API backed by Java for fetching latest document, triggering document generation, etc

There's a strong chance that the existing content served for desktop will not gracefully degrade on mobile devices, but we'll file additional tickets as and when that is observed.

Comment 1

[Nick Alexander :nalexander [he/him]]

Work in progress at

https://github.com/ncalexan/mozilla-central/tree/nalexander/bug-857419-about-healthreport

Comment 2

[Nick Alexander :nalexander [he/him]]

Progress update: first version that loads the iframe correctly posted to github.  Next up: wire up communication between Javascript and Java.

Comment 3

[Nick Alexander :nalexander [he/him]]

Here's a try build exercising Javascript robocop tests:

https://tbpl.mozilla.org/?tree=Try&rev=63977c0b128c

Comment 4

[Nick Alexander :nalexander [he/him]]

This try build includes about:healthreport, providing I haven't made an error in mobile/android/confvars.sh.

This lands one new string in

mobile/android/locales/en-US/chrome/aboutHealthReport.dtd

but this is a direct copy of

browser/locales/en-US/chrome/browser/aboutHealthReport.dtd

which has already been translated for desktop.  How do we inform l10n team that this work is already done and just needs to be duplicated?

Comment 5

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #4)
> This try build includes about:healthreport, providing I haven't made an
> error in mobile/android/confvars.sh.
> 
> This lands one new string in
> 
> mobile/android/locales/en-US/chrome/aboutHealthReport.dtd
> 
> but this is a direct copy of
> 
> browser/locales/en-US/chrome/browser/aboutHealthReport.dtd
> 
> which has already been translated for desktop.  How do we inform l10n team
> that this work is already done and just needs to be duplicated?

The only way I know how. Hi, Axel! :D

This should be landing in 23.

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 857419 - Implement about:healthreport on Android. r=rnewman

Comment 7

[Nick Alexander :nalexander [he/him]]

https://tbpl.mozilla.org/?tree=Try&rev=1b77c88798f2

Comment 8

[Nick Alexander :nalexander [he/him]]

Things I'd like QA to manually verify:

1. about:healthreport is viewable.

There is no data yet, so the data display should be empty -- but it should show the content page, and it shouldn't fail noisily.

2. page title and favicon look correct.

3. tapping the "upload enabled" button works, and preserves state across application restarts.

Once https://bugzilla.mozilla.org/show_bug.cgi?id=833625 lands:

4. "upload enabled" button plays nicely with Settings checkbox -- changes are reflected in both places, etc.

5. on a fresh profile, about:healthreport shows enabled by default.  That is, the data reporting notification is shown on first run, and then subsequently navigating to about:healthreport shows the "upload is enabled" green flag.

Comment 9

[Nick Alexander :nalexander [he/him]]

Mercurial 2.0.2 seems to have a Mac OS X filename case bug.  Try

https://tbpl.mozilla.org/?tree=Try&rev=85dad1ab2455

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 748290 [details] [diff] [review]
Bug 857419 - Implement about:healthreport on Android. r=rnewman

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

::: mobile/android/chrome/content/aboutHealthReport.js
@@ +23,5 @@
> +let sharedPrefs = new SharedPreferences();
> +
> +// Name of Android SharedPreference controlling whether to upload
> +// health reports.
> +let uploadEnabledPref = "android.not_a_preference.healthreport.uploadEnabled";

const.

@@ +26,5 @@
> +// health reports.
> +let uploadEnabledPref = "android.not_a_preference.healthreport.uploadEnabled";
> +
> +// Action sent via Android Ordered Broadcast to background service.
> +let healthReportBroadcastAction = "@ANDROID_PACKAGE_NAME@" + ".healthreport.request";

const, and move these things to the top.

@@ +29,5 @@
> +// Action sent via Android Ordered Broadcast to background service.
> +let healthReportBroadcastAction = "@ANDROID_PACKAGE_NAME@" + ".healthreport.request";
> +
> +// Name of Gecko Pref specifying report content location.
> +let reportUrlPref = "datareporting.healthreport.about.reportUrl";

const.

@@ +72,5 @@
> +
> +  recordHealthReportUploadEnabled: function(enabled) {
> +    enabled = !!enabled;
> +
> +    sharedPrefs.setBoolPref(uploadEnabledPref, enabled);

Just inline the !!.

::: mobile/android/chrome/jar.mn
@@ +38,4 @@
>    content/SelectionHandler.js          (content/SelectionHandler.js)
>    content/HelperApps.js                (content/HelperApps.js)
>    content/dbg-browser-actors.js        (content/dbg-browser-actors.js)
> +  content/WebAppRT.js                  (content/WebAppRT.js)

Good catch.

Comment 11

[Richard Newman [:rnewman]]

Waiting to land this in services after Bug 833625.

Comment 12

[Richard Newman [:rnewman]]

Attachment: Implement about:healthreport on Android. v2

Nits addressed.

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 748355 [details] [diff] [review]
Implement about:healthreport on Android. v2

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

::: mobile/android/components/AboutRedirector.js
@@ +76,5 @@
> +  healthreport: {
> +    uri: "chrome://browser/content/aboutHealthReport.xhtml",
> +    privileged: true
> +  },
> +#ifdef MOZ_SERVICES_HEALTHREPORT

WHOOPS

Comment 14

[Richard Newman [:rnewman]]

https://hg.mozilla.org/services/services-central/rev/dec449c6ac15

Comment 15

[Nick Alexander :nalexander [he/him]]

Follow-up:

https://hg.mozilla.org/services/services-central/rev/7130e5134a6e

Comment 16

[Axel Hecht [:Pike]]

(In reply to Richard Newman [:rnewman] from comment #5)

> This should be landing in 23.

We should have a no-string impact patch for aurora.

Comment 17

[Richard Newman [:rnewman]]

It's landing in m-c; the only l10n note is that these are strings that already exist for desktop.

Comment 18

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/dec449c6ac15
https://hg.mozilla.org/mozilla-central/rev/7130e5134a6e

Comment 19

[Teodora Vermesan (:TeoVermesan)]

Build: Firefox for Android 23.0a1(2013-05-14) 
Device: Asus Transformer TF 101
OS: Android 4.0.3

Based on comment #8 
- about:healthreport is viewable.
- page title and favicon look correct
- the preference is controllable via Settings > Data Choices > Nightly Health Report
- changing the preference in Settings > Data Choices > Nightly Health Report  changes correctly in about:healthreport
- with a fresh profile, health report preference is enabled by default. Going to about:healthreport shows the "Data Sharing" green flag.

Comment 20

[Teodora Vermesan (:TeoVermesan)]

Test Cases added in moztrap for Health Report:
https://moztrap.mozilla.org/manage/case/8133/
https://moztrap.mozilla.org/manage/case/8134/
https://moztrap.mozilla.org/manage/case/8135/