Handle gelocation accuracy

RESOLVED FIXED in Firefox OS v2.0

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jrconlin, Assigned: ggp)

Tracking

unspecified
2.0 S6 (18july)
All
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.0+, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
On device GPS returns an "accuracy" value. Device should include this in the tracking information. The server should either display area of accuracy and normalize position to avoid "jumping", or discard results that do not have high grade accuracy.

Updated

4 years ago
Assignee: nobody → ggoncalves
blocking-b2g: --- → 2.0?
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86_64 → All
(Assignee)

Comment 1

4 years ago
Created attachment 8451800 [details] [review]
gaia pull request

This is actually a one line fix, but testing it was a bit cumbersome, so the diff ended up huge.

I refactored MockGeolocation a little bit to make testing easier, and moved the unit test file findmydevice_test.js into findmydevice_commands_test.js, because it only contained tests for commands. I then added a test for this bug (and all the necessary scaffolding) in the file findmydevice_test.js.
Attachment #8451800 - Flags: review?(lissyx+mozillians)

Updated

4 years ago
Target Milestone: --- → 2.0 S6 (18july)

Updated

4 years ago
Blocks: 1031567

Updated

4 years ago
Summary: Handle gelocation accuracy → Handle geolocation accuracy

Updated

4 years ago
blocking-b2g: 2.0? → 2.0+
Summary: Handle geolocation accuracy → Handle gelocation accuracy
Comment on attachment 8451800 [details] [review]
gaia pull request

Thanks for splitting this in a more readable way! I already made some comments on the real payload of the patch. There are some nits and some things that would need to be addressed, otherwise it looks good.

I'll have a look at the moving parts.
And now that I was about to test the patch on my device, I'm getting 401 again. This seems to be related to the switching to the production server from the stage one.
My device is properly registered but the production website cannot connect to it, that will make testing this patch very hard ...
After switching back to stage server (which are supposed to have the code for the geolocation accuracy), stage website can connect to my device but the geolocation does not even start on the device.

This may be because of bug 1036423.
As agreed on IRC, I'm adding needinfo? for you so that you can make sure the accuracy sent by the device to the server is in the proper format, after bug 1036423 is fixed and before accuracy is used on stage/prod :)
Flags: needinfo?(jrconlin)
Thanks for fixing the previous feedback. I did some other comments, we are heading towards the proper direction. Meanwhile, do you mind addressing my latests comments and squash you patch but keep the separation that ease reviewing for now, since it helps a lot ?
Flags: needinfo?(ggoncalves)
(In reply to Alexandre LISSY :gerard-majax from comment #6)
> As agreed on IRC, I'm adding needinfo? for you so that you can make sure the
> accuracy sent by the device to the server is in the proper format, after bug
> 1036423 is fixed and before accuracy is used on stage/prod :)

Okay this bug has been fixed, my Flame got localized on FMD Stage. Device ID is 2033d21e40b2d79380a858449b9fbbbe, can you check that the accuracy is properly sent ? LOcation has been acquired via MLS as far as I can say.
(In reply to Alexandre LISSY :gerard-majax from comment #8)
> (In reply to Alexandre LISSY :gerard-majax from comment #6)
> > As agreed on IRC, I'm adding needinfo? for you so that you can make sure the
> > accuracy sent by the device to the server is in the proper format, after bug
> > 1036423 is fixed and before accuracy is used on stage/prod :)
> 
> Okay this bug has been fixed, my Flame got localized on FMD Stage. Device ID
> is 2033d21e40b2d79380a858449b9fbbbe, can you check that the accuracy is
> properly sent ? LOcation has been acquired via MLS as far as I can say.

> E/GeckoConsole(  326): *** WIFI GEO: gls returned status: 200 --> {"location":{"lat":48.8717855,"lng":2.3413425},"accuracy":100}
> I/Gecko   (  326): *** WIFI GEO: gls returned status: 200 --> {"location":{"lat":48.8717855,"lng":2.3413425},"accuracy":100}
> I/GeckoDump( 1035): [findmydevice] updating location to (48.8717855, 2.3413425)

If the server side properly received this accuracy, then we are good :)
Comment on attachment 8451800 [details] [review]
gaia pull request

Thanks for addressing all the comments. That all seems to be good now. Let's wait for green try and for making sure the server side was okay, then it can be merged :)
Attachment #8451800 - Flags: review?(lissyx+mozillians) → review+
(Reporter)

Comment 11

4 years ago
(In reply to Alexandre LISSY :gerard-majax from comment #9)
> (In reply to Alexandre LISSY :gerard-majax from comment #8)
> > (In reply to Alexandre LISSY :gerard-majax from comment #6)
> > > As agreed on IRC, I'm adding needinfo? for you so that you can make sure the
> > > accuracy sent by the device to the server is in the proper format, after bug
> > > 1036423 is fixed and before accuracy is used on stage/prod :)
> > 
> > Okay this bug has been fixed, my Flame got localized on FMD Stage. Device ID
> > is 2033d21e40b2d79380a858449b9fbbbe, can you check that the accuracy is
> > properly sent ? LOcation has been acquired via MLS as far as I can say.
> 
> > E/GeckoConsole(  326): *** WIFI GEO: gls returned status: 200 --> {"location":{"lat":48.8717855,"lng":2.3413425},"accuracy":100}
> > I/Gecko   (  326): *** WIFI GEO: gls returned status: 200 --> {"location":{"lat":48.8717855,"lng":2.3413425},"accuracy":100}
> > I/GeckoDump( 1035): [findmydevice] updating location to (48.8717855, 2.3413425)
> 
> If the server side properly received this accuracy, then we are good :)

Yep, client is sending accuracy to the stage box:
### Sending update to 1 pages: {"Latitude":48.87187313,"Longitude":2.34102594,"Altitude":0,"Accuracy":49,"Time":1404993510000,"Cmd":{"t":{"acc":49,"la":48.87187313,"lo":2.34102594,"ok":true,"ti":1.40499351e+12}}}
Flags: needinfo?(jrconlin)
(Assignee)

Updated

4 years ago
Flags: needinfo?(ggoncalves)
(Assignee)

Comment 12

4 years ago
Travis had an infrastructure error, plus two other errors in completely unrelated tests which I had already seen before, when landing bug 1030448. I think it's safe to check this in, but sheriffs, let me know if I should re-run this when Travis is green again.
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/f8aa626eecc1690ca5d7991d08160ba3a909623b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Reverted from 2.0 for Gaia unit test failures.
v2.0: https://github.com/mozilla-b2g/gaia/commit/c24d115bb51e40d5cb28c49324102b893966944b

https://tbpl.mozilla.org/php/getParsedLog.php?id=43621158&tree=Mozilla-Aurora
status-b2g-v2.0: fixed → affected
Flags: needinfo?(ggoncalves)
Keywords: branch-patch-needed
(Assignee)

Comment 16

4 years ago
Created attachment 8455433 [details] [review]
v2.0 pull request

We required a file that was introduced in bug 1004973. After consulting with the author, I found out that the patch is not being uplifted, so I'm just copying the file from master.

https://bugzilla.mozilla.org/show_bug.cgi?id=1004973#c46
Flags: needinfo?(ggoncalves)
(Assignee)

Updated

4 years ago
Keywords: branch-patch-needed
You need to log in before you can comment on or make changes to this bug.