1009592 - [Tarako] Avoid too many outbound network requests for Geo lookups

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Hanno Schlichting]

The debug log in #1009172 suggests that we do an outbound service request for every geolocation lookup, many of those being triggered by using the watchPosition API. The debug log from #1007953 also shows many outbound requests triggered by the HERE maps on FxOS (one every 5 seconds).

This sounds bad both from a client resource perspective and also from a service perspective (where we operate or pay for the service), especially if the device is actually stationary most of the time.

I wonder if there are simple measures we can do to avoid this. For example the addition of a small LRU cache (5-10 entries) which would remember the query and answer being sent to the outbound service. Before doing a new query, we could check the LRU cache to see if we already got a result for the query and use the cached response. The service will give the same answer to the same query for a long time, certainly for days to weeks.

One tricky part here is defining what the same query actually is. The queries contain signal strength numbers which will be rather variable, but won't effect the service answer in any significant way (or at all in case of MLS).

Another nice effect of such a cache would be to speed up the API response. Doing an outbound service request involves establishing a new SSL connection, which takes quite some time on high-latency networks.

Comment 2

[Doug Turner (:dougt)]

note that bug 1011643 has a wip patch based against a version of 1.3T

Comment 3

[David Flanagan [:djf]]

The wip patch in bug 1011643 was actually based on m-c, not on the 1.3t branch, but it looks like the code is pretty similar. Let me know if I can help.  I'd really love to get this fixed for Tarako.

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

triage: can we get the patch reviewed? once the patch is ready let's uplift to 1.3T but let's not block on this

Comment 5

[Hanno Schlichting]

Garvan asked for some comments in private mail. Replicating here. Part of his WIP patch is at http://pastebin.com/qg5wMS1j

To decide if Wifi networks have changed, I suggested to look at the complete set of bssids. Consider an old position to be correct if there's a 50% or more overlap between the old and new bssid set.

At this point we want to ignore signal strength, as it's too noisy. Just holding the device differently, walking behind a steel car / elevator shaft and many more things can cause the signal reading to go from almost perfect to almost nothing. At this point you either observe a WiFi network or you don't and we can treat it as a binary choice.

I'd also avoid the top-X logic from djf's patch and instead look at the full set. Most of the WiFi networks won't be in our database, and some of them might be from moving networks, like those from smartphone hotspots, train and bus networks. So the top-X list of networks might stay the same while you are moving. Or the service only uses the two networks with weakest signals to position you, so we want to consider a significant change in the weak WiFi's to trigger a new lookup.

For comparing cell networks, we currently only ever have a single GSM cell in the list, as we only use the serving cell. In order to compare two cells, you need to compare their full unique identifier. This includes all of radioType (currently hardcoded to 'gsm'), mobileCountryCode, mobileNetworkCode, locationAreaCode and cellId. Only those five pieces of information together uniquely identify a cell. A change in any one of them means you are observing a different cell.

Comment 6

[:garvan]

Attachment: bug_1009592

This patch applies over 1.3t after patches 1014924, patches 1007953. 

When xhr.onload gets a new location, cache the location (along with wifi and cell data used to get that location). Next time through, before xhr.send, check if the cell and/or wifi data are sufficiently unchanged that we can use the cached request.

Comment 7

[:garvan]

I mean  bug 1014924 and bug 1007953

Comment 8

[:garvan]

I'll get this right: I mean bug 1014916 and bug 1007953. 

Without the former, I can't run geolocation at all on my Tarako.

Comment 9

[David Flanagan [:djf]]

Comment on attachment 8431132 [details] [diff] [review]
bug_1009592

setting the patch flag so it is easier to review.

Comment 10

[:garvan]

Attachment: bug_1009592.diff

The last one I cleaned something manually and when I tried to reapply patch said it was malformatted

Comment 11

[David Flanagan [:djf]]

Comment on attachment 8431132 [details] [diff] [review]
bug_1009592

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

For what its worth, here are my comments on this patch

::: dom/system/NetworkGeolocationProvider.js
@@ +75,5 @@
> +    for (let i = 0; i < smallerList.length; i++) {
> +      if (smallerList[i].macAddress in wifiHash) {
> +        common++;
> +      }
> +    }

If you sorted the lists by mac address instead of signal
strength, there is probably a linear time algorithm for
counting the common elements in a single loop.

If you don't want to risk doing that, though, consider using the
ES6 Set class.

   new Set(this.wifiList.map(function(x) { return x.macAddress}))

