Closed Bug 604712 Opened 10 years ago Closed 10 years ago

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


(Firefox for Android Graveyard :: General, enhancement)

Not set



Tracking Status
fennec 2.0b3+ ---


(Reporter: aakashd, Assigned: dougt)




(3 files)

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

network operator
OS version
device type

The reason is to allow an extension to send that information to us, so we can better categorize information from our feedback mechanisms.
tracking-fennec: --- → ?
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.
not blocking
tracking-fennec: ? → 2.0-
Re-noming for b3
tracking-fennec: 2.0- → ?
Also, the new req is just to get device type in the same manner as (or close to) Crash Reporter.
Summary: Request to have Fennec grab more mobile phone information → Add device information to nsISystemInfo for use by crash reporting and feedback addon.
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.
Assignee: nobody → blassey.bugs
tracking-fennec: ? → 2.0+
Blassey, I believe this is a 2.0b3 blocker.
tracking-fennec: 2.0+ → ?
(In reply to comment #6)
> Blassey, I believe this is a 2.0b3 blocker.

tracking-fennec: ? → 2.0b3+
Attached patch patch v.1Splinter Review


These match up with the android Build. of the same (but uppercase) attributes.  On maemo, we hardcode what is more or less correct.
Assignee: blassey.bugs → doug.turner
Attachment #491249 - Flags: review?(blassey.bugs)
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.
Attachment #491249 - Flags: review?(blassey.bugs) → review+
Blocks: 610494
Closed: 10 years ago
Resolution: --- → FIXED
backed out.  This is causing bug 613321
Resolution: FIXED → ---
Mwu/Brad, is there a way to make GetStaticStringField fail gracefully?
Here you go.
Attachment #492082 - Flags: review?(doug.turner)
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!
Attachment #492082 - Flags: review?(doug.turner) → review+
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
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.
(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 on attachment 492111 [details] [diff] [review]
patch to check version

r+ only for the 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.
Attachment #492111 - Flags: review?(mwu) → review+
How can I verify this?
You need to log in before you can comment on or make changes to this bug.