If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Use hardware model instead of oscpu to identify clients

RESOLVED FIXED

Status

Cloud Services
Firefox Sync: Backend
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Mardak, Assigned: mbrubeck)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0+)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

7 years ago
Bug 562878 switched the "machine name" to use "oscpu", which shows up as "Mac OS 10.6" but also "Linux armv7l" for Nexus One. I was aiming to get "Android", and seems like that's available from "platform", but that shows up as "MacIntel".

Ideally we can show things like "Nexus One" or "MacBook Pro", etc (not sure what Windows equivalent would be.)
(Assignee)

Comment 1

7 years ago
Hardware device names can be accessed through Java on Android; I don't know if other platforms have equivalent APIs.
http://developer.android.com/reference/android/os/Build.html#MODEL
(Reporter)

Comment 2

7 years ago
For OS X, I found this code..

http://www.cocoadev.com/index.pl?MacintoshModels

But that seems to need an explicit list of internal model id to some string.

But maybe we just want "machine_name" instead of "machine_model":

~$ system_profiler -xml SPHardwareDataType | grep -A1 machine_
				<key>machine_model</key>
				<string>MacBook5,1</string>
				<key>machine_name</key>
				<string>MacBook</string>
(Assignee)

Comment 3

7 years ago
Created attachment 459913 [details] [diff] [review]
Android proof-of-concept

This is a minimal prototype; not suitable for checkin.  I'm experimenting with using js-ctypes to call Android Java code through our C++ bridge without XPCOM.

If we actually want to use this approach, we would need to:

- Make the C++ bridge code as generic as possible, to avoid writing new code for every Java function we call.

- Create some helper objects to reduce boilerplate in the JavaScript code.

- Move the C bridge functions out of libxul.so into a separate shared library, to reduce startup perf impact of exported symbols.

Also, there is a bug in this code - sometimes the string returned from GetDeviceModel has garbage characters appended, as though GetStringUTFLength is returning the wrong result.  I have no idea yet why this happens.
(Assignee)

Comment 4

7 years ago
The System Info service (@mozilla.org/system-info;1) already has code to do this on Maemo, and it looks like the right place to add the code for Android too:
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsSystemInfo.cpp
(Assignee)

Comment 5

7 years ago
Created attachment 473074 [details] [diff] [review]
part 1: Use nsSystemInfo to get device name

The "device" property is currently implemented on Maemo only.
Assignee: nobody → mbrubeck
Attachment #459913 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #473074 - Flags: review?(edilee)
(Assignee)

Comment 6

7 years ago
Created attachment 473076 [details] [diff] [review]
part 2: implement "device" property for Android
Attachment #473076 - Flags: review?(blassey.bugs)
(Assignee)

Updated

7 years ago
Component: Firefox Sync: Backend → General
Product: Mozilla Services → Fennec
QA Contact: sync-backend → general
Version: unspecified → Trunk
(Assignee)

Comment 7

7 years ago
Moving to Fennec:General so I can set the blocking-fennec flag.
tracking-fennec: --- → ?
(Reporter)

Comment 8

7 years ago
Comment on attachment 473074 [details] [diff] [review]
part 1: Use nsSystemInfo to get device name

>+++ b/services/sync/modules/engines/clients.js	Wed Sep 08 08:58:35 2010 -0700
>+    let system = Cc["@mozilla.org/system-info;1"]
>+                   .getService(Ci.nsIPropertyBag2).get("device") ||
You can just use Svc.SysInfo.get("device") [it was originally added for .get("host") when we used hostnames as part of the client id.]
Attachment #473074 - Flags: review?(edilee)
Attachment #473074 - Flags: review+
Attachment #473074 - Flags: feedback?(philipp)
Comment on attachment 473074 [details] [diff] [review]
part 1: Use nsSystemInfo to get device name

What Mardak says :). Looks good otherwise. Please note that this should land in fx-sync to avoid diverging codebases. (We periodically merge fx-sync to m-c).
Attachment #473074 - Flags: feedback?(philipp) → feedback+
(Assignee)

Comment 10

7 years ago
Part 1 pushed to fx-sync: http://hg.mozilla.org/services/fx-sync/rev/7a0e2d0b4366
Comment on attachment 473076 [details] [diff] [review]
part 2: implement "device" property for Android

>+    nsString str;
>+    int len = env->GetStringLength(s);
>+    str.SetLength(len);
>+    env->GetStringRegion(s, 0, len, str.BeginWriting());

why not just use nsJNIString?

