This involves collecting a bunch of stuff -- add-ons, app info, etc. -- and persisting it, tracking changes and so forth.

Still to do: fetching real values from Gecko; extending this to add-ons.

Again, git has the tests.

This grabs a bunch of stuff out of the profile.

Comment on attachment 751920 [details] [diff] [review] Proposed patch. v2 (from git) Review of attachment 751920 [details] [diff] [review]: ----------------------------------------------------------------- It's all nits all the way down, baby! ::: mobile/android/base/background/healthreport/EnvironmentBuilder.java @@ +21,5 @@ > + return cr.acquireContentProviderClient(HealthReportConstants.HEALTH_AUTHORITY); > + } > + > + public static HealthReportDatabaseStorage getStorage(ContentProviderClient cpc, > + String profilePath) { Is this how Fennec formats things? It's *not* how Android Sync (Eclipse) formats things. @@ +23,5 @@ > + > + public static HealthReportDatabaseStorage getStorage(ContentProviderClient cpc, > + String profilePath) { > + ContentProvider pr = cpc.getLocalContentProvider(); > + return ((HealthReportProvider) pr).getProfileStorage(profilePath); This is just waiting to screw somebody over. Let's catch the |ClassCastException| and provide a helpful error message. @@ +52,5 @@ > + e.sysName = SysInfo.getName(); > + e.sysVersion = SysInfo.getReleaseVersion(); > + > + e.profileCreation = (int) (info.getProfileCreationTime() / HealthReportConstants.MILLISECONDS_PER_DAY); > + e.isBlocklistEnabled = (info.isBlocklistEnabled() ? 1 : 0); // "extensions.blocklist.enabled" nit: Corresponds to Gecko preferences "...". @@ +53,5 @@ > + e.sysVersion = SysInfo.getReleaseVersion(); > + > + e.profileCreation = (int) (info.getProfileCreationTime() / HealthReportConstants.MILLISECONDS_PER_DAY); > + e.isBlocklistEnabled = (info.isBlocklistEnabled() ? 1 : 0); // "extensions.blocklist.enabled" > + e.isTelemetryEnabled = (info.isTelemetryEnabled() ? 1 : 0); // "datareporting.telemetry.enabled" via GeckoPreferences. nit: expand on the difference between desktop and whatever hijinks Android is doing. Bonus points for explaining which is authoritative and which is derived.

Comment on attachment 751926 [details] [diff] [review] Part 2: enhance BrowserHealthRecorder. v1 Review of attachment 751926 [details] [diff] [review]: ----------------------------------------------------------------- I'd really like to see some automated tests for the profile creation time, and a test that "start up creates a reasonable environment". But otherwise it looks okay, modulo that thread TODO. ::: mobile/android/base/health/BrowserHealthRecorder.java @@ +102,5 @@ > > + public synchronized void close(final EventDispatcher dispatcher) { > + switch (this.state) { > + case CLOSED: > + Log.w(LOG_TAG, "Attempting to double-close closed BrowserHealthRecorder."); Perhaps say "ignoring"? This is the flip side of a review comment on a different patch. @@ +108,5 @@ > + case INITIALIZED: > + Log.i(LOG_TAG, "Closing Health Report client."); > + break; > + default: > + Log.i(LOG_TAG, "Closing part-initialized BrowserHealthRecorder."); nit: partly? @@ +117,4 @@ > this.unregisterEventListeners(dispatcher); > this.storage = null; > + if (this.client != null) { > + this.client.release(); this.client = null; @@ +142,5 @@ > */ > public synchronized void onEnvironmentChanged() { > this.env = -1; > + try { > + profileCache.completeInitialization(); This seems wrong to me. Can you explain? @@ +163,5 @@ > return this.env = EnvironmentBuilder.registerCurrentEnvironment(this.storage, > this.profileCache); > } > > + private static long getProfileInitTime(final Context context, final String profilePath) { Nice function, hard as hell to test. Can we have this in 3 pieces, each individually tested? I worry that we'll find a code path not behaving if we don't actually unit test each path. @@ +165,5 @@ > } > > + private static long getProfileInitTime(final Context context, final String profilePath) { > + // Let's look in the profile. > + final File times = new File(profilePath + File.separator + "times.json"); nit: generic filenames are bad for MXR, least surprise, etc. But +1 for .json! @@ +241,5 @@ > + return; > + } > + > + Log.d(LOG_TAG, "Done initializing profile cache. Beginning storage init."); > + Eek! Any reason to not post this to the background handler and be done with it? @@ +269,5 @@ > + } > + }; > + > + // Oh, singletons. > + PrefsHelper.getPrefs(new String[] { It seems this adds an ordering: PrefsHelper must be initialized before BHR. Do you agree? If so, we should try to enforce that ordering, or at least comment on it in GeckoApp.