@@ +77,5 @@
> +        common++;
> +      }
> +    }
> +
> +    return common >= (largerList.length / 2);

Use a symbolic constant here instead of 2?  Since the 50% thing is just a guess, we should make this easy to tweak.

@@ +93,5 @@
> +      LOG("cell not equal len");
> +      return false;
> +    }
> +
> +    function makeCellHashKey(cell) {

This seems more complicated, than necessary, especially given
that typically only have one cell tower.  How about just
JSON.stringify(this.cellInfo) === JSON.stringify(cellInfo)

Failing that, since you've required that the number be the same,
why not just loop through the arrays and test that each of the
entries is the same.  Doing it with the cellHash is more
complicated that it needs to be.

@@ +118,5 @@
> +
> +let gCachedLocation = null;
> +
> +// return null if not better
> +function getCachedLocIfBetter(newCell, newWifiList)

I don't get this function name. Better than what?

How about just calling this useCachedLocation() and having it return true or false?

@@ +125,5 @@
> +    return null;
> +  }
> +
> +  // if new request has both cell and wifi, and old is just cell
> +  if (gCachedLocation.isCellOnly() && newCell && newWifiList) {

What if the old location was wifi only?  That probably won't happen, but I think you could generalize the logic here to something like this:

 if ((newCell && !gCachedLocation.cellInfo) ||
     (newWifiList && !gCachedLocation.wifiList) ||
     (!newCell && gCachedLocation.cellInfo) ||
     (!newWifiList && gCachedLocation.wifiList))

@@ +144,5 @@
> +  if (newWifiList) {
> +    hasEqualWifis = gCachedLocation.isWifiApproxEqual(newWifiList);
> +  }
> +
> +  if (gCachedLocation.isCellOnly()) {

I often test my devices with no sim card in them, and we might
want this to work on tablets that don't have any telephony, so it
would be good to support the wifi only case, too.

In any case, this logic seems overly complicated. Why can't we just
say: if (hasEqualCells && hasEqualWifis) return gCachedLocation.location

@@ +355,5 @@
>      xhr.onerror = function() {
>        listener.notifyError(POSITION_UNAVAILABLE);
>      };
> +
> +    let potentialCachedLocation = null;

I think it would be simpler to declare the data object above and use that to store the location data.

@@ +373,5 @@
> +      if (potentialCachedLocation) {
> +        potentialCachedLocation.location = newLocation;
> +        gCachedLocation = potentialCachedLocation;
> +        potentialCachedLocation = null;
> +      }

This could just be:

  gCachedLocation = new CachedLocation(newPosition, data.cellTowers, data.accessPoints);

Then you wouldn't need potentialCachedLocation and you would only create a CachedLocation object when you actually had the location.

@@ +394,5 @@
> +      listener.update(prevGoodLocation);
> +      LOG("use prev");
> +    } else {
> +      potentialCachedLocation = new CachedLocation(null, data.cellTowers, data.wifiAccessPoints);
> +      data = JSON.stringify(data);

If you're going to make the change I recommended above, get rid of potentialCachedLocation here, and don't overwrite data. But do something like:

 let requestBody = JSON.stringify(data)

Comment 12

[David Flanagan [:djf]]

I'm also concerned about the patch in bug 1007953.  That one assumes that if the user turns off wifi, then wifiScanningEnabled will be false.  But that doesn't seem to be the case for me.  If I turn off wifi on my phone, geolocation keeps on trying to scan for accessPoints. It never gets any, and since there is no timer running, it never every updates the location.  So as it stands with that patch and this one, geolocation doesn't work in Tarako if wifi is off (which it will be for most of our user base).  

(Note that this may be related to bug 1014924 where there is a JS error in WifiWorker.js when wifi is turned off.)

To be safe, though, it seems like we might want to have the timer running all the time (like we do on m-c, I think).

Comment 13

[Josh Matthews [:jdm]]

djf's comments on the patch seem reasonable to me.

Comment 14

[:garvan]

Attachment: bug_1009592.diff

> For what its worth, here are my comments on this patch

A lot! Its last minute, but I'd still like to get this in as good shape as possible.
 
> ::: dom/system/NetworkGeolocationProvider.js
> @@ +75,5 @@
> > +    for (let i = 0; i < smallerList.length; i++) {
> > +      if (smallerList[i].macAddress in wifiHash) {
> > +        common++;
> > +      }
> > +    }
> 

The current algo is linear time. I could creating this on assignment however. 

> > +    return common >= (largerList.length / 2);
> Use a symbolic constant here instead of 2?  Since the 50% thing is just a
> guess, we should make this easy to tweak.
> 

Sorry, forgot to do that in this patch. I'll upload another anyway, I think an hg patch is required, but the B2G pull is git, I am trying to get this straightened.
  
> This seems more complicated, than necessary, especially given
> that typically only have one cell tower.  How about just
> JSON.stringify(this.cellInfo) === JSON.stringify(cellInfo)

I added comments as to why not to do this. The algo is linear time for looking up multiple cells (my understanding is that I am to handle multiple).
The values in the cellInfo are being expanded in future, and those will not be used for equality.

The rest of the comments are because my code wasn't clear, sorry about that.
It is not just a cache, it is a cache plus logic for determining if the request origin was better than the current origin.
Ex. wifi betterthan cell betterthan geoip
(and combinations thereof)

Comment 15

[:garvan]

David: oops, typo on line 159 -this is my intermediate patch :)

Comment 16

[David Flanagan [:djf]]

Comment on attachment 8431625 [details] [diff] [review]
bug_1009592.diff

Switching the review request to my mozilla account, so it shows up on my dashboard.

Comment 17

[David Flanagan [:djf]]

Comment on attachment 8431625 [details] [diff] [review]
bug_1009592.diff

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

r- because I don't think that using the cached location when we lose input data is the right thing to do, because we'll end up reporting a position with higher accuracy than what we actually know.  If the user turns off wifi and keeps walking, we'll keep reporting that they are at the location where they turned off wifi, rather than reporting that they are somewhere in a much broader radius.

::: dom/system/NetworkGeolocationProvider.js
@@ +91,5 @@
> +      return false;
> +    }
> +
> +    // Use only these values for equality 
> +    // (the JSON will contain additional values in future) 

