872383 - Add Java equivalents of nsIXULAppInfo and nsSystemInfo to AppConstants or similar

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Richard Newman [:rnewman]]

It'd be very convenient for FHR, and presumably other features, to have ready and idiomatic access to all of the info that lives in sysinfo and appinfo -- CPU count, memory size, OS, Gecko version, etc.

I'm already tackling this for FHR environment calculations, so I propose rolling these two bundles of stuff into public inner classes of AppConstants (which is already preprocessed), so that we can do

  AppConstants.SysInfo.architecture

(or getArchitecture()), rather than reinventing the wheel, or accidentally ending up with subtle differences in data between JS and Java.

I'm also amenable to having org.mozilla.gecko.Info.{System,App}

Thoughts?

References:

https://developer.mozilla.org/en-US/docs/Using_nsIXULAppInfo

xpcom/base/nsSystemInfo.cpp
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIPropertyBag2

Comment 1

[Richard Newman [:rnewman]]

Scope notes for the worried:

* No setting of values. This ain't a property bag.
* Not every field. We don't care if MMX is enabled. (I misspoke when I wrote "all" in Comment 0.)

Comment 2

[Richard Newman [:rnewman]]

Example values (from to-be-removed debug output):

I/GeckoAppInfo( 9609): appBuildID: 20130515145150
I/GeckoAppInfo( 9609): version: 24.0a1
I/GeckoAppInfo( 9609): id: {aa3c5121-dab2-40e2-81ca-7ea25febc110}
I/GeckoAppInfo( 9609): name: fennec
I/GeckoAppInfo( 9609): vendor: Mozilla
I/GeckoAppInfo( 9609): platformVersion: 24.0a1
I/GeckoAppInfo( 9609): platformBuildID: 20130515145150
I/GeckoAppInfo( 9609): os: Android
I/GeckoAppInfo( 9609): xpcomabi: arm-eabi-gcc3
I/GeckoSysInfo( 9609): Arch: arm
I/GeckoSysInfo( 9609): CPUCount: 4
I/GeckoSysInfo( 9609): Device: HTC One
I/GeckoSysInfo( 9609): Hardware: m7
I/GeckoSysInfo( 9609): Manufacturer: HTC
I/GeckoSysInfo( 9609): MemSize: 1567
I/GeckoSysInfo( 9609): Name: Android
I/GeckoSysInfo( 9609): ReleaseVersion: 4.1.2
I/GeckoSysInfo( 9609): Version: 16

Comment 3

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

This is what FHR needs. There are a few other consumers we might be able to switch, and some simplification possible in AppConstants…

Comment 4

[Richard Newman [:rnewman]]

Two explanations:

* I sorted the DEFINES list, because it was unmanageable.
* I decided to put this in a separate file, because AppConstants was getting... very non-constanty. I'd be happy to rename this to something like "Environment" and have Environment.App supplant AppConstants.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 750099 [details] [diff] [review]
Proposed patch. v1

Wes, over to you! I'd be really grateful if you could get this checked over ASAP.

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 750099 [details] [diff] [review]
Proposed patch. v1

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

