Closed
Bug 965371
Opened 10 years ago
Closed 10 years ago
Report device type and attributes (tablet, phone, etc.)
Categories
(Firefox Health Report Graveyard :: Client: Android, defect)
Tracking
(fennec+)
RESOLVED
FIXED
Firefox 32
Tracking | Status | |
---|---|---|
fennec | + | --- |
People
(Reporter: rnewman, Assigned: rnewman)
References
Details
Attachments
(4 files, 2 obsolete files)
5.92 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
6.16 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
45.64 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
2.01 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
We could add the phone/tablet check easily. We already include that in the UA and have a simple function to call.
Assignee | ||
Updated•10 years ago
|
tracking-fennec: --- → ?
Comment 2•10 years ago
|
||
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.
Updated•10 years ago
|
Assignee: nobody → rnewman
tracking-fennec: ? → 30+
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
I'm unlikely to get this done before 30 reaches Beta.
Assignee: rnewman → nobody
Status: ASSIGNED → NEW
Updated•10 years ago
|
tracking-fennec: 30+ → +
Blocks: 997487
Assignee | ||
Updated•10 years ago
|
Summary: Report device type and attributes → Report device type and attributes (tablet, phone, etc.)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•10 years ago
|
||
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.
Assignee | ||
Comment 5•10 years ago
|
||
a-s part underway: https://github.com/mozilla-services/android-sync/pull/456
Assignee | ||
Comment 6•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8418212 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 7•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8418212 -
Attachment is obsolete: true
Comment 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
Results for Nexus 10: org.mozilla.device.config: { "hasHardwareKeyboard": false, "screenXInMM": 136, "screenLayout": 4, "uiType": "largeTablet", "screenYInMM": 209, "_v1": 1, "uiMode": 1 }
Assignee | ||
Comment 10•10 years ago
|
||
From android-sync.
Attachment #8418478 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 11•10 years ago
|
||
Fennec part.
Attachment #8418481 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 12•10 years ago
|
||
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.
Assignee | ||
Comment 13•10 years ago
|
||
Tested and working. Man, Java is boilerplatey.
Attachment #8418557 -
Flags: review?(michael.l.comella)
Assignee | ||
Updated•10 years ago
|
Attachment #8418478 -
Attachment is obsolete: true
Attachment #8418478 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8418808 -
Flags: review?(michael.l.comella)
No longer blocks: 997487
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-
Comment 16•10 years ago
|
||
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+
Assignee | ||
Comment 19•10 years ago
|
||
(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.
Assignee | ||
Comment 20•10 years ago
|
||
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+
Assignee | ||
Comment 22•10 years ago
|
||
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
Assignee | ||
Comment 23•10 years ago
|
||
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•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Updated•6 years ago
|
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•