To be reported according to this specification https://mozilla-ichnaea.readthedocs.org/en/latest/#service-1 The wifi reporting requires extra permissions, so I'll attach that as a seperate patch and we can debate it seperately

Could we move this to a background service?

Comment on attachment 743339 [details] [diff] [review] patch for cell tower info collection and reporting >+ URL url = new URL("https://location.services.mozilla.com/v1/submit"); Put the URL in a constant? >+ Log.i("GeckoCell", "posted: " + new String(bytes)); I assume you don't want to land with this

(In reply to Mark Finkle (:mfinkle) from comment #4) > Could we move this to a background service? At a minimum we could pull the code into a separate Java class/file.

Comment on attachment 743339 [details] [diff] [review] patch for cell tower info collection and reporting Review of attachment 743339 [details] [diff] [review]: ----------------------------------------------------------------- Looks ok with some nits ::: mobile/android/base/GeckoApp.java @@ +107,5 @@ > import java.util.regex.Pattern; > > +import android.telephony.TelephonyManager; > +import android.telephony.NeighboringCellInfo; > +import android.telephony.CellLocation; Apparently imports are supposed to be alphabetical, but I don't personally care. @@ +2350,4 @@ > public void onLocationChanged(Location location) { > // No logging here: user-identifying information. > GeckoAppShell.sendEventToGecko(GeckoEvent.createLocationEvent(location)); > + Whitespace @@ +2355,5 @@ > + } > + > + public void setCurrentSignalStrenth(SignalStrength ss) { > + if (ss.isGsm()) > + mSignalStrenth = ss.getGsmSignalStrength(); one too many spaces after = @@ +2374,5 @@ > + cellInfo.put(obj); > + } > + if (cells != null) { > + Iterator<NeighboringCellInfo> i = cells.iterator(); > + while (i.hasNext()) { I like this better: for (NeighboringCellInfo nci : cells) @@ +2413,5 @@ > + break; > + } > + locInfo.put("cell", cellInfo); > + } catch(JSONException jsonex) {} > + Whitespace @@ +2417,5 @@ > + > + ThreadUtils.postToBackgroundThread(new Runnable() { > + public void run() { > + try { > + URL url = new URL("https://location.services.mozilla.com/v1/submit"); Constantize ::: mobile/android/base/GeckoAppShell.java @@ +423,4 @@ > > Looper l = Looper.getMainLooper(); > Location loc = lm.getLastKnownLocation(provider); > + TelephonyManager tm = (TelephonyManager)GeckoApp.mAppContext.getSystemService(Context.TELEPHONY_SERVICE); Can this return null?

Comment on attachment 743339 [details] [diff] [review] patch for cell tower info collection and reporting Are we only submitting data on Wifi? If not, we need to rethink this patch a bit. We can't be sending data over a data connection every time the location changes. We should be storing the data and sending in batches, and preferably only on Wifi.

Also, how does this affect our privacy policy? Do we need opt-in? Can users opt-out?

You seem to be missing the mcc/mnc codes, I believe you can get them via the TelephonyManager.getNetworkOperator API. For proper CDMA support, you probably need to use the CdmaCellLocation API's. Our docs say: "In a CDMA network, the system id (sid) should be sent in the mnc field, the network id (nid) in the lac field and base station id (bid) in the cid field." And the CdmaCellLocation exposes those via getSystemId, getNetworkId and getBaseStationId. We don't yet support other network types, so in case of SIP or a missing telephone connection, please don't sent any cell data or a top-level radio entry at all. For signal strength, we'd like to get the dBm, value, which seems to be exposed via CellSignalStrength.getDbm. For NeighboringCellInfo there's only getRSSI but the API docs suggest a formula to convert the asu level to a dBm measurement (http://developer.android.com/reference/android/telephony/NeighboringCellInfo.html#getRssi%28%29). Please do the value conversion client-side, so we get uniform measurements from all platforms on the server end. If you get UNKNOWN_RSSI, don't sent anything in the "signal" value at all.

(In reply to Mark Finkle (:mfinkle) from comment #8) > Comment on attachment 743339 [details] [diff] [review] > Are we only submitting data on Wifi? If not, we need to rethink this patch a > bit. We can't be sending data over a data connection every time the location > changes. > > We should be storing the data and sending in batches, and preferably only on > Wifi. Those are very good points. Ideally we'd only sent batch data over wifi, or otherwise over the data connection if no wifi link could be established in say 14 days. And certainly not while not on your home network (roaming abroad). We don't have batch API's on the server side yet, as we need to focus on other things first. If you have time, please go ahead and start writing the batch/background sent logic and use the current API to sent them one-by-one. We'll have a batch API soon enough at which point we can change the actual batch sending code and make it more efficient. But that shouldn't stop the rest of the work.

(In reply to Mark Finkle (:mfinkle) from comment #9) > Also, how does this affect our privacy policy? Do we need opt-in? Can users > opt-out? This is tracked in the privacy review at https://bugzilla.mozilla.org/show_bug.cgi?id=862833. There's also a (private) legal bug to look into the contract obligations we have.

Comment on attachment 743344 [details] [diff] [review] patch to collect wifi data Review of attachment 743344 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoApp.java @@ +2420,5 @@ > + > + JSONArray wifiInfo = new JSONArray(); > + List<ScanResult> aps = wm.getScanResults(); > + Iterator<ScanResult> i = aps.iterator(); > + while (i.hasNext()) { Be fancier

(In reply to Mark Finkle (:mfinkle) from comment #8) > Comment on attachment 743339 [details] [diff] [review] > patch for cell tower info collection and reporting > > Are we only submitting data on Wifi? If not, we need to rethink this patch a > bit. We can't be sending data over a data connection every time the location > changes. Maybe it's enough to only collect/report when we are in the foreground? It's one POST with a relatively (I think?) small amount of data. If you are using the browser for other browsing tasks this doesn't seem too awful. If we are backgrounded and the user is in a car or something, I could see a problem.

(In reply to Hanno Schlichting [:hannosch] from comment #11) > > We don't have batch API's on the server side yet, as we need to focus on > other things first. If you have time, please go ahead and start writing the > batch/background sent logic and use the current API to sent them one-by-one. > We'll have a batch API soon enough at which point we can change the actual > batch sending code and make it more efficient. But that shouldn't stop the > rest of the work. Since I have implemented the queue that collects measures to push them as batches in the DB, adding a batch feature on the WS is a no-brainer on the server side. (2 lines) starting this at https://github.com/mozilla/ichnaea/issues/5

We changed the existing submit API to take batches (possibly of size 1) instead. In addition we introduced a new timestamp field for each measurement. API docs have been updated at https://mozilla-ichnaea.readthedocs.org/en/latest/#service-1 - batch means there's an outer {"items": [<measure>]} wrapper - timestamp means you can sent a "time": "2012-03-15T11:12:13.456Z" as a new key for each measurement inside the batch

Give me 3 days to prepare a blog post and coordinate a bit internally. After that I will a+ for trunk. We should talk to product to see when this can ride the train. For trunk we can decide that on our own.

(In reply to Hanno Schlichting [:hannosch] from comment #10) > You seem to be missing the mcc/mnc codes, I believe you can get them via the > TelephonyManager.getNetworkOperator API. mcc and mnc identify the network of the cell you're registered on. That doesn't necessarily apply to other cell towers in the area. > For proper CDMA support, you probably need to use the CdmaCellLocation > API's. Our docs say: "In a CDMA network, the system id (sid) should be sent > in the mnc field, the network id (nid) in the lac field and base station id > (bid) in the cid field." > > And the CdmaCellLocation exposes those via getSystemId, getNetworkId and > getBaseStationId. I was told by dougt to ignore CDMA. If that is no longer true, let me know

still need a product decision around the wifi data collection, specifically the added permissions needed.

Comment on attachment 747153 [details] [diff] [review] patch for cell tower info collection and reporting Not a proper review yet, just some initial comments: We want mcc/mnc in two separate fields, so "obj.put("mcc+mnc", tm.getNetworkOperator());" isn't correct. I've seen stackoverflow examples and core android code which simply took the first three characters of the combined value and reported it as mcc and the rest as mnc. You can remove reporting "psc/getPsc" as we don't deal with UMTS networks and the server API doesn't specify "psc" as a valid key (it's currently ignoring any extra keys you throw at it). For NeighboringCellInfo, could you add the mcc/mnc values from tm.getNetworkOperator() to the info? That should be correct, since you don't see cell towers from other operators. The docs suggest this info is for the currently registered operator and not the home/SIM operator. So this should be correct even while roaming in a different country.

(In reply to Hanno Schlichting [:hannosch] from comment #21) > Comment on attachment 747153 [details] [diff] [review] > patch for cell tower info collection and reporting > > Not a proper review yet, just some initial comments: > > We want mcc/mnc in two separate fields, so "obj.put("mcc+mnc", > tm.getNetworkOperator());" isn't correct. I've seen stackoverflow examples > and core android code which simply took the first three characters of the > combined value and reported it as mcc and the rest as mnc. Why do we want mcc/mnc as separate fields? We can separate them on the client, which should work most of the time, but I'd assume we would just wind up combining those values on the server side. > > You can remove reporting "psc/getPsc" as we don't deal with UMTS networks > and the server API doesn't specify "psc" as a valid key (it's currently > ignoring any extra keys you throw at it). I am suggesting that you start tracking psc. For example, I don't get a value for cid or lac in my testing because all of the devices I have tested are on UMTS networks. > For NeighboringCellInfo, could you add the mcc/mnc values from > tm.getNetworkOperator() to the info? That should be correct, since you don't > see cell towers from other operators. Why don't you see towers from other operators? > The docs suggest this info is for the > currently registered operator and not the home/SIM operator. So this should > be correct even while roaming in a different country. Yes, which is why it is reported for the registered cell. I don't know that we can assume that it is valid for neighboring cells.

Speaking to Doug on the topic, I am fairly comfortable with this change and added permission *provided* it is well communicated, SUMO is prepared in advance and marketing / PR is briefed (which I will certainly help with). It is my understanding that this will 'improve geolocation services', and this phrase could be used to state 'why' we are asking for this permission. What I didn't ask Doug, was whether a user could opt out of that permission... 1) on installation (or update) and/or 2) via the settings menu Thanks, Karen

I'd hope that anything sending data on the user's whereabouts other other privacy-relevant information *aynwhere*, we have at least an easy opt-out. It might even be better to have those things being opt-in but I guess that depends on what data is sent where for what purpose.

@kairo this isn't the right bug to talk about this. Please see comment #12.

Brad and me talked about these points on IRC, capturing here for everyone to see. (In reply to Brad Lassey [:blassey] from comment #22) > Why do we want mcc/mnc as separate fields? The Firefox OS client gets them as separate fields, and on the server side we can use the country code to shard data. So it's useful to get them in separate fields. > > You can remove reporting "psc/getPsc" as we don't deal with UMTS networks > > and the server API doesn't specify "psc" as a valid key (it's currently > > ignoring any extra keys you throw at it). > I am suggesting that you start tracking psc. For example, I don't get a > value for cid or lac in my testing because all of the devices I have tested > are on UMTS networks. I've looked into UMTS networks. From what I understand there's a couple of thousand different psc values in use in the world. These aren't useful to uniquely identify any cell / tower, but are just scrambling codes used to encode the signals. The UMTS cell id is in itself unique to identify a cell or rather a radio network controller + base station + cell combination in a carrier network. So if we don't get a cell id, we cannot report back any useful information. I found various stackoverflow and forum posts which suggest that many radio chipsets provide only partial information to Android. > > For NeighboringCellInfo, could you add the mcc/mnc values from > > tm.getNetworkOperator() to the info? That should be correct, since you don't > > see cell towers from other operators. > Why don't you see towers from other operators? We are at an impasse here. The API docs aren't clear on what to expect from the neighboring cell info API. Without knowing the actual mcc/mnc codes, we don't have any useful information to report. There's a new API in API level 17 called getAllCellInfo which has much more accurate data. We should use that API and make sure we request the "ACCESS_COARSE_UPDATES" permission for our app. Since this API is only available in the latest Android version (~3% market share), we should still use the older API to get at least the location of the serving cell.

Comment on attachment 747153 [details] [diff] [review] patch for cell tower info collection and reporting Current state of review discussion is in comment 26. Other comments: There's code for doing a rssi to dbm calculation, but this isn't actually used in the json value (obj.put("signal", nci.getRssi()); vs. obj.put("signal", dbm);) We changed the server API, so we'd like to get a time value for each measurement in ISO 8601 / UTC. More detail at https://mozilla-ichnaea.readthedocs.org/en/latest/#service-1-POST The Android location.getAccuracy / getAltitude API's are reporting back float/double types. The server API expects integers.

this patch makes the assumption that the mnc and mcc of the registered cell is the same for all neighboring cells that we talked about.

Comment on attachment 752250 [details] [diff] [review] patch for cell tower info collection and reporting This looks pretty good. You still include the "psc" value in the JSON. Could you reply to my findings in comment 26 about that?

(In reply to Hanno Schlichting [:hannosch] from comment #29) > Comment on attachment 752250 [details] [diff] [review] > patch for cell tower info collection and reporting > > This looks pretty good. You still include the "psc" value in the JSON. Could > you reply to my findings in comment 26 about that? That's still the only info we have for UMTS towers. Even if the psc isn't unique per cell, the mix of psc's in a location should be a unique finger print for the location. I'd suggest collecting the information.

Ok. I updated the API to include PSC and in general expand it to potentially cover all the extra information we'd get from the new Android API. New detailed docs at https://mozilla-ichnaea.readthedocs.org/en/latest/cell.html#cell-records Two changes I'd like you to do: 1. Report back the data from the NeighboringCellInfo.getRssi() in the new "asu" field and omit "signal". This means you can avoid the dBm calculations. 2. Report back a more detailed "radio" type. One of: "gsm", "umts", "cdma" or "lte". I think the cell-records page has a mapping for all the different Android network types (so GPRS, HSPA, etc. should be "gsm", EVDO and eHRPD should be "cdma") If we only care about GSM/UMTS for now, you only have to extend the radio check to include the "umts" case. We want this information, as the signal strength / asu ratings mean very different things for each network type. So we need those to make better estimates on how they relate to distance. It would be nice if we could get a second patch for using the new more detailed Android API's, if available on the phone. But that could be a separate bugzilla ticket.

One update on the more detailed new API: It would be good if we could at least make sure we ask for the right permissions in the app manifest to use these APIs (if they are different). We can always ship the code to actually use them later, but we do have a very strict window for the permission change. The same goes for the wifi side - where we need to make sure we ask for the permission, but code can come later.

(In reply to Hanno Schlichting [:hannosch] from comment #32) > One update on the more detailed new API: It would be good if we could at > least make sure we ask for the right permissions in the app manifest to use > these APIs (if they are different). > > We can always ship the code to actually use them later, but we do have a > very strict window for the permission change. The same goes for the wifi > side - where we need to make sure we ask for the permission, but code can > come later. We decided at some point (I'm sure it is documented in some bug somewhere) not to ship with permission requests that we don't use, even if we plan to use them in the near future. So let's get what we need in so it can all be bundled up in 23.

I assume you want this value still as "signal": http://developer.android.com/reference/android/telephony/SignalStrength.html#getGsmSignalStrength%28%29 Side note, if you want my attention either r-/+ the patch or use need info.

Comment on attachment 755779 [details] [diff] [review] patch ># HG changeset patch ># User Brad Lassey <blassey@mozilla.com> ># Date 1369890107 14400 ># Node ID 9202effe8816c49750c9c5d636c6b51476fdc4a8 ># Parent d5650e8612831c5ab9996651c49d546f72d26242 >[mq]: track_cells > >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java >--- a/mobile/android/base/GeckoApp.java >+++ b/mobile/android/base/GeckoApp.java >@@ -55,6 +55,15 @@ import android.os.Bundle; > import android.os.Handler; > import android.os.PowerManager; > import android.os.StrictMode; >+ >+import android.telephony.CellLocation; >+import android.telephony.NeighboringCellInfo; >+import android.telephony.SignalStrength; >+import android.telephony.TelephonyManager; >+import android.telephony.PhoneStateListener; >+import android.telephony.SignalStrength; >+import android.telephony.gsm.GsmCellLocation; >+ > import android.text.TextUtils; > import android.util.AttributeSet; > import android.util.Base64; >@@ -84,14 +93,20 @@ import android.widget.TextView; > import android.widget.Toast; > > import java.io.BufferedReader; >+import java.io.BufferedOutputStream; > import java.io.ByteArrayOutputStream; > import java.io.File; > import java.io.FileReader; >+import java.io.InputStreamReader; > import java.io.IOException; > import java.io.InputStream; >+import java.io.OutputStream; > import java.net.HttpURLConnection; > import java.net.URL; >+import java.text.DateFormat; >+import java.text.SimpleDateFormat; > import java.util.ArrayList; >+import java.util.Date; > import java.util.HashMap; > import java.util.Iterator; > import java.util.List; >@@ -133,6 +148,8 @@ abstract public class GeckoApp > static public final int RESTORE_OOM = 1; > static public final int RESTORE_CRASH = 2; > >+ static private final String LOCATION_URL = "https://location.services.mozilla.com/v1/submit"; >+ > protected RelativeLayout mMainLayout; > protected RelativeLayout mGeckoLayout; > public View getView() { return mGeckoLayout; } >@@ -171,6 +188,9 @@ abstract public class GeckoApp > > private volatile BrowserHealthRecorder mHealthRecorder = null; > >+ private int mSignalStrenth; >+ private PhoneStateListener mPhoneStateListener = null; >+ > abstract public int getLayout(); > abstract public boolean hasTabsSideBar(); > abstract protected String getDefaultProfileName(); >@@ -202,6 +222,15 @@ abstract public class GeckoApp > } > > public LocationListener getLocationListener() { >+ if (mPhoneStateListener == null) { >+ mPhoneStateListener = new PhoneStateListener() { >+ public void onSignalStrengthsChanged(SignalStrength signalStrength) { >+ setCurrentSignalStrenth(signalStrength); >+ } >+ }; >+ TelephonyManager tm = (TelephonyManager)getSystemService(Context.TELEPHONY_SERVICE); >+ tm.listen(mPhoneStateListener, PhoneStateListener.LISTEN_SIGNAL_STRENGTHS); >+ } > return this; > } > >@@ -2151,6 +2180,129 @@ abstract public class GeckoApp > public void onLocationChanged(Location location) { > // No logging here: user-identifying information. > GeckoAppShell.sendEventToGecko(GeckoEvent.createLocationEvent(location)); >+ collectAndReportLocInfo(location); >+ } >+ >+ public void setCurrentSignalStrenth(SignalStrength ss) { >+ if (ss.isGsm()) >+ mSignalStrenth = ss.getGsmSignalStrength(); >+ } >+ >+ private int getCellInfo(JSONArray cellInfo) { >+ TelephonyManager tm = (TelephonyManager)getSystemService(Context.TELEPHONY_SERVICE); >+ if (tm == null) >+ return TelephonyManager.PHONE_TYPE_NONE; >+ List<NeighboringCellInfo> cells = tm.getNeighboringCellInfo(); >+ CellLocation cl = tm.getCellLocation(); >+ String mcc = "", mnc = ""; >+ if (cl instanceof GsmCellLocation) { >+ JSONObject obj = new JSONObject(); >+ GsmCellLocation gcl = (GsmCellLocation)cl; >+ try { >+ obj.put("lac", gcl.getLac()); >+ obj.put("cid", gcl.getCid()); >+ obj.put("psc", gcl.getPsc()); >+ switch(tm.getNetworkType()) { >+ case TelephonyManager.NETWORK_TYPE_GPRS: >+ case TelephonyManager.NETWORK_TYPE_EDGE: >+ obj.put("radio", "gsm"); >+ break; >+ case TelephonyManager.NETWORK_TYPE_UMTS: >+ case TelephonyManager.NETWORK_TYPE_HSDPA: >+ case TelephonyManager.NETWORK_TYPE_HSUPA: >+ case TelephonyManager.NETWORK_TYPE_HSPA: >+ case TelephonyManager.NETWORK_TYPE_HSPAP: >+ obj.put("radio", "umts"); >+ break; >+ } >+ String mcc_mnc = tm.getNetworkOperator(); >+ mcc = mcc_mnc.substring(0, 3); >+ mnc = mcc_mnc.substring(3); >+ obj.put("mcc", mcc); >+ obj.put("mnc", mnc); >+ obj.put("signal", mSignalStrenth); This should use the "asu" key. The value comes from the getGsmSignalStrength method, which returns values in ASU format (0-31 for GSM). The "signal" key should be used if you'd get raw dBm measurements, so something like -51 to -113 for GSM. >+ } catch(JSONException jsonex) {} >+ cellInfo.put(obj); >+ } >+ if (cells != null) { >+ for (NeighboringCellInfo nci : cells) { >+ try { >+ JSONObject obj = new JSONObject(); >+ obj.put("lac", nci.getLac()); >+ obj.put("cid", nci.getCid()); >+ obj.put("psc", nci.getPsc()); >+ obj.put("mcc", mcc); >+ obj.put("mnc", mnc); >+ >+ int dbm; >+ switch(nci.getNetworkType()) { >+ case TelephonyManager.NETWORK_TYPE_GPRS: >+ case TelephonyManager.NETWORK_TYPE_EDGE: >+ obj.put("radio", "gsm"); >+ break; >+ case TelephonyManager.NETWORK_TYPE_UMTS: >+ case TelephonyManager.NETWORK_TYPE_HSDPA: >+ case TelephonyManager.NETWORK_TYPE_HSUPA: >+ case TelephonyManager.NETWORK_TYPE_HSPA: >+ case TelephonyManager.NETWORK_TYPE_HSPAP: >+ obj.put("radio", "umts"); >+ break; >+ } >+ >+ obj.put("asu", nci.getRssi()); >+ cellInfo.put(obj); >+ } catch(JSONException jsonex) {} >+ } >+ } >+ return tm.getPhoneType(); >+ } >+ >+ private void collectAndReportLocInfo(Location location) { >+ final JSONObject locInfo = new JSONObject(); >+ try { >+ JSONArray cellInfo = new JSONArray(); >+ int radioType = getCellInfo(cellInfo); >+ if (radioType == TelephonyManager.PHONE_TYPE_GSM) >+ locInfo.put("radio", "gsm"); >+ else >+ return; // we don't care about other radio types for now >+ locInfo.put("lon", location.getLongitude()); >+ locInfo.put("lat", location.getLatitude()); >+ locInfo.put("accuracy", (int)location.getAccuracy()); >+ locInfo.put("altitude", (int)location.getAltitude()); >+ DateFormat df = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm'Z'"); >+ locInfo.put("time", df.format(new Date(location.getTime()))); >+ locInfo.put("cell", cellInfo); >+ } catch(JSONException jsonex) {} >+ >+ ThreadUtils.postToBackgroundThread(new Runnable() { >+ public void run() { >+ try { >+ URL url = new URL(LOCATION_URL); >+ HttpURLConnection urlConnection = (HttpURLConnection) url.openConnection(); >+ try { >+ urlConnection.setDoOutput(true); >+ JSONArray batch = new JSONArray(); >+ batch.put(locInfo); >+ JSONObject wrapper = new JSONObject(); >+ wrapper.put("items", batch); >+ byte[] bytes = wrapper.toString().getBytes(); >+ urlConnection.setFixedLengthStreamingMode(bytes.length); >+ OutputStream out = new BufferedOutputStream(urlConnection.getOutputStream()); >+ out.write(bytes); >+ out.flush(); >+ } catch (JSONException jsonex) { >+ Log.e("GeckoCell", "error wrapping data as a batch", jsonex); >+ } catch (IOException ioex) { >+ Log.e("GeckoCell", "error submitting data", ioex); >+ } finally { >+ urlConnection.disconnect(); >+ } >+ } catch (IOException ioex) { >+ Log.e("GeckoCell", "error submitting data", ioex); >+ } >+ } >+ }); > } > > @Override

(In reply to Brad Lassey [:blassey] from comment #33) > (In reply to Hanno Schlichting [:hannosch] from comment #32) > > One update on the more detailed new API: It would be good if we could at > > least make sure we ask for the right permissions in the app manifest to use > > these APIs (if they are different). > > > > We can always ship the code to actually use them later, but we do have a > > very strict window for the permission change. The same goes for the wifi > > side - where we need to make sure we ask for the permission, but code can > > come later. > > We decided at some point (I'm sure it is documented in some bug somewhere) > not to ship with permission requests that we don't use, even if we plan to > use them in the near future. So let's get what we need in so it can all be > bundled up in 23. Ok. Do you have time to extend the patch to include a separate code path for using the newer Android API's? And extending them to cover CDMA/LTE networks? Preferably I'd like to get as much detail as possible from the client. The server side is able to handle it all now. For actual priorities, ask Doug :) The same goes for gathering wifi access points. But maybe wifi handling should be a separate bug?

CDMA/LTE doesn't require extra permissions. The additional APIs in 4.2.2 may (though I doubt it), I can look at that at least. WiFi is already done, the patch is on this bug and is r+'d. The only remaining loose end is the user visible pref that Karen asked for in comment 23. We should get a bug on file for that.

Comment on attachment 743344 [details] [diff] [review] patch to collect wifi data We changed the API and added some new constraints for wifi gathering. First is to filter out all wifi's where the ssid ends with _nomap and to ignore all wifi's in ad-hoc mode. The code for this filtering could be adapted from http://code.google.com/p/sensor-data-collection-library/source/browse/src/main/java/TextFileSensorLog.java#223 Second is to report back a "key" instead of the bssid/mac address. The key should be calculated as combination of the bssid and ssid. I updated the docs at https://mozilla-ichnaea.readthedocs.org/en/latest/index.html, but the relevant section is: The key is a SHA1 hash of the concatenated BSSID and SSID of the wifi network. So for example for a bssid of 01:23:45:67:89:ab and a ssid of network name, the result should be: 3680873e9b83738eb72946d19e971e023e51fd01. In Python this would be coded as: import hashlib bssid = '01:23:45:67:89:ab'.encode('utf-8') ssid = 'network name'.encode('utf-8') key = hashlib.sha1(bssid + ssid).hexdigest() I hope this is exact enough to replicate it in Java.

This puts everything a default-off pref until we can sort out the UX issues

Comment on attachment 756787 [details] [diff] [review] patch for pref I don't see "app.geo.reportdata" set in mobile.js so the PrefsHelper will never get called. This is fine since you default to | false | for mShouldReportGeoData. Just an FYI. Do you really want "app.geo.reportdata" to be an integer preference? It seems like it should be a boolean, in which case | value | would be a boolean too. Unless you really want an integer, please change to a boolean. --- I am a little worried about piling up PrefHelpers in the onCreate code. We already have one right above your new one. Could you just reuse the existing PrefHelper to get both preferences? Check the "pref" passed in to switch between the updater and the geodata code. Using two PrefHelpers means two separate calls to Gecko. Using one means both prefs are handled in a single call to Gecko. To get more than one pref use a String array and call PrefHelper.getPrefs(array, handler) Up to you, but we should probably handle in a follow up if you don't do it now.

(In reply to Mark Finkle (:mfinkle) from comment #41) > Comment on attachment 756787 [details] [diff] [review] > patch for pref > > I don't see "app.geo.reportdata" set in mobile.js so the PrefsHelper will > never get called. This is fine since you default to | false | for > mShouldReportGeoData. Just an FYI. > > Do you really want "app.geo.reportdata" to be an integer preference? It > seems like it should be a boolean, in which case | value | would be a > boolean too. > > Unless you really want an integer, please change to a boolean. I went with an integer so that we could have 1 for enabled, 0 for disabled and -1 for unset, in which case we can prompt the user for a choice. > > --- > > I am a little worried about piling up PrefHelpers in the onCreate code. We > already have one right above your new one. > > Could you just reuse the existing PrefHelper to get both preferences? Check > the "pref" passed in to switch between the updater and the geodata code. > > Using two PrefHelpers means two separate calls to Gecko. Using one means > both prefs are handled in a single call to Gecko. > > To get more than one pref use a String array and call > PrefHelper.getPrefs(array, handler) > > Up to you, but we should probably handle in a follow up if you don't do it > now. I don't think it makes much of a difference to have one event versus two. I prefer them as separate calls because the logic is cleaner for the handlers.

Finkle, I want to confirm with you that you're OK with the int pref since changing pref types is fragile

(In reply to Brad Lassey [:blassey] from comment #44) > Finkle, I want to confirm with you that you're OK with the int pref since > changing pref types is fragile I am fine with the INT pref, but I do want a followup to combine the pref getter.

Comment on attachment 757681 [details] [diff] [review] patch to collect wifi data Looks great!

Comment on attachment 757681 [details] [diff] [review] patch to collect wifi data Requesting approval for Aurora on these 3 patches because we want to batch the new permission requests in 23 to minimize the number of users who fall off of automatic updates. Risk is low because for 23 we're keeping this behind an about:config pref that is defaulted to off

Comment on attachment 757681 [details] [diff] [review] patch to collect wifi data in our mobile roadmap meeting we decided it would be best to just leave this on 24 (and move all our other new permission requests to 24 as well).

https://hg.mozilla.org/mozilla-central/rev/92178edfeb6e https://hg.mozilla.org/mozilla-central/rev/68766cdda810 https://hg.mozilla.org/mozilla-central/rev/ee864722aff9

When opt-in (bug 877725), how does one monitor or view logging of the JSON collected cell data just to confirm it's working as intended?

Disabled for 24.0 Beta in bug 902783.

Clearing the relnote nom as this is disabled in Fx24.Please renom when this rides up and is ready.