Closed Bug 965371 Opened 6 years ago Closed 6 years ago

Report device type and attributes (tablet, phone, etc.)

Categories

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

All
Android
defect
Not set

Tracking

(fennec+)

RESOLVED FIXED
Firefox 32
Tracking Status
fennec + ---

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(4 files, 2 obsolete files)

It occurred to me that FHR on Android doesn't offer a way for a document processor to distinguish between a phone, phablet, tablet, or TV — pretty important when you consider that support recommendations depend on UI, problems can fall into categories along these lines, and user behavior differs between these form factors.

I propose that we report some coarse-grained form-factor information in the FHR document payload. For example:

* Hardware keyboard or not?
* Phone, tablet, phablet, …
* Screen depth? (24bit?)
* Physical screen size?
We could add the phone/tablet check easily. We already include that in the UA and have a simple function to call.
tracking-fennec: --- → ?
We should absolutely be reporting phone/tablet, basic hardware characteristics, and screen size both pixels and physical. Please make it so.

I'm a little skeptical that screen depth is actually that useful to us, but I'd love to hear arguments about it.
Assignee: nobody → rnewman
tracking-fennec: ? → 30+
Status: NEW → ASSIGNED
I'm unlikely to get this done before 30 reaches Beta.
Assignee: rnewman → nobody
Status: ASSIGNED → NEW
tracking-fennec: 30+ → +
Summary: Report device type and attributes → Report device type and attributes (tablet, phone, etc.)
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
My proposed format:

  hasHardwareKeyboard: 0,
  uiType: "tablet",
  uiMode: 1,                 // UI_MODE_TYPE_NORMAL
  screenLayout: 4,           // SCREENLAYOUT_SIZE_XLARGE
  screenXInMM: 1240,
  screenYInMM: 942,
  

uiMode and screenLayout take Android constants directly. It seems foolhardy to attempt to translate an ever-growing set.

uiType corresponds to our own concept of whether to show a tablet UI.

Screen dimensions will be calculated from Android's own configuration classes. There's a chance that this will be wrong, but it should be broadly accurate.
We need HardwareUtils outside the main path, so warning every time it's inited is inappropriate.

I also short-cutted a couple of methods.
Attachment #8418212 - Flags: review?(mark.finkle)
Attachment #8418212 - Flags: review?(mark.finkle) → review+
Here's a more aggressive version. By pre-computing the simple flags, we avoid some Boolean checks at runtime, dramatically simplify several methods, eliminate the need to hold on to a static context, and do less work for isTablet and other common tests.

I also set these members to be volatile, 'cos we're freely accessing them without any threading constraints. Not perfect, but better than what's there now.
Attachment #8418223 - Flags: review?(mark.finkle)
Attachment #8418212 - Attachment is obsolete: true
Comment on attachment 8418223 [details] [diff] [review]
Part 2: improve HardwareUtils. v2

Let's use this version and see if anything shakes loose. We might run into a flag that requires ad-hoc computation, but until then this is more simple.
Attachment #8418223 - Flags: review?(mark.finkle) → review+
Depends on: 1006805
Results for Nexus 10:

  org.mozilla.device.config: {
    "hasHardwareKeyboard": false,
    "screenXInMM": 136,
    "screenLayout": 4,
    "uiType": "largeTablet",
    "screenYInMM": 209,
    "_v1": 1,
    "uiMode": 1
  }
From android-sync.
Attachment #8418478 - Flags: review?(michael.l.comella)
Fennec part.
Attachment #8418481 - Flags: review?(michael.l.comella)
HTC One:

"org.mozilla.device.config": {
  "hasHardwareKeyboard": false,
  "screenXInMM": 58,
  "screenLayout": 2,
  "uiType": "default",
  "screenYInMM": 103,
  "_v": 1,
  "uiMode": 1
},


Deltas between v2 and v3 environments work correctly -- the config is omitted.
Tested and working. Man, Java is boilerplatey.
Attachment #8418557 - Flags: review?(michael.l.comella)
Attachment #8418478 - Attachment is obsolete: true
Attachment #8418478 - Flags: review?(michael.l.comella)
Comment on attachment 8418557 [details] [diff] [review]
Part 1: introduce v3 environment format. v2

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

You have a multi-line patch description and I'm not sure what happens when you land on m-c like that.

r- on the mock hash bit.

