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

Status: RESOLVED FIXED

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

Description

[Richard Newman [:rnewman]]

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?

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

We could add the phone/tablet check easily. We already include that in the UA and have a simple function to call.

Comment 2

[Benjamin Smedberg]

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.

Comment 3

[Richard Newman [:rnewman]]

I'm unlikely to get this done before 30 reaches Beta.

Comment 4

[Richard Newman [:rnewman]]

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.

Comment 5

[Richard Newman [:rnewman]]

a-s part underway:

https://github.com/mozilla-services/android-sync/pull/456

Comment 6

[Richard Newman [:rnewman]]

Attachment: Part 2: improve HardwareUtils. v1

We need HardwareUtils outside the main path, so warning every time it's inited is inappropriate.

I also short-cutted a couple of methods.

Comment 7

[Richard Newman [:rnewman]]

Attachment: Part 2: improve HardwareUtils. v2

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.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 9

[Richard Newman [:rnewman]]

Results for Nexus 10:

  org.mozilla.device.config: {
    "hasHardwareKeyboard": false,
    "screenXInMM": 136,
    "screenLayout": 4,
    "uiType": "largeTablet",
    "screenYInMM": 209,
    "_v1": 1,
    "uiMode": 1
  }

Comment 10

[Richard Newman [:rnewman]]

Attachment: Part 1: introduce v3 environment format. v1

From android-sync.

Comment 11

[Richard Newman [:rnewman]]

Attachment: Part 2: report device type and attributes. v1

Fennec part.

Comment 12

[Richard Newman [:rnewman]]

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.

Comment 13

[Richard Newman [:rnewman]]

Attachment: Part 1: introduce v3 environment format. v2

Tested and working. Man, Java is boilerplatey.

Comment 14

[Richard Newman [:rnewman]]

Attachment: Part 3: new docs for v3 device configuration environment. v1

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 "".

Comment 16

[Benjamin Smedberg]

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 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 19

[Richard Newman [:rnewman]]

(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 20

[Richard Newman [:rnewman]]

Comment on attachment 8418557 [details] [diff] [review]
Part 1: introduce v3 environment format. v2

Requesting re-review further to comments.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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)

Comment 22

[Richard Newman [:rnewman]]

   https://hg.mozilla.org/integration/fx-team/rev/58de18fb0b29
   https://hg.mozilla.org/integration/fx-team/rev/23b496eff5b8
   https://hg.mozilla.org/integration/fx-team/rev/07663727c05d

Comment 23

[Richard Newman [:rnewman]]

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

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/58de18fb0b29
https://hg.mozilla.org/mozilla-central/rev/23b496eff5b8
https://hg.mozilla.org/mozilla-central/rev/07663727c05d
https://hg.mozilla.org/mozilla-central/rev/e41c8995d8dc