Closed Bug 604712 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox for Android Graveyard :: General, enhancement)

ARM
All
enhancement
Not set

Tracking

(fennec2.0b3+)

RESOLVED FIXED
Tracking Status
fennec 2.0b3+ ---

People

(Reporter: aakashd, Assigned: dougt)

References

Details

Attachments

(3 files)

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

indeed
tracking-fennec: ? → 2.0b3+
Attached patch patch v.1Splinter Review
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.
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
http://hg.mozilla.org/mozilla-central/rev/6cfd43067a07
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
backed out.  This is causing bug 613321
Status: RESOLVED → REOPENED
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+
http://hg.mozilla.org/mozilla-central/rev/ca12132a0d8c
http://hg.mozilla.org/mozilla-central/rev/91980a82eeae
Status: REOPENED → RESOLVED
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 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.
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.