::: mobile/android/base/background/healthreport/AndroidConfigurationProvider.java
@@ +36,5 @@
> +  public UIType getUIType() {
> +    if (HardwareUtils.isLargeTablet()) {
> +      return UIType.LARGE_TABLET;
> +    }
> +    if (HardwareUtils.isSmallTablet()) {

nit: `} else if (...) {` just because you can fold it into one line.

@@ +62,5 @@
> +  }
> +
> +  @Override
> +  public int getScreenYInMM() {
> +    return Math.round((displayMetrics.heightPixels / displayMetrics.ydpi) * MILLIMETERS_PER_INCH);

nit: Add a comment pointing to getScreenX (just in case), or have them call a shared function with the comment.

::: mobile/android/base/background/healthreport/Environment.java
@@ +25,2 @@
>  
> +  public static enum UIType {

nit: I personally find the below more readable, especially if we get more types. This also allows a more readable/maintainable iteration in `fromLabel` (i.e. `for (UIType type : UIType.values()) ...`).

... enum UIType {
  DEFAULT("default"),
  ...;

  private final String str;
  public UIType(final String str) {
    this.str = str;
  }

  public String toString() {
    return str;
  }
}

::: mobile/android/base/background/healthreport/HealthReportDatabaseStorage.java
@@ +766,5 @@
>        } catch (Exception e) {
>          // Nothing we can do.
>        }
>  
> +      if (version >= 3) {

This makes sense to me, but why don't we have one for version >= 2?

::: mobile/android/tests/background/junit3/src/healthreport/TestEnvironmentBuilder.java
@@ +70,5 @@
>      cache.beginInitialization();
>      cache.setBlocklistEnabled(false);
>      cache.completeInitialization();
>  
> +    assertFalse(EnvironmentBuilder.getCurrentEnvironment(cache, configProvider).getHash()

Perhaps you want to check that the values in the environment match the values you get from Config (perhaps a little redundant from the AndroidConfigurationProvider code).

::: mobile/android/tests/background/junit3/src/healthreport/TestHealthReportGenerator.java
@@ +69,5 @@
>    // v2 fields.
> +  private static final String EXPECTED_MOCK_BASE_HASH_SUFFIX_V2 = "null" + "null" + 0 + "null";
> +
> +  // v3 fields.
> +  private static final String EXPECTED_MOCK_BASE_HASH_SUFFIX_V3 = "" + 0 + "default" + 0 + 0 + 0 + 0;

I don't think this is correct. `hasHardwareKeyboard ? 1 : 0` is the first value appended, and that's not "".
Attachment #8418557 - Flags: review?(michael.l.comella) → review-
Multi-line commit messages are allowed (encouraged, if you're writing a small commit treatise!). But only the first line will show up in most logs, so the first line should list all the bug numbers, reviews, etc.
Comment on attachment 8418481 [details] [diff] [review]
Part 2: report device type and attributes. v1

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

::: mobile/android/base/health/BrowserHealthReporter.java
@@ +25,1 @@
>  import org.json.JSONException;

nit: I don't think you needed to remove the newline above this.
Attachment #8418481 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8418808 [details] [diff] [review]
Part 3: new docs for v3 device configuration environment. v1

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

r+ w/ nits.

::: services/healthreport/docs/dataformat.rst
@@ +329,5 @@
> +    One of "default", "smalltablet", "largetablet".
> +uiMode
> +    A mask of the Android *Configuration.uiMode* value, e.g., *UI_MODE_TYPE_CAR*.
> +screenLayout
> +    A mask of the Android *Configuration.SCREENLAYOUT_SIZE_* constants.

nit: screenLayout and uiMode are inconsistent. uiMode should probably be:

"A mask of the Android *Configuration.UI_MODE_TYPE_* value"

@@ +330,5 @@
> +uiMode
> +    A mask of the Android *Configuration.uiMode* value, e.g., *UI_MODE_TYPE_CAR*.
> +screenLayout
> +    A mask of the Android *Configuration.SCREENLAYOUT_SIZE_* constants.
> +    

nit: ws
Attachment #8418808 - Flags: review?(michael.l.comella) → review+
(In reply to Michael Comella (:mcomella) from comment #15)

> > +      return UIType.LARGE_TABLET;
> > +    }
> > +    if (HardwareUtils.isSmallTablet()) {
> 
> nit: `} else if (...) {` just because you can fold it into one line.

Counter-nit: we have a rule, 'round these here parts: "no else after return". ;P

Newlines are cheap.


> nit: Add a comment pointing to getScreenX (just in case), or have them call
> a shared function with the comment.

Done.

> I personally find the below more readable, especially if we get more

Done.


> > +      if (version >= 3) {
> 
> This makes sense to me, but why don't we have one for version >= 2?

Because the version 2 types were all "safe" -- no enums to munge, for example.


> Perhaps you want to check that the values in the environment match the
> values you get from Config (perhaps a little redundant from the
> AndroidConfigurationProvider code).

Added some basic checks.


> > +  private static final String EXPECTED_MOCK_BASE_HASH_SUFFIX_V3 = "" + 0 + "default" + 0 + 0 + 0 + 0;
> 
> I don't think this is correct. `hasHardwareKeyboard ? 1 : 0` is the first
> value appended, and that's not "".

The <"" +> trick is one way of ensuring that you get a string out at the other end in dynamic languages such as JS.

$ 0 + 1 + "foo"
"1foo"

$ "" + 0 + 1 + "foo"
"01foo"

This is just a clue and reassurance to folks used to defending themselves against the 'help' their language offers.

If you doubt: count the number of + signs you see (6). That means seven things being concatenated. There are only six items in the new environment.
Comment on attachment 8418557 [details] [diff] [review]
Part 1: introduce v3 environment format. v2

Requesting re-review further to comments.
Attachment #8418557 - Flags: review- → review?(michael.l.comella)
Comment on attachment 8418557 [details] [diff] [review]
Part 1: introduce v3 environment format. v2

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

Assuming you made the changes from comment 19 locally, lgtm.

(In reply to Richard Newman [:rnewman] from comment #19)
> Newlines are cheap.

counter-counter nit: Then I would find it more readable to add a newline between the if statements. :)

(AndroidConfigurationProvider.getUIType so you don't need to back reference)
Attachment #8418557 - Flags: review?(michael.l.comella) → review+
I needed a follow-up: neglected to run the full test suite, and fixed multi-step upgrades.

https://hg.mozilla.org/integration/fx-team/rev/e41c8995d8dc
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.