Given that the lengths of the two arrays are the same, it would be simpler to just loop through the arrays and compare each of the fields you care about. That's assuming that the order of the cell towers remains the same.  But if each sim card only has one, that is a valid assumption since the sim cards aren't going to change position.

I think this comparison code works, however, so this is just a nit saying that I think it could be simpler.

@@ +123,5 @@
> +// by the nature of its origin (wifi/cell/geoip), use the cached location.
> +// 
> +// If there is more source info than the cachedLocation had, return false
> +// In other cases, MLS is known to produce better/worse accuracy based on the 
> +// inputs, so base the decision on that.

This comment helps me understand what you're doing here a lot. Thanks.

What you don't take into account here, however, is time.  If the cached location is based on more input data than is currently available, you're always going to return the cached location. But then you're reporting invalid accuracy.

If you've got a location based on cell and wifi and then the user enters a building with no wifi signal (or turns off their wifi) you'll continue to report their location at the door of that building rather than updating the position to more honestly return a value with greater uncertainty.

It seems better to me to always make a new location request any time the inputs change, even if the new inputs have less data than the old inputs.

Note that this bug was about the user's location jumping around while the phone was standing still. It wasn't about inputs changing, it was about separate location requests being made for cell and wifi data, right?

I suppose an ideal solution to the problem would be something that accomodated temporary loss of a signal. So if we have cell and wifi and lose one, we continue to return the cached location for 15 seconds or something hoping that the signal will come back, and then if it doesn't we resort to making the less accurate location request.

But that doesn't seem like a 1.3T fix.  For 1.3T I would prefer the simplest possible working code: just make a new location request any time the inputs change significantly.

@@ +138,5 @@
> +
> +  // if new is geoip request
> +  if (!newCell && !newWifiList) {
> +    return true;
> +  }

Can this ever happen with this code?  I guess if wifi is off and there is no sim card in the phone?

@@ +159,5 @@
> +    return gCachedLocation.location;
> +  } else if (gCachedLocation.isCellAndWifi()) {
> +    if ((hasEqualCells && hasEqualWifis) ||
> +         (!newWifiList && hasEqualCells) ||
> +          (!newCell && hasEqualWifis))

If I've got a cached cell and wifi location and I turn off my wifi, the location will never change unless I move far enough to switch to a new cell tower. Significantly, the geolocation app (like a map client) will not be told that the cached location now has lower accuracy. And it will look like the user has a relatively precise location but is not moving.

@@ +353,5 @@
> +      data.wifiAccessPoints = wifiData;
> +    }
> +
> +    let useCached = isCachedLocationMoreAccurateThanServerRequest(data.cellTowers,
> +                                                                  data.wifiAccessPoints);

