837987 - [GPS] Position Fix Fail using CellID. Request Implementation of CellID Position Method.

Status: RESOLVED WORKSFORME

(Firefox OS Graveyard :: General, defect, P3)

Description

[leo.bugzilla.gecko]

Dear Mozilla Team,

Navigation Test Environment 
 - GPS setting 'OFF(disabled)'
 - Data Enabled
 - WiFi On

Is FFOS available CellId Position Method?
When Navigation App test start, result is not position fix.
If it were not use CellID, it must be major defect as compared with Android OS or iOS. 
We think CellId Method must-have function in FFOS.
Have you plan to implement CellId?

Thanks

Comment 1

[leo.bugzilla.gecko]

We would appreciate your prompt reply.

Thanks

Comment 2

[Kai-Chih Hu [:khu]]

Hi, Peter, it looks like a feature request. 
Did we put this into the product plan already? 
Thanks.

Comment 3

[Peter Dolanjski [:pdol]]

We are planning additional AGPS functionality for future releases of FFOS.  

Kan-Ru, might this be in your domain to comment on?

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

That will need a CellID database, presumably from a partner.

I know there are some freely available databases on the internet like  http://www.opencellid.org/ so we could implement this feature based on it first.

Comment 5

[leo.bugzilla.gecko]

Is http://www.opencellid.org/ cover Area of America and Europe?

Comment 6

[Beatriz Rodríguez [:brg]]

Telefonica Spain server has this functionality, you can use it.

Comment 7

[leo.bugzilla.gecko]

It is a goo thing that Telefonica server supported.

Is Cellid implemented by code? 
If Cellid is not coding, 
Could you tell us plan Cellid implementation?

Thanks

Comment 8

[Doug Turner (:dougt)]

Telefonica also requires client auth to connect, right?

Comment 9

[Doug Turner (:dougt)]

ignore me.

Comment 10

[Doug Turner (:dougt)]

openssl s_client -host  agps.movistar.es -port 7275 -showcerts



Server certificate
subject=/C=ES/ST=MADRID/L=MADRID/O=TELEFONICA MOVILES ESPANA SA/OU=SEGURIDAD DE REDES Y SERVICIOS/CN=agps.movistar.es
issuer=/C=US/O=VeriSign, Inc./OU=VeriSign Trust Network/OU=Terms of use at https://www.verisign.com/rpa (c)10/CN=VeriSign Class 3 International Server CA - G3
---
No client certificate CA names sent
---
SSL handshake has read 4276 bytes and written 316 bytes
---
New, TLSv1/SSLv3, Cipher is RC4-SHA
Server public key is 1024 bit
Secure Renegotiation IS NOT supported

Comment 11

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: WIP patch

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: WIP patch

forget qref..

Comment 13

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: CellId positioning using opencellid.org api

Comment 14

[Doug Turner (:dougt)]

Comment on attachment 735009 [details] [diff] [review]
CellId positioning using opencellid.org api

Review of attachment 735009 [details] [diff] [review]:
-----------------------------------------------------------------

the idea behind this is great.

jdm has time to review.

Comment 15

[Josh Matthews [:jdm]]

Comment on attachment 735009 [details] [diff] [review]
CellId positioning using opencellid.org api

Review of attachment 735009 [details] [diff] [review]:
-----------------------------------------------------------------

This looks ok; I just want to make sure my questions are considered.

::: dom/system/CellGeolocationProvider.js
@@ +68,5 @@
> +      return;
> +    this.started = true;
> +    this.ril = Cc["@mozilla.org/ril;1"].getService(Ci.nsIRadioInterfaceLayer);
> +    this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +    this.timer.initWithCallback(this, 1000, this.timer.TYPE_REPEATING_SLACK);

Do we really want a timer firing every second?

@@ +121,5 @@
> +        "lac " + lac + " " +
> +        "cellid " + cellid);
> +
> +    this.setCache(mcc, mnc, lac, cellid);
> +    this.api.getCurrentPosition(mcc, mnc, cellid, lac, this);

Is it really necessary to have another component for this? Do we anticipate multiple implementations here?

