Closed Bug 873360 Opened 7 years ago Closed 7 years ago

Profile information storage

Categories

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

ARM
Android
defect
Not set

Tracking

(firefox23 fixed)

RESOLVED FIXED
Firefox 24
Tracking Status
firefox23 --- fixed

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(1 file, 1 obsolete file)

FHR needs access to a certain set of profile information that's hard to get outside of Gecko. Add-ons is one main culprit (Bug 865444), but there's also blocklist and telemetry prefs, and the profile creation time (for which code already exists in JS, but not in Java).

The simplest and most performant solution for this seems to be to dump these values from Fennec after Gecko startup, store them in a single file written in the background, and to load that file lazily when needed.

This bug tracks doing so.
Work in <https://github.com/mozilla-services/android-sync/tree/rnewman/storage>.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attached patch hg patch. v1 (obsolete) — Splinter Review
Attachment #751210 - Flags: feedback?(nalexander)
Attachment #751210 - Attachment is obsolete: true
Attachment #751210 - Flags: feedback?(nalexander)
Attachment #751919 - Flags: review?(nalexander)
Git has the tests.
Blocks: 870925
Comment on attachment 751919 [details] [diff] [review]
Proposed patch. v2 (from git)

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

Fine by me.

::: mobile/android/base/background/healthreport/ProfileInformationCache.java
@@ +37,5 @@
> +  private volatile boolean telemetryEnabled = false;
> +  private volatile long profileCreationTime = 0;
> +
> +  public ProfileInformationCache(String profilePath) {
> +    file = new File(profilePath + File.separator + "info.cache");

nit: this is way too generic a name.  "profile_info_cache.json"?

@@ +47,5 @@
> +    needsWrite = true;
> +    profileCreationTime = 0;
> +  }
> +
> +  private JSONObject toJSON() {

/me likes re-use and testability -- public?

@@ +68,5 @@
> +    profileCreationTime = object.getLong("profileCreated");
> +  }
> +
> +  /**
> +   * Call this *on a background thread* when you're done adding things.

nit: Javadoc wants the <b>!

@@ +69,5 @@
> +  }
> +
> +  /**
> +   * Call this *on a background thread* when you're done adding things.
> +   * @throws IOException 

nit: trailing whitespace, and when does this throw?
Attachment #751919 - Flags: review?(nalexander) → review+
https://hg.mozilla.org/mozilla-central/rev/eacb047aa7f7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 878670
Comment on attachment 751919 [details] [diff] [review]
Proposed patch. v2 (from git)

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

User impact if declined: 
  No FHR in 23.

Testing completed (on m-c, etc.): 
  Baking on m-c for some time. QA overview written up; waiting for QA to execute. Manual testing and automated unit tests.

Risk to taking this patch (and alternatives if risky): 
  None: new work.

String or IDL/UUID changes made by this patch:
  None.
Attachment #751919 - Flags: approval-mozilla-aurora?
Comment on attachment 751919 [details] [diff] [review]
Proposed patch. v2 (from git)

Approving for uplift as part of the body of work for FHR's first revision on Android.
Attachment #751919 - 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.