Closed Bug 876473 Opened 8 years ago Closed 8 years ago

Connect generated health report to about:healthreport

Categories

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

All
Android
defect
Not set
normal

Tracking

(firefox22 unaffected, firefox23+ fixed, firefox24 fixed)

RESOLVED FIXED
Firefox 24
Tracking Status
firefox22 --- unaffected
firefox23 + fixed
firefox24 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(1 file, 1 obsolete file)

On Android, we have an about:healthreport page waiting (Bug 857419) and we're fleshing out the generated report (Dependencies of Bug 858742).  This ticket will track connecting the generated report to about:healthreport.
QA Contact: nalexander
Assignee: nobody → nalexander
QA Contact: nalexander
Comment on attachment 755489 [details] [diff] [review]
Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman

So, this ended up entirely Fennec-side, and I think it's better for it.  There is one TODO to follow up on when Bug 828654 lands.  It's hard to say it's working since the FHR site doesn't provide much in the way of diagnostics.
Attachment #755489 - Flags: review?(rnewman)
Comment on attachment 755489 [details] [diff] [review]
Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman

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

::: mobile/android/base/health/BrowserHealthReporter.java
@@ +71,5 @@
> +        HealthReportDatabases databases = new HealthReportDatabases(context);
> +        try {
> +            // Closed by closing databases in finally block.
> +            HealthReportDatabaseStorage storage = databases.getDatabaseHelperForProfile(profile.getDir());
> +            HealthReportGenerator generator = new HealthReportGenerator(storage);

Don't do this. You'll open a new SQLiteDatabase pointing to the same DB.

Do what BrowserHealthRecorder does, and fetch it from the ContentProvider (using the CP as a lifecycle hack):

---
        this.client = EnvironmentBuilder.getContentProviderClient(context);
        if (this.client == null) {
            throw new IllegalStateException("Could not fetch Health Report content provider.");
        }

        this.storage = EnvironmentBuilder.getStorage(this.client, profilePath);
---

It would also make sense, then, to make this an inner class of BrowserHealthRecorder, or otherwise share that handle.

@@ +88,5 @@
> +     * This method performs IO, so call it from a background thread.
> +     */
> +    public JSONObject generateReport() throws JSONException {
> +        long since = System.currentTimeMillis() - MILLISECONDS_PER_SIX_MONTHS;
> +        long lastPingTime = since; // TODO: read this from SharedPreference owned by background uploader.

final SharedPreferences preferences = context.getSharedPreferences(AnnouncementsConstants.PREFS_BRANCH, GlobalConstants.SHARED_PREFERENCES_MODE);
preferences.getLong(AnnouncementsConstants.PREF_LAST_LAUNCH, -1);

::: mobile/android/chrome/content/aboutHealthReport.js
@@ +18,4 @@
>  // Name of Gecko Pref specifying report content location.
>  const PREF_REPORTURL = "datareporting.healthreport.about.reportUrl";
>  
> +function dump(s) {

Deliberately overriding dump()?
Attachment #755489 - Flags: review?(rnewman)
(In reply to Richard Newman [:rnewman] from comment #4)
> Comment on attachment 755489 [details] [diff] [review]
> Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman
> 
> Review of attachment 755489 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/health/BrowserHealthReporter.java
> @@ +71,5 @@
> > +        HealthReportDatabases databases = new HealthReportDatabases(context);
> > +        try {
> > +            // Closed by closing databases in finally block.
> > +            HealthReportDatabaseStorage storage = databases.getDatabaseHelperForProfile(profile.getDir());
> > +            HealthReportGenerator generator = new HealthReportGenerator(storage);
> 
> Don't do this. You'll open a new SQLiteDatabase pointing to the same DB.
> 
> Do what BrowserHealthRecorder does, and fetch it from the ContentProvider
> (using the CP as a lifecycle hack):
> 
> ---
>         this.client = EnvironmentBuilder.getContentProviderClient(context);
>         if (this.client == null) {
>             throw new IllegalStateException("Could not fetch Health Report
> content provider.");
>         }
> 
>         this.storage = EnvironmentBuilder.getStorage(this.client,
> profilePath);
> ---
> 
> It would also make sense, then, to make this an inner class of
> BrowserHealthRecorder, or otherwise share that handle.
> 
> @@ +88,5 @@
> > +     * This method performs IO, so call it from a background thread.
> > +     */
> > +    public JSONObject generateReport() throws JSONException {
> > +        long since = System.currentTimeMillis() - MILLISECONDS_PER_SIX_MONTHS;
> > +        long lastPingTime = since; // TODO: read this from SharedPreference owned by background uploader.
> 
> final SharedPreferences preferences =
> context.getSharedPreferences(AnnouncementsConstants.PREFS_BRANCH,
> GlobalConstants.SHARED_PREFERENCES_MODE);
> preferences.getLong(AnnouncementsConstants.PREF_LAST_LAUNCH, -1);
> 
> ::: mobile/android/chrome/content/aboutHealthReport.js
> @@ +18,4 @@
> >  // Name of Gecko Pref specifying report content location.
> >  const PREF_REPORTURL = "datareporting.healthreport.about.reportUrl";
> >  
> > +function dump(s) {
> 
> Deliberately overriding dump()?

I've never been clear on what's defined where.  This follows about:reader's lead.  Don't really care one way or the other -- do you prefer console.log, or nothing, or ADB only messages?
(In reply to Nick Alexander :nalexander from comment #5)
> (In reply to Richard Newman [:rnewman] from comment #4)
> > Comment on attachment 755489 [details] [diff] [review]
> > Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman
> > 
> > Review of attachment 755489 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/base/health/BrowserHealthReporter.java
> > @@ +71,5 @@
> > > +        HealthReportDatabases databases = new HealthReportDatabases(context);
> > > +        try {
> > > +            // Closed by closing databases in finally block.
> > > +            HealthReportDatabaseStorage storage = databases.getDatabaseHelperForProfile(profile.getDir());
> > > +            HealthReportGenerator generator = new HealthReportGenerator(storage);
> > 
> > Don't do this. You'll open a new SQLiteDatabase pointing to the same DB.
> > 
> > Do what BrowserHealthRecorder does, and fetch it from the ContentProvider
> > (using the CP as a lifecycle hack):
> > 
> > ---
> >         this.client = EnvironmentBuilder.getContentProviderClient(context);
> >         if (this.client == null) {
> >             throw new IllegalStateException("Could not fetch Health Report
> > content provider.");
> >         }
> > 
> >         this.storage = EnvironmentBuilder.getStorage(this.client,
> > profilePath);
> > ---
> > 
> > It would also make sense, then, to make this an inner class of
> > BrowserHealthRecorder, or otherwise share that handle.

So I didn't fold this into Recorder because I explicitly don't want to have to manage the life cycle: the only way to share the handle safely is to synchronize on the Recorder instance and handle all the different states.  Seems more complicated than necessary.

If I use getContentProviderClient and getStorage, is that cached enough to do each request?
> > +        long lastPingTime = since; // TODO: read this from SharedPreference owned by background uploader.
> 
> final SharedPreferences preferences =
> context.getSharedPreferences(AnnouncementsConstants.PREFS_BRANCH,
> GlobalConstants.SHARED_PREFERENCES_MODE);
> preferences.getLong(AnnouncementsConstants.PREF_LAST_LAUNCH, -1);

I thought lastPingTime was the last time we uploaded a document to the server, not anything about the last time the browser was launched.  A quick look at

http://mxr.mozilla.org/mozilla-central/source/services/healthreport/healthreporter.jsm#131

suggests I'm right.  Was your suggestion just an example?
(In reply to Nick Alexander :nalexander from comment #7)

> suggests I'm right.  Was your suggestion just an example?

Hurrr. I read lastLaunch, not lastPingTime!

(In reply to Nick Alexander :nalexander from comment #6)

> If I use getContentProviderClient and getStorage, is that cached enough to
> do each request?

So long as you hold on to the CPC, Android won't kill it. You can share the storage object safely. You'll get the same CPC (and thus the same CP and storage object) that BrowserHealthRecorder is using, and it might even stick around between activities.

It's strictly more cached than what you're doing in this patch (which is hard-closing everything each time you generate!), so yes, safe to do each request!

(In reply to Nick Alexander :nalexander from comment #5)

> > > +function dump(s) {
> > 
> > Deliberately overriding dump()?
> 
> I've never been clear on what's defined where.  This follows about:reader's
> lead.  Don't really care one way or the other -- do you prefer console.log,
> or nothing, or ADB only messages?

I was under the assumption that dump() was always defined, but perhaps not. If I had the option, I'd go for console.log, but nbd.
Comment on attachment 755638 [details] [diff] [review]
Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman

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

Is this life-cycle right?  I'd really rather not persist the storage object if I don't have to.  I expect this to be called very infrequently, and not at all for most sessions.  Just realized there may be unneeded imports; I will trim imports before landing.
Attachment #755638 - Flags: review?(rnewman)
Attachment #755489 - Attachment is obsolete: true
Just realized there may be unneeded imports; I
> will trim imports before landing.

Just one:

/Users/ncalexan/Mozilla/mcgit/mobile/android/base/health/BrowserHealthReporter.java:17:8: Unused import - org.mozilla.gecko.background.healthreport.HealthReportDatabases.
Btw, I should probably land this on top of my queue...
Status: NEW → ASSIGNED
Comment on attachment 755638 [details] [diff] [review]
Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman

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

I will clean this up and add it to my queue for landing.

::: mobile/android/base/health/BrowserHealthReporter.java
@@ +47,5 @@
> +    // Health Report throughout the browser.  The Provider owns all underlying
> +    // Storage instances, so we don't need to explicitly close them.  Since the
> +    // Provider caches aggressively, we should even reference the same Storage
> +    // instances that BrowserHealthRecorder references and not waste resources.
> +    private ContentProviderClient client;

I would actually go so far as to fetch this within generateReport.

::: mobile/android/chrome/content/aboutHealthReport.js
@@ +23,5 @@
>      .getService(Ci.nsIAndroidBridge)
>      .handleGeckoMessage(JSON.stringify(message));
>  }
>  
> +// about:healthreport prefs are stored Firefox's default Android

stored in.

@@ +35,5 @@
>      let report = this._getReportURI();
>      iframe.src = report.spec;
>  
>      sharedPrefs.addObserver(PREF_UPLOAD_ENABLED, this, false);
> +    Services.obs.addObserver(this, "HealthReport:Response", false);

const.

@@ +98,5 @@
>  
>    injectData: function (type, content) {
>      let report = this._getReportURI();
>  
> +    // File URIs can't be used for targetOrigin, so we use "*" for

Should be "file:"
Attachment #755638 - Flags: review?(rnewman) → review+
Marking as assigned to me; I'll switch back when landing.
Assignee: nalexander → rnewman
(In reply to Richard Newman [:rnewman] from comment #13)
> Comment on attachment 755638 [details] [diff] [review]
> Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman
> 
> Review of attachment 755638 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I will clean this up and add it to my queue for landing.

Don't forget that unused import.

> ::: mobile/android/base/health/BrowserHealthReporter.java
> @@ +47,5 @@
> > +    // Health Report throughout the browser.  The Provider owns all underlying
> > +    // Storage instances, so we don't need to explicitly close them.  Since the
> > +    // Provider caches aggressively, we should even reference the same Storage
> > +    // instances that BrowserHealthRecorder references and not waste resources.
> > +    private ContentProviderClient client;
> 
> I would actually go so far as to fetch this within generateReport.

You know best.
 
> ::: mobile/android/chrome/content/aboutHealthReport.js
> @@ +23,5 @@
> >      .getService(Ci.nsIAndroidBridge)
> >      .handleGeckoMessage(JSON.stringify(message));
> >  }
> >  
> > +// about:healthreport prefs are stored Firefox's default Android
> 
> stored in.
> 
> @@ +35,5 @@
> >      let report = this._getReportURI();
> >      iframe.src = report.spec;
> >  
> >      sharedPrefs.addObserver(PREF_UPLOAD_ENABLED, this, false);
> > +    Services.obs.addObserver(this, "HealthReport:Response", false);
> 
> const.

Maybe take the names from the .java file, so mxr searches are as helpful as possible.

> @@ +98,5 @@
> >  
> >    injectData: function (type, content) {
> >      let report = this._getReportURI();
> >  
> > +    // File URIs can't be used for targetOrigin, so we use "*" for
> 
> Should be "file:"

Thanks for landing!
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b149bc52ebe

With these changes in place, the FHR page will load and receive a document, and will produce console error messages if you try, because the FHR page currently doesn't understand that document format (Bug 863440). It'll get there!
Assignee: rnewman → nalexander
Target Milestone: --- → Firefox 24
https://hg.mozilla.org/mozilla-central/rev/0b149bc52ebe
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment on attachment 755638 [details] [diff] [review]
Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  New work.

User impact if declined: 
  No FHR.

Testing completed (on m-c, etc.): 
  Extensive manual testing on m-c.

Risk to taking this patch (and alternatives if risky): 
  Slim.

String or IDL/UUID changes made by this patch:
  None.
Attachment #755638 - Flags: approval-mozilla-aurora?
Comment on attachment 755638 [details] [diff] [review]
Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman

Approving as part of the body of work needed for FHR on FF23 Android.
Attachment #755638 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.