@@ +130,5 @@
> +    this.updateLocation(newLocation);
> +  },
> +
> +  /* private */
> +  updateLocation: function updateLocation(location) {

_updateLocation

@@ +143,5 @@
> +    }
> +  },
> +
> +  /* private */
> +  valueCached: function valueCached(mcc, mnc, lac, cellid) {

_valueIsCached

@@ +151,5 @@
> +            this.lastCellId == cellid);
> +  },
> +
> +  /* private */
> +  setCache: function setCache(mcc, mnc, lac, cellid) {

_cacheValue

Comment 16

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Josh Matthews [:jdm] from comment #15)
> Comment on attachment 735009 [details] [diff] [review]
> CellId positioning using opencellid.org api
> 
> Review of attachment 735009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks ok; I just want to make sure my questions are considered.
> 
> ::: dom/system/CellGeolocationProvider.js
> @@ +68,5 @@
> > +      return;
> > +    this.started = true;
> > +    this.ril = Cc["@mozilla.org/ril;1"].getService(Ci.nsIRadioInterfaceLayer);
> > +    this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> > +    this.timer.initWithCallback(this, 1000, this.timer.TYPE_REPEATING_SLACK);
> 
> Do we really want a timer firing every second?

When we reconnect to a different cell tower, the RIL may or may not issue a call state change event; I haven't checked that. If we could get the event then this timer isn't necessary.

> @@ +121,5 @@
> > +        "lac " + lac + " " +
> > +        "cellid " + cellid);
> > +
> > +    this.setCache(mcc, mnc, lac, cellid);
> > +    this.api.getCurrentPosition(mcc, mnc, cellid, lac, this);
> 
> Is it really necessary to have another component for this? Do we anticipate
> multiple implementations here?

To enable the vendors to plugin different providers' support I think this is necessary.

Comment 17

[Josh Matthews [:jdm]]