indentation is weird here.

@@ +367,3 @@
>      let url = Services.urlFormatter.formatURLPref("geo.wifi.uri");
>      let listener = this.listener;
>      LOG("Sending request: " + url + "\n");

Please remove this line, or at least drop the query/hash parts of the url

Comment 18

[:garvan]

> r- because I don't think that using the cached location when we lose input
> data is the right thing to do, because we'll end up reporting a position
> with higher accuracy than what we actually know.  If the user turns off wifi
> and keeps walking, we'll keep reporting that they are at the location where
> they turned off wifi, rather than reporting that they are somewhere in a
> much broader radius.

If they shut off wifi, and we now report that they are 10km away, or if all they get is a geoip, and we report they are 100km away, we don't want to do this. That said we are looking mainly at the percieved 99% use case, which is getCurrentLocation on a webpage, or app, that only needs approximate location (Home Depot website, show me store locations.) 
An app using the watch position is a different use case, and I think it may need to fall outside of this logic. NetGeoProv has no knowledge of watch vs getcurrent that know of, and it doesn't handle high vs. low accuracy. And a few other things. It needs some serious refactoring. I just don't know we can do that right now.

Comment 19

[Doug Turner (:dougt)]

for battery usage and dos concerns, we must fix this.

Comment 20

[:garvan]

Attachment: bug_1009592.diff

This is for 1.3 T only. Will open another bug for m-c.

This patch applies cleanly to the 1.3t at this time. Two changes from pref patch: 1) clear the cache on provider shutdown, so we don't keep using this cached value indefinitely (added a TODO: in future we can expand on the cache lifespan logic).
2) Remove "sending request" logging as requested.

Comment 21

[:garvan]

Added separate bug for mozilla-central: bug 1018383

Comment 22

[David Flanagan [:djf]]

Comment on attachment 8431785 [details] [diff] [review]
bug_1009592.diff

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

A few comments on this patch. The only serious issue is whether we should call update() when the cached position has not changed.

I'll try the patch out on my Tarako and report what I find.

::: dom/system/NetworkGeolocationProvider.js
@@ +75,5 @@
> +      if (smallerList[i].macAddress in wifiHash) {
> +        common++;
> +      }
> +    }
> +    let kPercentMatch = 0.5;

If it was me, I'd put this up at the top of the file with the other constants, but that's just a nit.