::: mobile/android/base/Info.java.in
@@ +20,5 @@
> +/**
> + * A collection of system and app info values, broadly mirroring a subset of
> + * nsSystemInfo and nsIXULAppInfo.
> + */
> +public final class Info {

Hiding these inside an Info class seems a bit like overkill to me. How about we put all the AppInfo stuff in AppConstants.java and then just have a SysInfo class here? I realize that naming sucks, but I'd rather have fewer classes.

@@ +37,5 @@
> +           * We can't use a nice tidy API call, because they're all wrong:
> +           * 
> +           * <http://stackoverflow.com/questions/7962155/how-can-you-detect-a-dual-core-
> +           * cpu-on-an-android-device-from-code>
> +           * 

nit: trailing whitespace

@@ +51,5 @@
> +
> +            class CpuFilter implements FileFilter {
> +                @Override
> +                public boolean accept(File pathname) {
> +                    return Pattern.matches("cpu[0-9]", pathname.getName());

This will limit us to at most 10 cpus. Seems like adding a plus is an easy win!

@@ +69,5 @@
> +         * Fetch the total memory of the device in MB by parsing /proc/meminfo.
> +         * 
> +         * Of course, Android doesn't have a neat and tidy way to find total
> +         * RAM, so we do it by parsing /proc/meminfo.
> +         * 

nit: trailing whitespace

@@ +83,5 @@
> +                try {
> +                    String memTotal = reader.readLine();
> +
> +                    // Double-check.
> +                    if (!memTotal.startsWith("MemTotal: ")) {

I'm not sure this is guaranteed to be the first line.

@@ +153,5 @@
> +        public static String getKernelVersion() {
> +            // Corresponds to nsSystemInfo's "version".
> +            // PR_SI_RELEASE.
> +            // e.g., "3.4.10-g42e6c45"
> +            // TODO:

nit: These comments seem a bit cryptic. Can you make them into a sentence?

@@ +199,5 @@
> +        public static final String vendor = "@MOZ_APP_VENDOR@";
> +        public static final String platformVersion = "@GRE_MILESTONE@";
> +
> +        public static final String os = "@OS_TARGET@";
> +        public static final String xpcomabi = "@TARGET_XPCOM_ABI@";

Most of these are already in AppContants.java, and we should add the ones that aren't there to avoid adding more pre-processed files. Similarly for the @CPU_ARCH@ bit above. I would just put it in AppConstants (our one true source of pre-processing) and pull it in here if you want.

Comment 7

[Richard Newman [:rnewman]]

These were both hidden under Info to avoid preprocessing another file. And I'd much rather move stuff out of AppConstants to use this than go the other way; AppConstants is turning into a grab-bag of #define imports without structure or ties to the rest of our codebase.

If we're concerned with the number of preprocessed files, we should do what I suggested in Comment 4 and merge AppConstants into Info, possibly renaming it to be more encompassing. 

Thoughts?

Comment 8

[Richard Newman [:rnewman]]

(In reply to Wesley Johnston (:wesj) from comment #6)

> Hiding these inside an Info class seems a bit like overkill to me. How about
> we put all the AppInfo stuff in AppConstants.java and then just have a
> SysInfo class here? I realize that naming sucks, but I'd rather have fewer
> classes.

Oh, I misread this comment. (Was reading on phone.)

We could do that, sure.

> I'm not sure this is guaranteed to be the first line.

The sad thing is that Android has com.android.internal.util.MemInfoReader to do exactly this parsing, and it does it via string comparisons. It's horrific.

But that class does imply that it's in the first three lines, and probably the first. I'll change it to check three lines.

Comment 9

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v2

Comments addressed (and a few more besides). Carrying forward review.

Thanks, Wes!

Comment 10

[Wesley Johnston (:wesj)]

(In reply to Richard Newman [:rnewman] from comment #8)
> But that class does imply that it's in the first three lines, and probably
> the first. I'll change it to check three lines.

That sounds good. I saw some redhat docs that had some basic header stuff at the top, but I don't know much about this either.

Comment 11

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac01a5774db

Tracking 23, because this'll be part of the FHR uplift pile.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ac01a5774db

Comment 13

[Richard Newman [:rnewman]]

Follow-up to fix an issue on API 8 (Bug 875000):

https://hg.mozilla.org/integration/mozilla-inbound/rev/2610c01d749f

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 750198 [details] [diff] [review]
Proposed patch. v2

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

User impact if declined: 
  No FHR.

Testing completed (on m-c, etc.): 
  Been baking on m-c for a while. Manual testing. (End-to-end testing will be part of FHR pre-uplift; this is a util dependency.)

Risk to taking this patch (and alternatives if risky): 
  Essentially none; new code that's only called by FHR.

String or IDL/UUID changes made by this patch:
  None.

Comment 15

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/624e976d35f0