(In reply to Kan-Ru Chen [:kanru] from comment #16)
> (In reply to Josh Matthews [:jdm] from comment #15)
> > Comment on attachment 735009 [details] [diff] [review]
> > CellId positioning using opencellid.org api
> > 
> > Review of attachment 735009 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks ok; I just want to make sure my questions are considered.
> > 
> > ::: dom/system/CellGeolocationProvider.js
> > @@ +68,5 @@
> > > +      return;
> > > +    this.started = true;
> > > +    this.ril = Cc["@mozilla.org/ril;1"].getService(Ci.nsIRadioInterfaceLayer);
> > > +    this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> > > +    this.timer.initWithCallback(this, 1000, this.timer.TYPE_REPEATING_SLACK);
> > 
> > Do we really want a timer firing every second?
> 
> When we reconnect to a different cell tower, the RIL may or may not issue a
> call state change event; I haven't checked that. If we could get the event
> then this timer isn't necessary.

Once per second seems high to me, but I'll defer to your judgement.

Comment 18

[Dylan Oliver [:doliver]]

This issue was identified as a Leo QE1 blocker. Nominating for leo?

Comment 19

[Doug Turner (:dougt)]

let's work with Mark to host this service.  In this way, we can help update the database without having a API key from opencellid.org.

Comment 20

[Alex Keybl [:akeybl]]

Comment on attachment 735009 [details] [diff] [review]
CellId positioning using opencellid.org api

Approving this new feature for v1.1 instead of blocking on this bug. If this causes significant regressions, we will back out and try again in a later release.

Comment 21

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: WIP patch that adds MozCellDB API support and data collecting

Comment 22

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: WIP patch that adds MozCellDB API support and data collecting v2

Comment 23

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: CellId positioning and data collecting

Two patches merged into one.

Comment 24

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: CellId positioning and data collecting

remove opencellid bits

Comment 25

[Josh Matthews [:jdm]]

Comment on attachment 739115 [details] [diff] [review]
CellId positioning and data collecting

Review of attachment 739115 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/CellGeolocationProvider.js
@@ +79,5 @@
> +    this.updater = c;
> +  },
> +
> +  shutdown: function() {
> +    if (this.timer != null) {

nit: if (this.timer)

@@ +83,5 @@
> +    if (this.timer != null) {
> +      this.timer.cancel();
> +      this.timer = null;
> +    }
> +    if (this.ril != null) {

nit: if (this.ril)

@@ +175,5 @@
> +    LOG(apiUrl);
> +
> +    let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
> +    xhr.mozBackgroundRequest = true;
> +    xhr.open("GET", apiUrl, true);

This really looks like it should be a POST instead.

@@ +184,5 @@
> +    xhr.addEventListener("load", function(req) {
> +      LOG(req.target.responseText);
> +      let response = JSON.parse(req.target.responseText);
> +      if (response && "ok" == response.status) {
> +        // success!

If we don't need to do anything else with the response, can you make that clear in the comment?

@@ +196,5 @@
> +    this._updateLocation(newLocation);
> +  },
> +
> +  _updateLocation: function updateLocation(location) {
> +    if (location != null) {

nit: if (location)

@@ +199,5 @@
> +  _updateLocation: function updateLocation(location) {
> +    if (location != null) {
> +      this.lastLocation = location;
> +    }
> +    if (this.lastLocation != null) {

nit: if (this.lastLocation)

@@ +206,5 @@
> +           this.lastLocation.coords.longitude + " " + this.lastLocation.coords.accuracy);
> +    }
> +  },
> +
> +  _valueCached: function valueCached(mcc, mnc, lac, cellid) {

Call this _cachedValuesEqual.

::: xpcom/system/nsIGeolocationProvider.idl
@@ +68,5 @@
> +   * inject
> +   * When a location change is observed by other providers, we get an
> +   * update about it.
> +   */
> +  void inject(in nsIDOMGeoPosition position);

Please implement stubs for the other providers (Network and GPSD), or the terminal will show annoying xpconnect errors in debug builds.

Comment 26

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Josh Matthews [:jdm] from comment #25)
> > +    xhr.mozBackgroundRequest = true;
> > +    xhr.open("GET", apiUrl, true);
> 
> This really looks like it should be a POST instead.

Server side are changing to a json based api, so it's likely that we will have to change this again...

Comment 27

[Doug Turner (:dougt)]

Comment on attachment 739115 [details] [diff] [review]
CellId positioning and data collecting


we can use https://location.services.mozilla.com

Comment 28

[Andrew Overholt [:overholt]]

Comment on attachment 739115 [details] [diff] [review]
CellId positioning and data collecting

It seems like Alex intended for comment 20 to be a pre-approval so carrying that here.

Comment 29

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: CellId positioning and data collection

Updated to use https://location.services.mozilla.com with new JSON API documented at https://mozilla-ichnaea.readthedocs.org/en/latest/

Fixed nits from comment 25.

@overholt: could you mark as approval-mozilla-b2g18+ again?

@jdm: Not carrying forward the r+ since you should like a look at the modified _truncate() and _toApproximateInteger() functions. Also the stubs added to existing providers.

Comment 30

[Hanno Schlichting]

After looking at some more competing API's, we made some minor edits to the API, which need to be reflected in the code.

1. Instead of using "voice.relSignalStrength" for a "strength" entry, the code should sent a "signal" entry denoting the signal strength in dBm (so likely a negative integer).

2. If any cell entries are sent, a new top level key called "radio" should be included, which is one of the strings "gsm" or "cdma". The API docs explains how the different CDMA identifiers (system id, network id, base station id) should be mapped into the mnc, lac and cid entries. Probably CDMA support should be tracked in a separate bug.

3. The wifi related section of the API has been extended, but that's for bug 864716.

Comment 31

[Josh Matthews [:jdm]]

Comment on attachment 740738 [details] [diff] [review]
CellId positioning and data collection

Review of attachment 740738 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/CellGeolocationProvider.js
@@ +131,5 @@
> +    this.api.getCurrentPosition(mcc, mnc, lac, cellid, signal, this);
> +  },
> +
> +  inject: function inject(position)
> +  {

Move this on the previous line.

::: dom/system/MozCellDBAPI.js
@@ +32,5 @@
> +    LOG(apiUrl);
> +    let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +              .createInstance(Ci.nsIXMLHttpRequest);
> +    xhr.mozBackgroundRequest = true;
> +    xhr.open("POST", apiUrl, true);

Really? For a search?

Comment 32

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Shouldn't getCurrentPosition be called getCellPosition? Because it returns not the position of the device but the Cell instead.

Comment 33

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

(In reply to Josh Matthews [:jdm] from comment #31)
...
> Move this on the previous line.
> 
> ::: dom/system/MozCellDBAPI.js
> @@ +32,5 @@
> > +    LOG(apiUrl);
> > +    let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> > +              .createInstance(Ci.nsIXMLHttpRequest);
> > +    xhr.mozBackgroundRequest = true;
> > +    xhr.open("POST", apiUrl, true);
> 
> Really? For a search?

Sending a POST for a search might not be completely be super RESTey but I don't see anything wrong with it. From a security point of view it actually is a good thing because it prevents personal information from appearing in server logs. (An issue we've had with other services)

BTW ElasticSearch actually uses a GET with a body for search queries. From an API design point of view I like that. But, I don't know how safe it is to use that if you are on public infrastructure with proxies and filters and whatnot.

Comment 34

[Tarek Ziadé (:tarek)]

(In reply to Stefan Arentz [:st3fan] from comment #33)
> 
> BTW ElasticSearch actually uses a GET with a body for search queries. From
> an API design point of view I like that. But, I don't know how safe it is to
> use that if you are on public infrastructure with proxies and filters and
> whatnot.

yes that's why it also allows you to use POST instead of GET for its search API.

Comment 35

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: CellId positioning and data collection

Uses notifyVoiceChanged() rather than timeout.
Adds "radio" to JSON payload.
Uses signalStrength in dBm.

@vyang: Is notifyVoiceChanged() called periodically? On a SIM I'm using, it seems to be called once after the geolocation application is started. But I don't know under what conditions it will be called again. If notifyVoiceChanged() isn't always called on geolocation access, the patch will need an initial manual call to notifyVoiceChanged() to kick start the process. Asking you based on blame info for RILContentHelper.

Comment 36

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I'm not sure why the separate getCurrentLocation() interface is needed. Kanru said that way additional providers could be plugged in, but in the CellGeolocationProvider itself, we are using the mozilla API.

Comment 37

[Hanno Schlichting]

Comment on attachment 741251 [details] [diff] [review]
CellId positioning and data collection

In one function you use:

let signal = voice.signalStrength;

and two other places:

let signalStrength = voice.signalStrength;

That should probably be consistent and use "signal" as the variable name in all places.

And then there's two places in the code sending the payload having:

"strength": parseInt(signalStrength, 10),

which should be:

"signal": parseInt(signal, 10),

I changed the key in the API, after I saw your choice of local variable names and failing to type strength myself a couple times.

Comment 38

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Nikhil Marathe from comment #35)
> Created attachment 741251 [details] [diff] [review]
> CellId positioning and data collection
> 
> Uses notifyVoiceChanged() rather than timeout.
> Adds "radio" to JSON payload.
> Uses signalStrength in dBm.
> 
> @vyang: Is notifyVoiceChanged() called periodically? On a SIM I'm using, it
> seems to be called once after the geolocation application is started. But I
> don't know under what conditions it will be called again. If
> notifyVoiceChanged() isn't always called on geolocation access, the patch
> will need an initial manual call to notifyVoiceChanged() to kick start the
> process. Asking you based on blame info for RILContentHelper.

I'm sill not sure when will we get this notification. It would be great if it was called for any cell update. Otherwise we still need the timer fallback.

Comment 39

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Nikhil Marathe from comment #36)
> I'm not sure why the separate getCurrentLocation() interface is needed.
> Kanru said that way additional providers could be plugged in, but in the
> CellGeolocationProvider itself, we are using the mozilla API.

In CellGeolocationProvider itself we use the mozilla API to "update" the location data. But we could use different API providers to query the location. I've implemented opencellid.org, minigps.net and mozilla API support so far.

If we decided that we only want to use the mozilla database then I'm fine to remove the additional interface.

Comment 40

[Hanno Schlichting]

After feedback from st3fan, we moved the lat/lon in the measurement API to the JSON body and renamed the endpoint to `/v1/submit/`.

So a request should now be made against:

https://location.s.m.o/v1/submit

with a data payload like:

{"lat": 12.345678, "lon": 23.45678, "radio": "gsm", "cell": [], "wifi": []}

The new code is already deployed on the server.

Comment 41

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: CellId positioning and data collection

Updated patch. I'm testing the notifyVoiceChanged callback.

Comment 42

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 741763 [details] [diff] [review]
CellId positioning and data collection

Review of attachment 741763 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 43

[Hanno Schlichting]

Comment on attachment 741763 [details] [diff] [review]
CellId positioning and data collection

In "b/dom/system/MozCellDBAPI.js" the function signature for "getCurrentPosition: function" still contains "signalStrength" instead of just "signal" as a variable name and further down in the payload part, it should be:

"signal": parseInt(signal, 10),

instead of:

"strength": parseInt(signalStrength, 10),

Comment 44

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Hanno Schlichting [:hannosch] from comment #43)
> Comment on attachment 741763 [details] [diff] [review]
> CellId positioning and data collection
> 
> In "b/dom/system/MozCellDBAPI.js" the function signature for
> "getCurrentPosition: function" still contains "signalStrength" instead of
> just "signal" as a variable name and further down in the payload part, it
> should be:
> 
> "signal": parseInt(signal, 10),
> 
> instead of:
> 
> "strength": parseInt(signalStrength, 10),

Ah, thanks. I missed this one.

Comment 45

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Kan-Ru Chen [:kanru] from comment #38)
> (In reply to Nikhil Marathe from comment #35)
> > Created attachment 741251 [details] [diff] [review]
> > CellId positioning and data collection
> > 
> > Uses notifyVoiceChanged() rather than timeout.
> > Adds "radio" to JSON payload.
> > Uses signalStrength in dBm.
> > 
> > @vyang: Is notifyVoiceChanged() called periodically? On a SIM I'm using, it
> > seems to be called once after the geolocation application is started. But I
> > don't know under what conditions it will be called again. If
> > notifyVoiceChanged() isn't always called on geolocation access, the patch
> > will need an initial manual call to notifyVoiceChanged() to kick start the
> > process. Asking you based on blame info for RILContentHelper.
> 
> I'm sill not sure when will we get this notification. It would be great if
> it was called for any cell update. Otherwise we still need the timer
> fallback.

Tested, notifyVoiceChanged is called each time when there is cell tower change. However even using this approach, I think we should still send locations regularly otherwise we could timeout in watchPosition if we didn't move. NetworkGeolocationProvider has similar problem.

Comment 46

[Kai-Chih Hu [:khu]]

Talked with Kan-Ru, Doug, could you help to review this bug when :jdm is on PTO? Thank you!

Comment 47

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: CellId positioning and data collection

Change all signalStrength variable to signal.

Call notifyVoiceChange() at startup.

Comment 48

[Hanno Schlichting]

Comment on attachment 742942 [details] [diff] [review]
CellId positioning and data collection

Protocol / server interaction looks good to me.

Comment 49

[Doug Turner (:dougt)]

Comment on attachment 742942 [details] [diff] [review]
CellId positioning and data collection

Review of attachment 742942 [details] [diff] [review]:
-----------------------------------------------------------------

Looks really good.

Kan-Ru,  jdm has made some significant changes in bug 863051.  We have a patch queue: https://hg.mozilla.org/users/dougt_mozilla.com/geolocation_redesign/.  I think we will want to rebase this patch on top of that work.  Can you do this, or do you want me to roll this into that work?

::: dom/src/geolocation/nsGeolocation.cpp
@@ +880,5 @@
> +  if (aSomewhere) {
> +    for (uint32_t i = 0; i < mProviders.Length(); i++) {
> +      mProviders[i]->Inject(aSomewhere);
> +    }
> +  }

This is going to change a bit when bug 863051 lands.

::: dom/system/CellGeolocationProvider.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

This provider is going to change a bit after we land bug 863051.

@@ +28,5 @@
> +CellIDGeoCoordsObject.prototype = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIDOMGeoPositionCoords]),
> +  classInfo: XPCOMUtils.generateCI(
> +    {interfaces: [Ci.nsIDOMGeoPositionCoords],
> +     flags: Ci.nsIClassInfo.DOM_OBJECT,

Does this object ever get forward to content, or do you just send this to the geolocation provider?  If this object doesn't go to content, maybe you can drop the flags here.

@@ +34,5 @@
> +};
> +
> +function CellIDGeoPositionObject(lat, lng, acc) {
> +  this.coords = new CellIDGeoCoordsObject(lat, lng, acc, 0, 0);
> +  this.address = null;

We no longer support .address.

@@ +42,5 @@
> +CellIDGeoPositionObject.prototype = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIDOMGeoPosition]),
> +  classInfo: XPCOMUtils.generateCI(
> +    {interfaces: [Ci.nsIDOMGeoPosition],
> +     flags: Ci.nsIClassInfo.DOM_OBJECT,

Same comment as above.  is this is internal only, then we don't need to mark this object as a DOM_OBJECT

@@ +48,5 @@
> +};
> +
> +function CellIDGeoPositionProvider() {
> +  this.api = Cc["@mozilla.org/geolocation/cellid-api;1"]
> +             .createInstance(Ci.nsICellIdAPI);

Kind of a funky variable name.  Nit: don't include 'api' in interface names.

@@ +66,5 @@
> +                                           Ci.nsICellIdProvider,
> +                                           Ci.nsIMobileConnectionListener]),
> +  startup:  function() {
> +    if (this.started)
> +      return;

style nit.

if (test) {
  stmt;
}

@@ +67,5 @@
> +                                           Ci.nsIMobileConnectionListener]),
> +  startup:  function() {
> +    if (this.started)
> +      return;
> +    this.started = true;

move this line to the bottom of the function in case you throw from the following calls.

@@ +154,5 @@
> +      LOG("no RIL service?");
> +      return;
> +    }
> +
> +    let voice = this.ril.rilContext.voice;

Does every ril have a valid rilContext, or can that be null?

@@ +169,5 @@
> +    let cellid = voice.cell.gsmCellId;
> +    let signal = voice.signalStrength;
> +
> +    let apiUrlBase = Services.prefs.getCharPref("geo.cellid.uri");
> +    let apiUrl = apiUrlBase + '/v1/submit';

You probably do not want to do this.  Instead, just use the pref without modification.

You may also want to add code here to ensure that the scheme is https (because, 'belts and suspenders')

@@ +193,5 @@
> +      }
> +    }, false);
> +
> +    let payload = {
> +      "lat": this._truncate(position.coords.latitude, 6),

Magic number -- why 6?

@@ +261,5 @@
> +    return Math.floor(n * Math.pow(10, precision));
> +  },
> +
> +  _closeEnough: function closeEnough(oldPos, newPos) {
> +    let oldLat = this._toApproximateInteger(oldPos.coords.latitude, 3);

What are you trying to do here.  Can you add a comment?

::: dom/system/MozCellDBAPI.js
@@ +27,5 @@
> +  getCurrentPosition: function(networkType, mcc, mnc, lac,
> +                               cellid, signal, update) {
> +    // mcc/mnc/lac/cid
> +    let apiUrlBase = Services.prefs.getCharPref("geo.cellid.uri");
> +    let apiUrl = apiUrlBase + '/v1/search';

Same.  Just use the pref.

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +651,5 @@
> +GonkGPSGeolocationProvider::Inject(nsIDOMGeoPosition *aSomewhere)
> +{
> +  if (mGpsInterface) {
> +    nsCOMPtr<nsIDOMGeoPositionCoords> coords;
> +    aSomewhere->GetCoords(getter_AddRefs(coords));

if (!coords) {
  return NS_ERROR_FAILURE;
}

::: dom/system/nsICellIdProvider.idl
@@ +4,5 @@
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(1496b6d3-f96c-4ab7-b852-53e247556cbc
> +)]

move this up one line.

@@ +14,5 @@
> +
> +[scriptable, uuid(fce7de30-35fd-404f-9576-7623117d9847)]
> +interface nsICellIdAPI : nsISupports
> +{
> +  void getCurrentPosition(in DOMString networkType,

I would like a comment block above this.  Please, define the arguments. what unit is signal in?

::: xpcom/system/nsIGeolocationProvider.idl
@@ +68,5 @@
> +   * inject
> +   * When a location change is observed by other providers, we get an
> +   * update about it.
> +   */
> +  void inject(in nsIDOMGeoPosition position);

Take a look at the bug 863051.  This interface is going to change a bit.  It might make sense to rebase on that patchset...

Comment 50

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Doug Turner (:dougt) from comment #49)
> Comment on attachment 742942 [details] [diff] [review]
> CellId positioning and data collection
> 
> Review of attachment 742942 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks really good.
> 
> Kan-Ru,  jdm has made some significant changes in bug 863051.  We have a
> patch queue:
> https://hg.mozilla.org/users/dougt_mozilla.com/geolocation_redesign/.  I
> think we will want to rebase this patch on top of that work.  Can you do
> this, or do you want me to roll this into that work?

Ah, I'll take a look of the patch queue.

Comment 51

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: [wrong bug] Send wifi accesspoints to submit API call

Applies on top of attachment 742942 [details] [diff] [review].

Comment 52

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 743077 [details] [diff] [review]
[wrong bug] Send wifi accesspoints to submit API call

Attached patch to wrong bug. Updating 864716.

Comment 53

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

(In reply to Nikhil Marathe from comment #52)
> Comment on attachment 743077 [details] [diff] [review]
> [wrong bug] Send wifi accesspoints to submit API call
> 
> Attached patch to wrong bug. Updating 864716.

What does the LOG() function do?

We should be certain that this is disabled for production builds. We do not want to leak and record any information on the device.

Comment 54

[Hanno Schlichting]

We made two more changes to the submit API which need to be reflected in the patch:

1. The submit API now takes a batch of measurements (possibly of batch size 1)
2. For each measurement the client can sent along a timestamp of that measurement

API docs have been updated at https://mozilla-ichnaea.readthedocs.org/en/latest/#service-1

1. means there's an outer {"items": [<measure>]} wrapper
2. means you can sent a "time": "2012-03-15T11:12:13.456Z" as a new key for each measurement inside the batch

Comment 55

[Kan-Ru Chen [:kanru] (UTC+9)]

Since jdm has made some significant changes in bug 863051, I plan to change the CellID provider to a MozCellDBService and use it in GonkGPSGeolocationProvider

Comment 56

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: wip MozCellDBService

Comment 57

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: wip MozCellDBService

WIP hook GonkGPSGeolocationProvider and new API.

TODO: batch upload.

Comment 58

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: wip MozCellDBService

TODO: polish

Comment 59

[Hanno Schlichting]

Comment on attachment 746873 [details] [diff] [review]
wip MozCellDBService

Server side interaction code looks ok. It's still missing the new per report "time" field.

Comment 60

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: MozCellDBService

This is based on the redesign in bug 863051. It would need rebase once that bug is landed or we decide we don't want to land 863051.

Comment 61

[Doug Turner (:dougt)]

Comment on attachment 747276 [details] [diff] [review]
MozCellDBService

Review of attachment 747276 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/MozCellDBService.js
@@ +43,5 @@
> +                                           Ci.nsIMobileConnectionListener]),
> +
> +  registerCallback: function registerCallback(callback) {
> +    LOG("register callback");
> +    if (this.callbacks.indexOf(callback) == -1) {

if you register twice, shouldn't the second time you register for the same callback throw?

I am worried about this pattern:

register(foo)
register(foo)
unregister(foo)
unregister(foo) <----

@@ +172,5 @@
> +          timestamp: null,
> +          coords: {
> +            latitude: 25.033611,
> +            longitude: 121.564444,
> +            accuracy: 100,

Clearly, the accuracy should be 101.

@@ +173,5 @@
> +          coords: {
> +            latitude: 25.033611,
> +            longitude: 121.564444,
> +            accuracy: 100,
> +            altitude: 0,

509m

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +34,5 @@
>  #ifdef AGPS_TYPE_INVALID
>  #define AGPS_HAVE_DUAL_APN
>  #endif
>  
> +#define DEBUG_GPS 1

revert?

@@ +94,5 @@
>        if (callback) {
>          callback->Update(mPosition);
>        }
> +      if (provider->mMozCellDB) {
> +        provider->mMozCellDB->UnregisterCallback(provider);

why do you unregister the callback here?

::: dom/system/nsIMozCellDBService.idl
@@ +7,5 @@
> +
> +[scriptable, uuid(89036553-ded6-471d-9353-0c38d51caecb)]
> +interface nsIMozCellDBServiceCallback : nsISupports
> +{
> +  void cellTowerChanged(in nsIDOMGeoPosition position);

This is a weird name for a method.  it name implies that you might be able to get information about the cell tower, but all you get is the position change.

Would you mind changing this name to cellLocationChanged? or something that implies that the this event is about position changes more than tower changes.

@@ +22,5 @@
> +   * Submit cell tower and WIFI station data to server.
> +   * All data is collected then sent in batch when WIFI is the primary
> +   * connection.
> +   */
> +  void submit(in nsIDOMGeoPosition position);

updatePosition(in nsIDOMGeoPosition position)

Comment 62

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Doug Turner (:dougt) from comment #61)
> Comment on attachment 747276 [details] [diff] [review]
> MozCellDBService
> 
> Review of attachment 747276 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/MozCellDBService.js
> @@ +43,5 @@
> > +                                           Ci.nsIMobileConnectionListener]),
> > +
> > +  registerCallback: function registerCallback(callback) {
> > +    LOG("register callback");
> > +    if (this.callbacks.indexOf(callback) == -1) {
> 
> if you register twice, shouldn't the second time you register for the same
> callback throw?
> 
> I am worried about this pattern:
> 
> register(foo)
> register(foo)
> unregister(foo)
> unregister(foo) <----

The second unregister should be just a no-op?

> @@ +172,5 @@
> > +          timestamp: null,
> > +          coords: {
> > +            latitude: 25.033611,
> > +            longitude: 121.564444,
> > +            accuracy: 100,
> 
> Clearly, the accuracy should be 101.
> @@ +173,5 @@
> > +          coords: {
> > +            latitude: 25.033611,
> > +            longitude: 121.564444,
> > +            accuracy: 100,
> > +            altitude: 0,
> 
> 509m

LOL

> ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
> @@ +34,5 @@
> >  #ifdef AGPS_TYPE_INVALID
> >  #define AGPS_HAVE_DUAL_APN
> >  #endif
> >  
> > +#define DEBUG_GPS 1
> 
> revert?
> 
> @@ +94,5 @@
> >        if (callback) {
> >          callback->Update(mPosition);
> >        }
> > +      if (provider->mMozCellDB) {
> > +        provider->mMozCellDB->UnregisterCallback(provider);
> 
> why do you unregister the callback here?

Because we started to receive positions from the GPS. We don't want to send the less-accurate position to nsGeolocation otherwise we will have to do the IsBetter dance again.

> > +  void cellTowerChanged(in nsIDOMGeoPosition position);
> 
> This is a weird name for a method.  it name implies that you might be able
> to get information about the cell tower, but all you get is the position
> change.
> 
> Would you mind changing this name to cellLocationChanged? or something that
> implies that the this event is about position changes more than tower
> changes.

I agree. cellLocationChanged looks good to me.

Comment 63

[Doug Turner (:dougt)]

Comment on attachment 747276 [details] [diff] [review]
MozCellDBService

please post a fixed up patch.  it is very close.

Comment 64

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: MozCellDBService

I forgot to update the patch...

Comment 65

[Kan-Ru Chen [:kanru] (UTC+9)]

Let's land 863051 together?

Comment 67

[Dylan Oliver [:doliver]]

Kanru - I see bug 863051 is no longer leo+. What do we need to do to move this bug forward?

Comment 68

[Kan-Ru Chen [:kanru] (UTC+9)]

As I understood the Geolocation team wants to have this enabled on Android first and collect some real data. One problem is that the service API is still changing and we will have to change the client code as well to match the API. If that won't happen too often then we could still land this code without bug 863051 (with some revision of course).

Comment 69

[Hanno Schlichting]

This status of this bug is a bit unclear, as it's really about three different things.

1. FxOS phones need geo-location lookup
2. FxOS phones need geo-location lookup when GPS is disabled
3. Mozilla should host and implement a CellID/Location service

All the attached patches are about the third option. We are working on a Mozilla hosted service, but this is a multiple-quarter project and we are in the early stages. At this point we are getting data from Android nightly (via opt-in) and we don't want to handle more data just yet. So keeping the patch pending is the right thing to do.

All this is different from what the bug was originally about. For the first option, we have geo-lookup based on A-GPS and a SUPL server.

The status of the second option (geo-lookup without GPS) is unclear to me. I believe the SUPL protocol also works without GPS, but could be mistaken. Someone more knowledgeable has to chime in on this.

Comment 70

[Pin Zhang [:pzhang] (inactive)]

Hi Guys, I think we should implement a separate Location Service[1][2] based geolocation provider instead of merge it to the current gps geolocation provider that attachment 754361 [details] [diff] [review] did, there are two reasons for my proposal: 1, geolocation team will bring the service to multiple platforms including b2g and android as Kanru mentioned in comment 68, and I think it should work on laptop because we can get the position by wifi info, so implementing a separate provider based on this service make more sense to me. 2, the gps geolocation provider might be overridden by vendor's own version, which is exactly happened on Himachi (you can check $B2G/distribution/bundles/ on Himachi), then the benefit of location service will be lost, and Vendor have to implement them from scratch if they want to bring it back.

what do you think?
 
[1] https://location.services.mozilla.com/
[2] https://mozilla-ichnaea.readthedocs.org/en/latest/api/search.html

Comment 71

[Hanno Schlichting]

In order to support the Mozilla Location Service, we probably want to use the http://mozilla-ichnaea.readthedocs.org/en/latest/api/geolocate.html API - which is a close replica of the API used by Google. So in current Firefox desktop you can change geo.wifi.uri and use our service. No other code changes required. I'm guessing other platforms might already have similar support code for the Google API.

Comment 72

[Pin Zhang [:pzhang] (inactive)]

(In reply to Hanno Schlichting [:hannosch] from comment #71)
> In order to support the Mozilla Location Service, we probably want to use
> the http://mozilla-ichnaea.readthedocs.org/en/latest/api/geolocate.html API
> - which is a close replica of the API used by Google. So in current Firefox
> desktop you can change geo.wifi.uri and use our service. No other code
> changes required. I'm guessing other platforms might already have similar
> support code for the Google API.

But the Location Service needs devices' GPS position plus Cell ID and wifi info to be submitted (http://mozilla-ichnaea.readthedocs.org/en/latest/api/submit.html) for the data accuracy and quality further improvements, so if only change geo.wifi.uri won't help on that.

Comment 73

[Robert Kaiser]

(In reply to Pin Zhang [:pzhang] from comment #72)
> But the Location Service needs devices' GPS position plus Cell ID and wifi
> info to be submitted
> (http://mozilla-ichnaea.readthedocs.org/en/latest/api/submit.html) for the
> data accuracy and quality further improvements, so if only change
> geo.wifi.uri won't help on that.

You're confusing "stumbling", i.e. sending data into our DB (where we need a GPS location plus the seen wifi/cell signals) with actually using the service, where we give you a location based on the wifi/cell signals you see and our DB.
The former is what that "sumbit" page you link is describing, the latter is what is the replacement for the Google service.

Comment 74

[Elad Alfassa]

I tried setting geo.wifi.uri to that API url provided in that doc on my laptop, and it caused every geolocation request to fail without a specific error message. We need to make sure this service works before we switch Firefox to it.

Comment 75

[Pin Zhang [:pzhang] (inactive)]

(In reply to Robert Kaiser (:[e-mail]) from comment #73)
> (In reply to Pin Zhang [:pzhang] from comment #72)
> > But the Location Service needs devices' GPS position plus Cell ID and wifi
> > info to be submitted
> > (http://mozilla-ichnaea.readthedocs.org/en/latest/api/submit.html) for the
> > data accuracy and quality further improvements, so if only change
> > geo.wifi.uri won't help on that.
> 
> You're confusing "stumbling", i.e. sending data into our DB (where we need a
> GPS location plus the seen wifi/cell signals) with actually using the
> service, where we give you a location based on the wifi/cell signals you see
> and our DB.
> The former is what that "sumbit" page you link is describing, the latter is
> what is the replacement for the Google service.

Thanks for your explanation, but i don't think i am confusing. My proposal might be kinda an improved version of the NetworkGeolocationProvider[1] for mobile device (Firefox OS and Firefox for Android), because the mobile device has GPS HW, we could and should leverage it to improve the Location Service, i.e sending data back to service db, either Mozilla's or 3rd party's service with compatible query/submit interface.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js

Comment 76

[Hanno Schlichting]

Thanks everyone who contributed to this bug.

Going back to comment 69 (https://bugzilla.mozilla.org/show_bug.cgi?id=837987#c69) - the discussion here is now about three different unrelated things.

The issue was originally about general problems with geolocation lookups on FxOS devices. Those are tracked elsewhere.

For Mozilla Location Service integration into FxOS a new issue should be created with https://bugzilla.mozilla.org/show_bug.cgi?id=862827 as a parent bug. Though currently there are no concrete plans to do so.