@@ +336,5 @@
>    },
>  
>    notify: function (timeoutTimer) {
> +    if (gCellScanningEnabled) {
> +      gCellResults = this.updateMobileInfo();

Notice that gCellResults doesn't have to be a global, since you set it here and clear it at the end of the function.

Also, updateMobileInfo() normally returns the results, but in the error case it sets gCellResults to null and returns undefined. Maybe worth fixing that so it always returns null.

@@ +350,5 @@
> +
> +    let useCached = isCachedLocationMoreAccurateThanServerRequest(data.cellTowers,
> +                                                                  data.wifiAccessPoints);
> +    if (useCached) {
> +      this.listener.update(gCachedLocation.location);

I wish we didn't have to keep sending the same cached location to the watchPosition() callback every 5 seconds.  Could you modify the startup() method to set a 'firstime' flag when it is invoked, and to do it even if it is already started?

Then check that flag here.  If there was a new call to getPosition() or watchPosition() since last time, then pass the cached position to update. Otherwise, don't send it.

I think that is how watchPosition() is supposed to work.

But maybe a change like that is too risky for 1.3T?

@@ +396,2 @@
>      gWifiResults = gCellResults = null;
> +    LOG("sending " + jsonData);

This probably won't apply cleanly since the line above was changed in the other bug.

Comment 23

[Josh Matthews [:jdm]]

(In reply to David Flanagan [:djf] from comment #22)
> @@ +350,5 @@
> > +
> > +    let useCached = isCachedLocationMoreAccurateThanServerRequest(data.cellTowers,
> > +                                                                  data.wifiAccessPoints);
> > +    if (useCached) {
> > +      this.listener.update(gCachedLocation.location);
> 
> I wish we didn't have to keep sending the same cached location to the
> watchPosition() callback every 5 seconds.  Could you modify the startup()
> method to set a 'firstime' flag when it is invoked, and to do it even if it
> is already started?
> 
> Then check that flag here.  If there was a new call to getPosition() or
> watchPosition() since last time, then pass the cached position to update.
> Otherwise, don't send it.
> 
> I think that is how watchPosition() is supposed to work.
> 
> But maybe a change like that is too risky for 1.3T?

Definitely too risky, and I'm pretty sure we don't have easy access to that information in the geolocation provider right now.

Comment 24

[:garvan]

> ::: dom/system/NetworkGeolocationProvider.js
> @@ +75,5 @@
> > +      if (smallerList[i].macAddress in wifiHash) {
> > +        common++;
> > +      }
> > +    }
> > +    let kPercentMatch = 0.5;
> 
> If it was me, I'd put this up at the top of the file with the other
> constants, but that's just a nit.

IIRC, Code Complete makes an argument that if you do that, it implies it is used in multiple places, moreover, it argues that the reader would most logically look for these values immediately beside their place of usage. I will double check this for future so I am not misquoting.
 
> @@ +336,5 @@
> >    },
> >  
> >    notify: function (timeoutTimer) {
> > +    if (gCellScanningEnabled) {
> > +      gCellResults = this.updateMobileInfo();
> 
> Notice that gCellResults doesn't have to be a global, since you set it here
> and clear it at the end of the function.
> 
> Also, updateMobileInfo() normally returns the results, but in the error case
> it sets gCellResults to null and returns undefined. Maybe worth fixing that
> so it always returns null.

Will fix.
UpdateMobileInfo() and the cell global isn't my code, I just noticed that there was a missing assignment. How was this code ever working I wonder? 
 
> @@ +396,2 @@
> >      gWifiResults = gCellResults = null;
> > +    LOG("sending " + jsonData);
> 
> This probably won't apply cleanly since the line above was changed in the
> other bug.

Agreed, my understanding is to avoid patches that are stacked, so this will have to get merged with that change.

Comment 25

[:garvan]

Attachment: bug_1009592.diff

Not sure if this should have been filed as a separate bug, but as :djf found, the updateMobileInfo() was assigning to a global in one case, and not another, and the cell data global is not necessary at all. 
Also this change resulted in removing a potential merge conflict with 1007953, as the null assignment to the cell data global is no longer needed at all.

Comment 26

[:garvan]

Attachment: bug_1009592.diff

As noted by :djf, need to ALSO clear gWifiResults on shutdown, or we could end up using the same results indefinitely.

Comment 27

[:garvan]

Attachment: bug_1009592.diff

As noted by :djf, need to ALSO clear gWifiResults on shutdown, or we could end up using the same results indefinitely.

Comment 28

[David Flanagan [:djf]]

Here's what I find when I try this patch out:

1) When I launch the camera app (with wifi enabled and a sim card in the phone)

When the camera first starts, the first location request is made before we've gotten wifi data, so it is a cell-only location request.  Then, the next time the timer fires, we do a cell+wifi request, and after that it just uses the cached location (because I'm not moving around).  Strangely, the cell-only request gives a pretty inaccurate location, with accuracy of 20000m.  When we do the cell+wifi request, the returned location is pretty close (maybe 2000m off) but the accuracy field is 50000.  But that's a bug in the MLS service, not in geolocation.

2) I killed the camera app, turned off Wifi in settings and relaunched the camera app.

I saw startup get called, and then a location request was sent out using my cell data plus the cached wifi data. This surprised me because I thought the patch was clearing the wifi data on shutdown. Looking at the code, I see it clears the cached location, but not the raw wifi data. Interestingly, my cell id changed on the second timer and then switched back on the third, so three queries were made with cell data plus old wifi data.  The returned location was off by 3 degress of latitude. I do have a California area code on this SIM, so maybe it was returning a california location instead of my Washington location? Presumably that is another MLS bug, not a geolocation issue. After a while, I started seeing the WifiWorker error from bug 1014924, but that is expected, I suppose.

3) Next I removed my sim card (it was in slot 2) and tried again with no sim and no wifi.  The location provider started up, and repeatedly tried to send this payload:

   sending {"cellTowers":[]}

But without a sim card or a wifi connection, it is not possible to contact the location service, so this always fails and the onerror handler of the xhr object is invoked. I'd think it is worth handling this case and punting before setting up the xhr request, but probably not a big deal.

4) With no sim and wifi on