>-#endif   
>+#endif
>+
>+#ifdef ANDROID
>+    nsString str = mozilla::AndroidBridge::GetStaticField("android/os/Build", "MODEL");
>+    SetPropertyAsAString(NS_ConvertASCIItoUTF16("device"), str);
>+#endif

This can only be called from the chrome process, please either add a check for that or if your sure that shouldn't ever happen an assertion.
Attachment #473076 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 12

7 years ago
Created attachment 473221 [details] [diff] [review]
part 2 (v2): implement "device" property for Android

(In reply to comment #11
> why not just use nsJNIString?

Oh, handy.  Done.

> This can only be called from the chrome process, please either add a check for
> that or if your sure that shouldn't ever happen an assertion.

Added a check.  Also did some minor cleanup.  Re-requesting review for the changes.
Attachment #473076 - Attachment is obsolete: true
Attachment #473221 - Flags: review?(blassey.bugs)
Comment on attachment 473221 [details] [diff] [review]
part 2 (v2): implement "device" property for Android

>+nsString AndroidBridge::GetStaticField(const char *className, const char *fieldName)

this should be returned as an out param. Sorry I didn't catch that the first time.
Attachment #473221 - Flags: review?(blassey.bugs) → review-
(Assignee)

Comment 14

7 years ago
Created attachment 473544 [details] [diff] [review]
part 2 (v3): implement "device" property for Android

Use an outparam, and add error checking.
Attachment #473221 - Attachment is obsolete: true
Attachment #473544 - Flags: review?(blassey.bugs)
Assignee: mbrubeck → nobody
tracking-fennec: ? → 2.0+
Component: General → Firefox Sync: UI
Product: Fennec → Mozilla Services
QA Contact: general → sync-ui
Version: Trunk → unspecified
Component: Firefox Sync: UI → Firefox Sync: Backend
QA Contact: sync-ui → sync-backend

Comment 15

7 years ago
Comment on attachment 473544 [details] [diff] [review]
part 2 (v3): implement "device" property for Android

>+PRBool
>+AndroidBridge::GetStaticField(const char *className, const char *fieldName, nsAString &result)
>+{
>+    jclass cls = (jclass) mJNIEnv->NewGlobalRef(mJNIEnv->FindClass(className));
>+    if (!cls)
>+        return PR_FALSE;
>+
>+    jfieldID field = mJNIEnv->GetStaticFieldID(cls, fieldName, "Ljava/lang/String;");
>+    if (!field)
>+        return PR_FALSE;
>+
>+    jstring jstr = (jstring) mJNIEnv->GetStaticObjectField(cls, field);
>+    if (!jstr)
>+        return PR_FALSE;
>+
>+    result.Assign(nsJNIString(jstr));
>+    return PR_TRUE;
>+}
>+
You need an AutoLocalJNIFrame here instead of grabbing a global reference. (the code in the bridge initialization is called from Java so there's already a local frame)

Also, I suggest naming this GetStaticStringField and use bool for the return value.

>@@ -118,17 +122,25 @@ nsSystemInfo::Init()
>             break;
>           }
>         }
>       }
>       if (line)
>         free(line);
>       fclose(fp);
>     }
>-#endif   
>+#endif
>+
>+#ifdef ANDROID
>+    if (mozilla::AndroidBridge::Bridge()) {
>+        nsString str;
nsAutoString, unless you believe your string will be over 63 characters long most of the time.

>+        if (mozilla::AndroidBridge::Bridge()->GetStaticField("android/os/Build", "MODEL", str))
>+            SetPropertyAsAString(NS_ConvertASCIItoUTF16("device"), str);
NS_LITERAL_STRING
Attachment #473544 - Flags: review?(blassey.bugs) → review-
Blocks: 594506
(Assignee)

Comment 16

7 years ago
Created attachment 474312 [details] [diff] [review]
part 2 (v4): implement "device" property for Android

Addresses requests from comment 15.
Assignee: nobody → mbrubeck
Attachment #473544 - Attachment is obsolete: true
Attachment #474312 - Flags: review?(mwu)

Updated

7 years ago
Attachment #474312 - Flags: review?(mwu) → review+
(Assignee)

Comment 17

7 years ago
Android patch pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/753ddb918cd5

I guess this issue can be marked resolved when fx-sync is merged to m-c.  We can enter followup bugs if we want to do similar things on other platforms (Mac, Windows...).
Actually we resolve once we land in fx-sync. I marked this blocking the next merge (bug 594506).
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.