We need a provider for Firefox Health Report that reports basic application info. Patch coming soon...

This collects basic application info for FHR. rnewman and gavin get primary/code review. Unfocused signs off that obtaining hotfix info via direct preference lookup is acceptable. The alternative would be going through an API somewhere. But, I'm not sure what that would be.

Comment on attachment 679390 [details] [diff] [review] AppInfo provider, v1 Review of attachment 679390 [details] [diff] [review]: ----------------------------------------------------------------- I'm pretty happy with this. I am very keen to hear what Gavin and Blair say, because this is starting to touch on stuff that's less familiar to me! ::: services/healthreport/healthreporter.jsm @@ +36,5 @@ > } > > this._collector = new MetricsCollector(); > + > + this._collector.registerProvider(new AppInfoProvider()); Mental note to find a way of doing this that scales better. ::: services/healthreport/providers.jsm @@ +49,5 @@ > + }, > + > + name: { > + type: "TYPE_STRING", > + }, Let's break convention here for the sake of readability. vendor: { type: "TYPE_STRING" }, name: { type: "TYPE_STRING" }, And let's put these in lexicographic order. You might also consider introducing BASIC_STRING = { type: "TYPE_STRING" }; somewhere, because we're going to be doing this a lot. @@ +122,5 @@ > + appBuildID: "appBuildID", > + platformVersion: "platformVersion", > + platformBuildID: "platformBuildID", > + > + // From nsIXULRuntime Trailing period. @@ +164,5 @@ > + this._prefs.get("distribution.id", "")); > + result.setValue("appinfo", "distributionVersion", > + this._prefs.get("distribution.version", "")); > + result.setValue("appinfo", "hotfixVersion", > + this._prefs.get("extensions.hotfix.lastVersion", "")); I would generally prefer long lines here than splits. My friends doing code review on flickering green-screen terminals might disagree. @@ +168,5 @@ > + this._prefs.get("extensions.hotfix.lastVersion", "")); > + > + let locale = Cc["@mozilla.org/chrome/chrome-registry;1"] > + .getService(Ci.nsIXULChromeRegistry) > + .getSelectedLocale("global"); This can throw, according to my reading of nsChromeRegistryChrome.cpp. You should wrap the locale lookup in an addError catcher. ::: services/healthreport/tests/xpcshell/test_provider_appinfo.js @@ +62,5 @@ > + do_check_true(result.measurements.has("appinfo")); > + do_check_eq(result.errors.length, 0); > + > + let ai = result.measurements.get("appinfo"); > + do_check_eq(ai.getValue("vendor"), "Mozilla"); Non-comment: the usual convention in the wider world (e.g., JUnit) is test(expected, actual). I wonder why we do it differently? Worth introducing function do_check_value_eq(in, val, expected) { do_check_eq(in.getValue(val), expected); } in our head file, I think.

Something else that came to mind was the act of synchronously populating a result from the same function call that creates it is inherently dangerous. If you throw anywhere, the caller doesn't get the result. It has no opportunity to capture partial data. It won't know what measurements are expected. We may want to draw a line in the API between "return an empty result object" and "populate that result object." If the latter throws, we can catch that in the collector and still submit partial data or a signal to the effect. What do you think, rnewman?

(In reply to Gregory Szorc [:gps] from comment #3) > What do you think, rnewman? Anything that makes collection more robust against runtime or programming errors is a good idea IMO. This doesn't mean we shouldn't strive to make population never throw, of course, but I'm a belt-and-braces kind of chap. Getting submitted statistics for which providers are throwing would be a pleasant introspection…

Comment on attachment 679390 [details] [diff] [review] AppInfo provider, v1 Review of attachment 679390 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/healthreport/healthreporter.jsm @@ +36,5 @@ > } > > this._collector = new MetricsCollector(); > + > + this._collector.registerProvider(new AppInfoProvider()); The Add-ons Manager solves the same issue by using category manager entries, which works quite well. Add a "category" line to a chrome.manifest file, with whatever category name, the entry name as the provider's name, and the value as the URL to the JSM. eg: category healthreporter-provider-module AppInfoProvider resource://gre/modules/services/healthreport/providers.jsm Then you just look at all entries for that category. ::: services/healthreport/providers.jsm @@ +163,5 @@ > + result.setValue("appinfo", "distributionID", > + this._prefs.get("distribution.id", "")); > + result.setValue("appinfo", "distributionVersion", > + this._prefs.get("distribution.version", "")); > + result.setValue("appinfo", "hotfixVersion", Yea, just looking at the hotfix pref is fine. We don't have any API for that (we do for the hotfix ID, but that's not useful here) - easy to add it if really wanted, but it would just do what you're doing here. @@ +168,5 @@ > + this._prefs.get("extensions.hotfix.lastVersion", "")); > + > + let locale = Cc["@mozilla.org/chrome/chrome-registry;1"] > + .getService(Ci.nsIXULChromeRegistry) > + .getSelectedLocale("global"); As rnewman said, this can technically throw, but in practice it won't for "global".

ooo - I didn't know about category managers. I've got the code plugged in now and it will make the next patch on bug 808219! Thanks!

Now using new collection API and category manager for registration.

We should probably limit the category registration to specific application UUIDs. I'm assuming the set will be {dekstop, mobile/xul, mobile/android, metro} (the omission from where the service is installed is B2G, which will require a custom provider - I think).

Comment on attachment 679799 [details] [diff] [review] AppInfo provider, v2 Review of attachment 679799 [details] [diff] [review]: ----------------------------------------------------------------- I know nothing about whether the category registration is correct. Otherwise, r+ when you satisfy the philikon on all of our shoulders. ::: services/healthreport/providers.jsm @@ +43,5 @@ > + __proto__: MetricsMeasurement.prototype, > + > + fields: { > + vendor: { > + type: "TYPE_STRING", I still have a strong preference for (a) not wasting four lines per field, and (b) not allocating fourteen objects — one for each identical field description — where one would do. Field descriptions are immutable, so sharing them should be entirely safe. Let's do it.

I think the changes are trivial. Requesting re-review regardless (the review should be trivial then!).

https://hg.mozilla.org/projects/larch/rev/41591fdc0e3e Landed without application limiting in the manifest file. We'll figure this out as part of the merge.

Comment on attachment 680160 [details] [diff] [review] AppInfo provider, v3 FHR for B2G ADU ping, won't be built/enabled for Mobile/Desktop.

https://hg.mozilla.org/releases/mozilla-aurora/rev/40368dea73a2 https://hg.mozilla.org/releases/mozilla-beta/rev/1992b971397d