The first time the timer fires, we don't have wifi scan results yet, so we end up sending the "cellTowers":[] body as above. This time there is a network connection, so the location request gets sent but fails (I've added logging) with status 400 and this body: {"error":{"code":400,"message":"Parse Error","errors":[{"domain":"global","message":"Parse Error","reason":"parseError"}]}}  I suspect that the location server doesn't like an empty array of cell towers and would prefer an empty body or something.  We should probably just avoid sending a request in this no-data case and this problem will go away.

After that first failed request, we get some wifi data and the next time the timer fires, we make a valid request (with an empty cell towers array) and get a location response.  After that we keep using the cached value.

5) Now I put my sim card back in (but in slot 1 this time instead of slot 2) and leave wifi on

We start up, make a cell-only location request and then make a cell+wifi location request.

Overall, this seems pretty good. We're not making unnecessary requests to the server, which is the main thing to me.  There's the issues of the stale wifi data being retained across shutdowns, but I think you've already fixed that.

And there's the issues of making cell-only requests before we get wifi data. That isn't a realy problem for the watchPosition() case, but is problematic for the getPosition() case, I think. Can we work around this (or minimize it) by making the timer 6 seconds instead of 5 (assuming that the wifi scanning is based on a 5s timer)?

Comment 29

[Hanno Schlichting]

(In reply to David Flanagan [:djf] from comment #28)
> 4) With no sim and wifi on
> 
> The first time the timer fires, we don't have wifi scan results yet, so we
> end up sending the "cellTowers":[] body as above. This time there is a
> network connection, so the location request gets sent but fails (I've added
> logging) with status 400 and this body:
> {"error":{"code":400,"message":"Parse
> Error","errors":[{"domain":"global","message":"Parse
> Error","reason":"parseError"}]}}  I suspect that the location server doesn't
> like an empty array of cell towers and would prefer an empty body or
> something.  We should probably just avoid sending a request in this no-data
> case and this problem will go away.

Yes. The service will answer a request with the exact payload of '{}' and return a GeoIP based result. But if you send either cellTowers or wifiAccessPoints, one of those arrays has to actually contain an entry to be valid.

As a test, this works:

curl -i -k -XPOST -H "Content-Type: application/json" https://location.services.mozilla.com/v1/geolocate?key=test -d '{}'

Comment 30

[David Flanagan [:djf]]

Comment on attachment 8431881 [details] [diff] [review]
bug_1009592.diff

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

r+ with the following changes:

Change these lines

// if we don't see any wifi responses in 5 seconds, send the request.
let gTimeToWaitBeforeSending = 5000; //ms

To these:

// Poll every 6 seconds. Note that this value must be slightly longer than the wifi
// scanning interval, to maximize the likelihood that we'll have wifi results when
// we send our position request. If this value is 5000, then we often report cell-only
// locations and ignore the wifi data. This is a brittle hack, but it seems to work on
// the Tarako.
let gTimeToWaitBeforeSending = 6000; //ms

Also, change these lines:

    if (cellResults) {
      data.cellTowers = cellResults;
    }

To:

    if (cellResults && cellResults.length) {
      data.cellTowers = cellResults;
    }

This is per Hanno's comment above. Though it seems unlikely that it will ever make a difference in practice because we'll generally either have cell or wifi results or won't have an internet connection.

Comment 31

[David Flanagan [:djf]]

Actually, I take back the second requested change above. Please don't make that change because it might have implications I don't understand for the isCachedLocationMoreAccurate... method  Let's just change the 5000ms timeout to a 6000ms timeout and call it good.  Without that change I always get a cell-only location for the first update and with the chagne I always get a cell+wifi location on the first update, so I think we really want to do it.

Comment 32

[David Flanagan [:djf]]

Attachment: Garvan's patch plus the one change I requested in review

Doug,

Since Garvan is done for the day, this patch is Garvan's patch plus the one change I requested in my review (changing the 5000ms timeout to 6000ms). I'll leave it up to you to do final review and landing of this.

Comment 33

[Doug Turner (:dougt)]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/b5b43ae6b589

This will only land on 1.3T due to 1018537.

I am going to close this bug as we will want to do something similar for m-c (and maybe m-a) which doesn't break TIMEOUT.  Garvan, please file follow ups.

Comment 34

[:garvan]

See bug https://bugzilla.mozilla.org/show_bug.cgi?id=1018383, added notes to properly respect timeout and maximumAge on m-c.