(In reply to Nick Alexander :nalexander from comment #6) > Is this how Fennec formats things? It's *not* how Android Sync (Eclipse) > formats things. I thought column alignment was our style? This is not our first patch that uses it...

(In reply to Nick Alexander :nalexander from comment #7) > I'd really like to see some automated tests for the profile creation time, > and a test that "start up creates a reasonable environment". But otherwise > it looks okay, modulo that thread TODO. Yeah, I'd like to see those too. They're on my plate for when some of this is more fully baked. > > public synchronized void onEnvironmentChanged() { > > this.env = -1; > > + try { > > + profileCache.completeInitialization(); > > This seems wrong to me. Can you explain? Does this new comment clarify? * Call this when a material change has occurred in the running environment, * such that a new environment should be computed and prepared for use in * future events. * * Invoke this method after calls that mutate the environment, such as * {@link #onBlocklistPrefChanged(boolean to)}. That `completeInitialization` call is the complement to the `beginInitialization` call in those other methods. Yes, you can reinitialize the profile cache. > > + private static long getProfileInitTime(final Context context, final String profilePath) { > > + // Let's look in the profile. > > + final File times = new File(profilePath + File.separator + "times.json"); > > nit: generic filenames are bad for MXR, least surprise, etc. But +1 for > .json! This shipped long ago, with Mossop's approval :) > Eek! Any reason to not post this to the background handler and be done with > it? See question about which thread this runs on. Note that this is just a typical async chain pattern. > It seems this adds an ordering: PrefsHelper must be initialized before BHR. > Do you agree? If so, we should try to enforce that ordering, or at least > comment on it in GeckoApp. PrefsHelper initializes itself; see `ensureRegistered()` inside PrefsHelper.getPrefs.

(In reply to Richard Newman [:rnewman] from comment #9) > (In reply to Nick Alexander :nalexander from comment #7) > > > I'd really like to see some automated tests for the profile creation time, > > and a test that "start up creates a reasonable environment". But otherwise > > it looks okay, modulo that thread TODO. > > Yeah, I'd like to see those too. They're on my plate for when some of this > is more fully baked. > > > > public synchronized void onEnvironmentChanged() { > > > this.env = -1; > > > + try { > > > + profileCache.completeInitialization(); > > > > This seems wrong to me. Can you explain? > > Does this new comment clarify? <snip> Yep! > > > + private static long getProfileInitTime(final Context context, final String profilePath) { > > > + // Let's look in the profile. > > > + final File times = new File(profilePath + File.separator + "times.json"); > > > > nit: generic filenames are bad for MXR, least surprise, etc. But +1 for > > .json! > > This shipped long ago, with Mossop's approval :) Gah! > > Eek! Any reason to not post this to the background handler and be done with > > it? > > See question about which thread this runs on. Note that this is just a > typical async chain pattern. I understand, but since we don't know which thread it will run on, and by posting to the background thread we can cheaply know, why not just do it?

https://hg.mozilla.org/integration/mozilla-inbound/rev/64cbce6df9fb and part of https://hg.mozilla.org/integration/mozilla-inbound/rev/c32533bf03f3

Comment on attachment 751920 [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.

Things we need to verify when we can actually see the generated report: * x86 devices report architecture appropriately. * x86 devices report the target ABI for their build of Firefox appropriately. * armv6 devices similarly. For example, an x86 device running Firefox built for ARM might include: "architecture": "x86", "xpcomabi": "arm-eabi-gcc3" versus an ARMv7 device: "architecture": "armeabi-v7a", "xpcomabi": "arm-eabi-gcc3" This will involve obtaining an ARMv6 device and an x86 device, and both ARM versions and x86 builds of Nightly. Tracy, can you get hold of the appropriate devices?

Comment on attachment 751926 [details] [diff] [review] Part 2: enhance BrowserHealthRecorder. v1 Approving for uplift as part of the body of work for FHR's first revision on Android.

For x86: architecture = x86 xpcomabi = x86-gcc3