604712 - Add device information to nsISystemInfo for use by crash reporting and feedback addon.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Aakash Desai [:aakashd]]

Here's a list of things I'd like Fennec to grab (and store within prefs):

network operator
country
OS version
device type
manufacturer

The reason is to allow an extension to send that information to us, so we can better categorize information from our feedback mechanisms.

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

When implementing, please consider having this an option somewhere to allow the user to send this data and what data they are sending and why.

Reason: IMO, one of the biggest strength Mozilla has is that people trust Mozilla for upholding privacy information and letting users have control.  Having this an option and let the end user know exactly what they are sharing may help maintain the trust.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

not blocking

Comment 3

[Aakash Desai [:aakashd]]

Re-noming for b3

Comment 4

[Aakash Desai [:aakashd]]

Also, the new req is just to get device type in the same manner as (or close to) Crash Reporter.

Comment 5

[Doug Turner (:dougt)]

aakash - don't worry about the implementation.  just explain what you need.  The subject was extra scary when I read it.

naoki - no idea what your talking.  i think it is unrelated to this bug.

Comment 6

[Aakash Desai [:aakashd]]

Blassey, I believe this is a 2.0b3 blocker.

Comment 7

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

(In reply to comment #6)
> Blassey, I believe this is a 2.0b3 blocker.

indeed

Comment 8

[Doug Turner (:dougt)]

Attachment: patch v.1

Adds:

manufacturer
hardware
fingerprint

These match up with the android Build. of the same (but uppercase) attributes.  On maemo, we hardcode what is more or less correct.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 491249 [details] [diff] [review]
patch v.1

>diff --git a/xpcom/base/nsSystemInfo.cpp b/xpcom/base/nsSystemInfo.cpp
>--- a/xpcom/base/nsSystemInfo.cpp
>+++ b/xpcom/base/nsSystemInfo.cpp
>@@ -109,36 +109,49 @@ nsSystemInfo::Init()
>     size_t  len = 0;
>     ssize_t read;
>     FILE *fp = fopen ("/proc/component_version", "r");
>     if (fp) {
>       while ((read = getline(&line, &len, fp)) != -1) {
>         if (line) {
>           if (strstr(line, "RX-51")) {
>             SetPropertyAsACString(NS_ConvertASCIItoUTF16("device"), NS_LITERAL_CSTRING("Nokia N900"));
>+            SetPropertyAsACString(NS_ConvertASCIItoUTF16("manufacturer"), NS_LITERAL_CSTRING("Nokia"));
>+            SetPropertyAsACString(NS_ConvertASCIItoUTF16("hardware"), NS_LITERAL_CSTRING("RX-51"));
>+            SetPropertyAsACString(NS_ConvertASCIItoUTF16("fingerprint"), NS_LITERAL_CSTRING("Nokia N900 RX-51"));
>             break;
>           } else if (strstr(line, "RX-44") ||
>                      strstr(line, "RX-48") ||
>                      strstr(line, "RX-32") ) {
>+            /* not as accurate as we can be, but these devices are deprecated */
>             SetPropertyAsACString(NS_ConvertASCIItoUTF16("device"), NS_LITERAL_CSTRING("Nokia N8xx"));
>+            SetPropertyAsACString(NS_ConvertASCIItoUTF16("manufacturer"), NS_LITERAL_CSTRING("Nokia"));
>+            SetPropertyAsACString(NS_ConvertASCIItoUTF16("hardware"), NS_LITERAL_CSTRING("Nokia N8xx"));
>+            SetPropertyAsACString(NS_ConvertASCIItoUTF16("fingerprint"), NS_LITERAL_CSTRING("Nokia N8xx"));

use NS_LITERAL_STRING() and not NS_ConvertASCIItoUTF16() for all of these

Two suggestions. Drop "Nokia" from the hardware line (no need to repeat that from the manufacture). And in clude the model number ("RX-##") in the fingerprint.

Comment 10

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/6cfd43067a07

Comment 11

[Stuart Parmenter]

backed out.  This is causing bug 613321

Comment 12

[Doug Turner (:dougt)]

Mwu/Brad, is there a way to make GetStaticStringField fail gracefully?

Comment 13

[Michael Wu [:mwu]]

Attachment: Make autoframes clean up unhandled exceptions

Here you go.

Comment 14

[Doug Turner (:dougt)]

Comment on attachment 492082 [details] [diff] [review]
Make autoframes clean up unhandled exceptions

I am not sure about the PushLocalFrame() change.  Please add a comment to why adding 1 makes a different.  On IRC, you mentioned that you're reserving space for the exception, but weren't sure if that was required.

Otherwise Woot!

Comment 15

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/ca12132a0d8c
http://hg.mozilla.org/mozilla-central/rev/91980a82eeae

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

While catching the exception is a good thing, we know it'll throw if the android api level is less that 8, so let's avoid making the call in that case.

One thing I don't understand is why GetStaticFieldID() isn't returning null and allowing us to bail before the exception.

Comment 17

[Michael Wu [:mwu]]

(In reply to comment #16)
> One thing I don't understand is why GetStaticFieldID() isn't returning null and
> allowing us to bail before the exception.

It does both. Not clearing the exception causes problems.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to check version

Comment 19

[Michael Wu [:mwu]]

Comment on attachment 492111 [details] [diff] [review]
patch to check version

r+ only for the CrashReporter.java.in part.

I don't feel the additional complexity of version checking is currently necessary when we want to dump pre-froyo anyway. The code is fine however, in case we have a more compelling reason to grab integers from static classes in the future.

An alternative would be to store this value somewhere in AndroidBridge since the sdk version is the only integer of interest in the build object, and this could be something different parts of the code will want to easily access.

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

pushed the CrashReporter part http://hg.mozilla.org/mozilla-central/rev/35b96b38d398

Comment 21

[Anna (Waverley)]

How can I verify this?